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

[Fabrice Bellard <fabrice@bellard.org>]

Laurent Vivier wrote:
> 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, Laurent.Vivier@bull.net
>>>>>> wrote:
>>>>>>> From: Laurent Vivier <vivierl@frecb07144.(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 ?

What is important is that MAX_IDE_BUS is bigger than the number of IDE
disks used by every machine. If the machine does not use them it is less
an issue. If you use DriveInfo you no longer have this problem.

> For the moment it is the sum of all available MAX_* .
> Should I take an explicit value like "32" or something like ?

If you use my suggestion of DriveInfo structure, this problem disappears
as you will have a single constant MAX_DISKS that you can set to a fixed
value.

>> - 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 ,"

Yes "-disk file=a.img" or "-disk media=cdrom". But I am open to other
suggestion. I was just surprised by the syntax "-disk ,media=cdrom".

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

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

As you wish.

Regards,

Fabrice.