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

[Laurent Vivier]

Hi,

you can find attached a patch trying to manage your comments.

This patch adds parameter "-drive" and manages all drives type supported
by qemu (ide, scsi, sd, mtd, floppy, pflash). It adds a DriveInfo
structure to do that.

It applies on top of patch I posted 2 days ago
(http://article.gmane.org/gmane.comp.emulators.qemu/21011 )

All comments are welcome,
Laurent

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++.
> 
> - 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.
-- 
------------- address@hidden  --------------
 "In short: just say NO TO DRUGS and maybe you won't
   end up like the Hurd people." -- Linus Torvald

drive-arg.patch
Description: Text Data

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