From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1IpuPL-0007iA-Hm for qemu-devel@nongnu.org; Wed, 07 Nov 2007 18:34:03 -0500 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1IpuPK-0007hx-Ma for qemu-devel@nongnu.org; Wed, 07 Nov 2007 18:34:03 -0500 Received: from [199.232.76.173] (helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1IpuPK-0007hu-Iy for qemu-devel@nongnu.org; Wed, 07 Nov 2007 18:34:02 -0500 Received: from sp604002mt.neufgp.fr ([84.96.92.61] helo=sMtp.neuf.fr) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1IpuPK-0002vS-2Q for qemu-devel@nongnu.org; Wed, 07 Nov 2007 18:34:02 -0500 Received: from [84.102.211.92] by sp604002mt.gpm.neuf.ld (Sun Java System Messaging Server 6.2-5.05 (built Feb 16 2006)) with ESMTP id <0JR500MS3TED4FN0@sp604002mt.gpm.neuf.ld> for qemu-devel@nongnu.org; Thu, 08 Nov 2007 00:32:38 +0100 (CET) Date: Thu, 08 Nov 2007 00:32:12 +0100 From: Fabrice Bellard Subject: Re: [Qemu-devel] [PATCH 1/3] Add args to -cdrom to define where is connected the cdrom In-reply-to: <472B250C.4000709@bull.net> Message-id: <47324AFC.3080808@bellard.org> MIME-version: 1.0 Content-type: text/plain; charset=ISO-8859-1 Content-transfer-encoding: QUOTED-PRINTABLE 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> Reply-To: qemu-devel@nongnu.org List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org Cc: Laurent.Vivier@bull.net Laurent Vivier wrote: > Thiemo Seufer a =E9crit : >> Laurent Vivier wrote: >>> Daniel P. Berrange a =E9crit : >>>> On Sun, Oct 28, 2007 at 11:43:33PM +0100, Laurent.Vivier@bull.ne= t >>>> wrote: >>>>> From: Laurent Vivier >>>>> >>>>> 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=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 cove= r them. >>> 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 ? 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 ca= n call it instead of doing pstrcpy(); nb_disk++. - You replaced the constant "2" in many machines by MAX_IDE_BUS. It i= s 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 hav= ing an explicit "file=3D" 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.