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

[Fabrice Bellard]

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++.

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

- 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 ?

- 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.

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

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

- 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...

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

Regards,

Fabrice.