From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44043) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1axbhQ-0003or-2c for qemu-devel@nongnu.org; Tue, 03 May 2016 10:53:26 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1axbhD-00071A-W6 for qemu-devel@nongnu.org; Tue, 03 May 2016 10:53:14 -0400 References: <1461706338-20219-1-git-send-email-mreitz@redhat.com> <1461706338-20219-7-git-send-email-mreitz@redhat.com> <20160502153611.GJ4882@noname.redhat.com> <57288929.7050804@redhat.com> <20160503133136.GF3917@noname.str.redhat.com> <5728AC3A.8080002@redhat.com> <20160503143453.GI3917@noname.str.redhat.com> From: Max Reitz Message-ID: <5728BB2C.3060801@redhat.com> Date: Tue, 3 May 2016 16:52:28 +0200 MIME-Version: 1.0 In-Reply-To: <20160503143453.GI3917@noname.str.redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="hv2Ktp7UdM4h8BWFkF5nG1Vpb5kUaWvMt" Subject: Re: [Qemu-devel] [PATCH 06/19] block: Make bdrv_default_refresh_format_filename public List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: qemu-block@nongnu.org, qemu-devel@nongnu.org, Alberto Garcia , Eric Blake This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --hv2Ktp7UdM4h8BWFkF5nG1Vpb5kUaWvMt Content-Type: multipart/mixed; boundary="6uG0fM4rM774G1DP9IXIWU3duQRbdifNs" From: Max Reitz To: Kevin Wolf Cc: qemu-block@nongnu.org, qemu-devel@nongnu.org, Alberto Garcia , Eric Blake Message-ID: <5728BB2C.3060801@redhat.com> Subject: Re: [PATCH 06/19] block: Make bdrv_default_refresh_format_filename public References: <1461706338-20219-1-git-send-email-mreitz@redhat.com> <1461706338-20219-7-git-send-email-mreitz@redhat.com> <20160502153611.GJ4882@noname.redhat.com> <57288929.7050804@redhat.com> <20160503133136.GF3917@noname.str.redhat.com> <5728AC3A.8080002@redhat.com> <20160503143453.GI3917@noname.str.redhat.com> In-Reply-To: <20160503143453.GI3917@noname.str.redhat.com> --6uG0fM4rM774G1DP9IXIWU3duQRbdifNs Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: quoted-printable On 03.05.2016 16:34, Kevin Wolf wrote: > Am 03.05.2016 um 15:48 hat Max Reitz geschrieben: >> On 03.05.2016 15:31, Kevin Wolf wrote: >>> Am 03.05.2016 um 13:19 hat Max Reitz geschrieben: >>>> On 02.05.2016 17:36, Kevin Wolf wrote: >>>>> Am 26.04.2016 um 23:32 hat Max Reitz geschrieben: >>>>>> In order to allow block drivers to use that function, it needs to = be >>>>>> public. In order to be useful, it needs to take a parameter which = allows >>>>>> the caller to specify whether the runtime options allowed by the b= lock >>>>>> driver are actually significant for the guest-visible BDS content.= >>>>>> >>>>>> Signed-off-by: Max Reitz >>>>> >>>>> Is this actually good enough? I expect that many drivers will have = some >>>>> options that are significant and other options that aren't. We alre= ady >>>>> have some (Quorum: children are significant, rewrite-corrupted isn'= t), >>>>> but as we convert more things to proper options, we'll get more of = them >>>>> (raw-posix: filename is significant, aio=3Dnative isn't). >>>>> >>>>> We might actually need to pass a list of significant fields instead= that >>>>> append_open_options() can use. >>>> >>>> Well, in theory, every driver with insignificant options would just >>>> implement .bdrv_refresh_filename() however it's needed. Making >>>> bdrv_default_refresh_format_filename() function public is just a way= of >>>> keeping that implementation very simple for some drivers that only h= ave >>>> insignificant options. >>>> >>>> I'm not opposed to extending this function in the future when it >>>> actually makes sense. Right now I don't think it does. The only thin= g >>>> that changes if a significant option is detected is that no plain >>>> filename is generated; however, for Quorum we can never generate suc= h a >>>> filename. Therefore, we cannot use this function for Quorum anyway. >>> >>> If you integrate it into append_open_options(), I suppose it would al= so >>> mean that insignificant options are dropped from the json: descriptio= n, >>> i.e. Quorum would return a json: object with all children, but not th= e >>> rewrite-corrupted setting. Which I think would be a good thing. >> >> I'm not sure I do. :-) >> >> At least right now the JSON version is supposed to contain all options= , >> be they significant or not. Let me try to remember my reasoning: >> >> Ideally, we want to get a filename which *exactly* results in the same= >> BDS that we have. >=20 > Here I'm not sure we do. :-) >=20 > We already don't do this. We filter out any options that are parsed by > the block layer. For example, we don't include the node name or caching= > options. If we really wanted to represend the BDS as exactly as we can,= > this wouldn't be right and we'd have to fix it. >=20 > But as I see it, what we were really after when we implemented things > this way was that we distinguish options that are conceptually part of > some address that points to the image data (which I thought matches you= r > "significant" options) and other options that just influence our access= > patterns and what we do with the image at this address. >=20 > The filename (json: or not) consists then only of the address part, as > the other options can differ between qemu invocations without actually > changing which image we see. I don't expect something called a filename= > (json: exists just because a plain filename can't represent everything)= > to contain various runtime configuration settings, but just a pure > pointer to the image. >=20 >> This should always be possible if instead of a plain >> filename one specifies options, e.g. using a JSON filename. However, >> such a JSON filename (or giving options using the dot syntax or as JSO= N >> with blockdev-add) is cumbersome. >> >> In many simple cases, we can (re-)construct a plain filename which >> yields exactly the same BDS, though. That's nice so that's what we try= >> to do first. >> >> In some cases, it is impossible to construct a plain filename which >> yields a BDS that will return the same data when accessed. Then, we ju= st >> cannot give such a filename and have to stick to a JSON filename, >> there's no way around this. >> >> However, sometimes we are in a gray area. We can construct a plain >> filename which yields a slightly different BDS than the one we have; b= ut >> it will return the same data when accessed and thus it is "close >> enough". We then have to make a tradeoff between getting exactly the >> same BDS and having a nice and simple filename. I opted for the latter= =2E >=20 > I can imagine that there are use cases for some mechanism to return the= > JSON object that creates exactly the same BDS, like you seem to be > envisioning here. I just doubt that it's useful in those cases where we= > really wanted a filename and have to go for JSON because we can't do > anything more user friendly. >=20 >> However, if we do have to emit a JSON filename at some point in the tr= ee >> I think we've basically "lost" already. If we get to that point, we ma= y >> as well just emit all the options that have been used to construct the= >> BDS, even if they don't change the data it yields. >=20 > In places where you want a filename (which is mostly, if not > exclusively, messages for the user), emitting everything may just make > an already unfriendly message even worse. Well, I don't particularly care either way. Thus, I'd be fine with removing insignificant options even from full_open_options if you deem that useful, but that is something we don't do already so we'd have to implement it. I guess I can implement it in this series if I decide to address the issue you originally raised here ("Is this good enough?"). If it gets too much, I'd rather handle it in a follow-up, though. Max --6uG0fM4rM774G1DP9IXIWU3duQRbdifNs-- --hv2Ktp7UdM4h8BWFkF5nG1Vpb5kUaWvMt Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQEcBAEBCAAGBQJXKLssAAoJEDuxQgLoOKyto6gH/2YGyJI9rvVd66l3WvWvhPP8 ZYEPcrzh7wCICyuCxhU3RkN7+e8evM5XpKACvWFamk95qRfIcAPkG/nd572v40Gy zt9EeNRbvv6g509HWgtkItB2AXBkUoHiICLBwW2MCQ+GLL4y1vslA7VQeYqCT2Yb Mse+fi4biktkPnJXseZO/H/Q957PKMQvojQLEFdMLOhmsSUUsihhcs7CZGxKHUkj 3TAtMXOzhv7fOM0YR7CXqb7HPI8n6KboRGCGajTemxiBbkffS3OPjzUHOStFGT/S 9xDM+jPuFzppTSmympfbeWrB4QMtpmBmW+PTayIv7YONXxfbqzvtItzLPdJpgkY= =1Dir -----END PGP SIGNATURE----- --hv2Ktp7UdM4h8BWFkF5nG1Vpb5kUaWvMt--