Re: [Qemu-devel] [PATCH 1/3] Add args to -cdrom to define where is connected the cdrom

[Laurent Vivier]

Le jeudi 08 novembre 2007 à 00:32 +0100, Fabrice Bellard a écrit :
> Laurent Vivier wrote:
> > Thiemo Seufer a écrit :
> >> Laurent Vivier wrote:
> >>> Daniel P. Berrange a écrit :
> >>>> On Sun, Oct 28, 2007 at 11:43:33PM +0100, address@hidden
> >>>> wrote:
> >>>>> From: Laurent Vivier <address@hidden(none)>
> >>>>>
> >>>>> This patch allows to define where is connected the CDROM device (bus,
> >>>>> unit).
> >>>>> It extends the "-cdrom" syntax to add these paramaters:
> >>>>>
> >>>>>      -cdrom file[,if=type][,bus=n][,unit=m]
> >>>>>
> >>>>>  where "type" defines the interface (by default, "ide")
> >>>>>        "n" defines the bus number (by default 1)
> >>>>>        "m" defines the unit number (by default 0)
> >>>> Having a separately named arg just for CDROMs was always rather
> >>>> odd/unhelpful.
> >>>> I'd suggest that we leave all the -hda,hdb,hdc,-cdrom,-fda,-fdb etc
> >>>> unchanged
> >>>> and use the -disk for setting up all types of disks, floppys,
> >>>> cdroms, etc. It
> >>>> would just require one extra field for the -disk arg:
> >>>>       -disk file[,if=type][,bus=n][,unit=m][,mode=mode]
> >>>>    where "type" defines the interface. [ide,scsi,fd] (by default,
> >>>> "ide")
> >>>>         "n" defines the bus number (by default 1)
> >>>>         "m" defines the unit number (by default 0)
> >>>>         "mode" defines one of [disk,floppy,cdrom]
> >>>> If we ever up able to emulate other types of SCSI / IDE devices
> >>>> (tape drives,
> >>>> cdr, dvd perhaps) then the 'mode' can easily be extended to cover them.
> >>> I agree with that. And if everyone agrees I can modify patches to do
> >>> that...
> >>
> >> Please go ahead. :-)
> > 
> > Well, it is done... is there someone that can comment them ?
> > Or if they are perfect (as usual ;-) ) perhaps it could be included in
> > CVS ?
> 
> I had rejected a similar patch in the past, but I agree that a -disk
> command line option is needed.
> 
> A few remarks:
> 
> - Add a function in vl.c to add a disk so that all option handlers can
> call it instead of doing pstrcpy(); nb_disk++.

I agree

> - You replaced the constant "2" in many machines by MAX_IDE_BUS. It is
> dangerous because each machine has its own constraints.

I agree, I'll use MAX_IDE_BUS only for PC.
But how can I compute MAX_DISKS if the number of IDE disks vary with
target ?
For the moment it is the sum of all available MAX_* .
Should I take an explicit value like "32" or something like ?

> 
> - Maybe the real option name could be "-drive" to insist on the fact
> that a drive can be created without a disk in it ! Any more comments ?

I agree, and a drive can be something else like a flash or a tape (in
the future...).

> - disk_init() should not modify its argument str. Moreover, maybe having
> an explicit "file=" argument would be clearer in the case there is no
> disk in the drive.

I'm not sure I understand correctly this one.
For the moment "-disk ," allows to declare a drive without media (an
empty CDROM drive, for instance).
Do you mean we must use "-drive file=a.img" instead of "-disk a.img" and
"-drive file=" instead of "-disk ,"

> - While modifying the machine init function, you can suppress the
> snapshot parameter.

OK

> - In disk_init(), you should factorize the bdrv_new() and bdrv_open() as
> it is the same for all types.

OK.
Can I use bdrv_set_geometry_hint() and bdrv_set_translation_hint()
before the bdrv_open() ?

> 
> - It could be simpler to export an array of structs containg a bdrv,
> each one tagged with if, index and bus. Then each machine could call a
> function to get the bdrv from the parameters "if", "index" and "bus".
> For example, look at the NICInfo structure and do the same with a
> structure DriveInfo...

OK

> If you prefer, you can resubmit a big patch with all the changes.

Well, I like to write a lot of little patches ;-)
It is easier to review them, isn't it ?
But it is as you want.

> Regards,
> 
> Fabrice.

Thank you for your comments,
Laurent

signature.asc
Description: Ceci est une partie de message numériquement signée