From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43449) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VotPY-0006FM-4w for qemu-devel@nongnu.org; Fri, 06 Dec 2013 06:17:38 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VotPS-0006Fm-3l for qemu-devel@nongnu.org; Fri, 06 Dec 2013 06:17:32 -0500 Received: from mx1.redhat.com ([209.132.183.28]:46581) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VotPR-0006El-Rr for qemu-devel@nongnu.org; Fri, 06 Dec 2013 06:17:26 -0500 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id rB6BHPPJ025503 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Fri, 6 Dec 2013 06:17:25 -0500 Message-ID: <52A1B26F.3000702@redhat.com> Date: Fri, 06 Dec 2013 12:18:07 +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> <52A1B1B3.6040808@redhat.com> In-Reply-To: <52A1B1B3.6040808@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:14, Max Reitz wrote: > 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=20 >>> two: >>> The legacy filename option of -drive is passed as a separate paramete= r >>> 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 li= ke >>> the legacy option. We need to move the parsing of the 'file' option=20 >>> from >>> blockdev_init() to drive_init() to make it work this way. It's the=20 >>> right >>> 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 th= at >>> 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 ca= ll >>> 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., = they >>>> 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, blkde= bug-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=20 > some protocols =E2=80=93 or we just don't call it "file" (i.e., don't l= et=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. ;-) Er, make that bdrv_open_common() instead of bdrv_file_open(), obviously. Max