From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42886) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VotMW-0004gM-FE for qemu-devel@nongnu.org; Fri, 06 Dec 2013 06:14:30 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VotMQ-0005RR-DJ for qemu-devel@nongnu.org; Fri, 06 Dec 2013 06:14:24 -0500 Received: from mx1.redhat.com ([209.132.183.28]:59775) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VotMQ-0005RH-4o for qemu-devel@nongnu.org; Fri, 06 Dec 2013 06:14:18 -0500 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id rB6BEHrE023853 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Fri, 6 Dec 2013 06:14:17 -0500 Message-ID: <52A1B1B3.6040808@redhat.com> Date: Fri, 06 Dec 2013 12:14:59 +0100 From: Max Reitz MIME-Version: 1.0 References: <52A0BAD8.90001@redhat.com> <20131206104338.GA2911@dhcp-200-207.str.redhat.com> <52A1AE46.6020504@redhat.com> In-Reply-To: <52A1AE46.6020504@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 12:00, Max Reitz wrote: > 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 = string (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 tw= o: >> 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 lik= e >> the legacy option. We need to move the parsing of the 'file' option fr= om >> blockdev_init() to drive_init() to make it work this way. It's the rig= ht >> thing to do anyway. >> >>> My current solution is to ignore the =E2=80=9Cfile=E2=80=9D value in = case >>> 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 tha= t >> 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 cal= l >> 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., t= hey >>> 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, blkdeb= ug-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. ;-) Ah, and what's worse, bdrv_file_open() attempts a bdrv_swap() which is=20 bound to fail. That means, we either have to disable bdrv_swap() on some=20 protocols =E2=80=93 or we just don't call it "file" (i.e., don't let=20 BlockdevOptionsBlkdebug be based on BlockdevOptionsGenericFormat). I'd=20 prefer the second option, since then I don't even have to care about=20 nesting "file"s anymore. ;-) Max