From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39020) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Vot8N-0007fQ-G6 for qemu-devel@nongnu.org; Fri, 06 Dec 2013 05:59:53 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Vot8H-0000FZ-DB for qemu-devel@nongnu.org; Fri, 06 Dec 2013 05:59:47 -0500 Received: from mx1.redhat.com ([209.132.183.28]:51068) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Vot8H-0000FR-58 for qemu-devel@nongnu.org; Fri, 06 Dec 2013 05:59:41 -0500 Received: from int-mx12.intmail.prod.int.phx2.redhat.com (int-mx12.intmail.prod.int.phx2.redhat.com [10.5.11.25]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id rB6AxenS018303 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Fri, 6 Dec 2013 05:59:40 -0500 Message-ID: <52A1AE46.6020504@redhat.com> Date: Fri, 06 Dec 2013 12:00:22 +0100 From: Max Reitz MIME-Version: 1.0 References: <52A0BAD8.90001@redhat.com> <20131206104338.GA2911@dhcp-200-207.str.redhat.com> In-Reply-To: <20131206104338.GA2911@dhcp-200-207.str.redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [RFC] Using BlockdevRef in the block layer List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: "qemu-devel@nongnu.org" , Stefan Hajnoczi On 06.12.2013 11:43, Kevin Wolf wrote: > Am 05.12.2013 um 18:41 hat Max Reitz geschrieben: >> When trying to implement this, I hit the problem that BlockdevRef >> allows you to reference an existing block device; however, this >> seems currently unimplemented. This is further hindered by the fact >> how this reference is done: If you want to give a file to some >> driver such as qcow2 (or blkdebug) which references an existing >> block device, the option dict should look something like this: >> >> { >> 'driver': 'blkdebug', >> 'id': 'drive0-debug', >> 'file': 'drive0' >> } >> >> So, as you can see, the =E2=80=9Cfile=E2=80=9D value now is simply a s= tring (this is >> what distinguishes a reference from a "real" file, in which case it >> would be a dict). The problem is that blockdev_init() already uses >> this case for just specifying a filename without any further >> options. > Yes, -drive uses 'file' for the filename, but blockdev-add doesn't. > Inside bdrv_open(), there is a pretty clear distinction between the two= : > The legacy filename option of -drive is passed as a separate parameter > and never as an options QDict entry. If the option comes from QMP, > though, it will be in the QDict. > > /me checks the source code... > > Okay, that's not true today, even if it comes from QMP we treat it like > the legacy option. We need to move the parsing of the 'file' option fro= m > blockdev_init() to drive_init() to make it work this way. It's the righ= t > thing to do anyway. > >> My current solution is to ignore the =E2=80=9Cfile=E2=80=9D value in c= ase >> blockdev_init() is called from qmp_blockdev_add(), but this doesn't >> solve the general legacy issue. So, did I miss anything or is >> referencing an existing block device really not supported so far and >> the only meaning to the "file" option containing a pure string is >> specifying a filename? > It's not supported so far, but we want to have it. > > Ideally legacy option handling would be moved to drive_init() by > conversion to the new data structures. This might not be entirely > trivial with file names, though, so I think for now changing things > within block_open() and friends is okay. Okay, this makes sense. I'll try my best. ;-) >> Second, if specifying a reference to an existing device should >> really be supported, bdrv_open() should ideally not call >> bdrv_file_open() anymore, but a function bdrv_find_ref() instead >> which resolves a BlockdevRef structure (for simplicity, it appears >> to be easier to use a QDict equivalent to a BlockdevRef instead of >> the latter itself (since that results in many effectively redundant >> conversions to and from those representations)). > Agreed. Not sure about the function name (perhaps bdrv_open_ref is > clearer?), but otherwise I like this design; perhaps the reason is that > I suggested it myself earlier. ;-) > >> However, >> bdrv_file_open() supports parsing protocol filenames, which >> bdrv_find_ref() would not. As a result, it is probably best to call >> bdrv_find_ref() from bdrv_file_open() instead and leave bdrv_open() >> generally the way it is right now =E2=80=93 yes, this is a question. ;= -) >> (=E2=80=9CDo you agree?=E2=80=9D) > Perhaps what we need to do is to call bdrv_file_open() for the legacy > case (filename passed as a separate parameter to bdrv_open()), and call > bdrv_find_ref() when we have a 'file' QDict entry? Yes, that is what I referred to in my reply to the original RFC.=20 However, it seems easier to just do everything in bdrv_file_open(). >> Third, I planned to implement the blkdebug and blkverify QMP >> interface by just making them BlockdevOptionsGenericFormat (with the >> addition of a "test" BlockdevRef for blkverify). This will give them >> a =E2=80=9Cfile=E2=80=9D automatically. However, this makes them =E2=80= =9Cdrivers for the >> protocol level=E2=80=9D (or however this is properly called), i.e., th= ey >> need to specify bdrv_open() instead of bdrv_file_open() to work. But >> blkdebug and blkverify are their own protocols with the current >> interface: Making them require an underlying file will break the >> current interface with the filename specifying the required options. >> To resolve this, I added two new =E2=80=9Cinterfaces=E2=80=9D, blkdebu= g-qmp and >> blkverify-qmp, which reference the same functions as blkdebug and >> blkverify, respectively, however, they offer bdrv_open() instead of >> bdrv_file_open(). These new block drivers will thus not support the >> current interface, but they will be properly supported through the >> QMP interface. > Hm... This is ugly. > > I'm not entirely sure I understand why we can't keep using > bdrv_file_open() when inheriting from BlockdevOptionsGenericFormat. > Granted, we wouldn't get bs->file automatially opened by > bdrv_open_common() - but we don't today, and calling bdrv_file_open() > manually from blkdebug_open() works just fine. Yes, we wouldn't get it opened automatically. I liked it being opened=20 automatically. ;-) Okay, then I'll have to make it do without. Max