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, [EMAIL PROTECTED] > >>>> wrote: > >>>>> From: Laurent Vivier <[EMAIL PROTECTED](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
