From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1Iq3Iz-0005Lh-CV for qemu-devel@nongnu.org; Thu, 08 Nov 2007 04:04:05 -0500 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1Iq3Iy-0005LE-ST for qemu-devel@nongnu.org; Thu, 08 Nov 2007 04:04:05 -0500 Received: from [199.232.76.173] (helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Iq3Iy-0005LB-Jt for qemu-devel@nongnu.org; Thu, 08 Nov 2007 04:04:04 -0500 Received: from ecfrec.frec.bull.fr ([129.183.4.8]) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1Iq3Iw-0002KN-Og for qemu-devel@nongnu.org; Thu, 08 Nov 2007 04:04:03 -0500 Subject: Re: [Qemu-devel] [PATCH 1/3] Add args to -cdrom to define where is connected the cdrom From: Laurent Vivier In-Reply-To: <47324AFC.3080808@bellard.org> References: <11936114152690-git-send-email-Laurent.Vivier@bull.net> <11936114153499-git-send-email-Laurent.Vivier@bull.net> <20071029132525.GA7128@redhat.com> <4725E7ED.6050305@bull.net> <20071029143421.GD18384@networkno.de> <472B250C.4000709@bull.net> <47324AFC.3080808@bellard.org> Date: Thu, 08 Nov 2007 10:02:57 +0100 Message-Id: <1194512577.3640.19.camel@frecb07144> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="=-2W5NSA4rcxUs1JazkdeF" Reply-To: qemu-devel@nongnu.org List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Fabrice Bellard Cc: qemu-devel@nongnu.org --=-2W5NSA4rcxUs1JazkdeF Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Le jeudi 08 novembre 2007 =C3=A0 00:32 +0100, Fabrice Bellard a =C3=A9crit = : > Laurent Vivier wrote: > > Thiemo Seufer a =C3=A9crit : > >> Laurent Vivier wrote: > >>> Daniel P. Berrange a =C3=A9crit : > >>>> On Sun, Oct 28, 2007 at 11:43:33PM +0100, Laurent.Vivier@bull.net > >>>> wrote: > >>>>> From: Laurent Vivier > >>>>> > >>>>> This patch allows to define where is connected the CDROM device (bu= s, > >>>>> unit). > >>>>> It extends the "-cdrom" syntax to add these paramaters: > >>>>> > >>>>> -cdrom file[,if=3Dtype][,bus=3Dn][,unit=3Dm] > >>>>> > >>>>> 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=3Dtype][,bus=3Dn][,unit=3Dm][,mode=3Dmode] > >>>> 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 th= em. > >>> I agree with that. And if everyone agrees I can modify patches to do > >>> that... > >> > >> Please go ahead. :-) > >=20 > > 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 ? >=20 > I had rejected a similar patch in the past, but I agree that a -disk > command line option is needed. >=20 > A few remarks: >=20 > - 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 ? >=20 > - 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=3D" 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=3Da.img" instead of "-disk a.img" and "-drive file=3D" 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() ? >=20 > - 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, >=20 > Fabrice. Thank you for your comments, Laurent --=-2W5NSA4rcxUs1JazkdeF Content-Type: application/pgp-signature; name=signature.asc Content-Description: Ceci est une partie de message =?ISO-8859-1?Q?num=E9riquement?= =?ISO-8859-1?Q?_sign=E9e?= -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.6 (GNU/Linux) iD8DBQBHMtDB9Kffa9pFVzwRAtqEAJ0ZTTkr5JItTDq1O0xHP4fvUUTb3wCgvM1o 02nAEYNfEU/ZAW4LXO1CHgM= =YnAP -----END PGP SIGNATURE----- --=-2W5NSA4rcxUs1JazkdeF--