From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50595) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XKNhs-0000fv-Am for qemu-devel@nongnu.org; Thu, 21 Aug 2014 04:26:58 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XKNhm-0006Mc-53 for qemu-devel@nongnu.org; Thu, 21 Aug 2014 04:26:52 -0400 Received: from mx1.redhat.com ([209.132.183.28]:12224) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XKNhl-0006MW-Te for qemu-devel@nongnu.org; Thu, 21 Aug 2014 04:26:46 -0400 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s7L8QimI002093 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK) for ; Thu, 21 Aug 2014 04:26:45 -0400 Date: Thu, 21 Aug 2014 10:26:41 +0200 From: Kevin Wolf Message-ID: <20140821082641.GD4452@noname.redhat.com> References: <1405707901-8253-1-git-send-email-mreitz@redhat.com> <1405707901-8253-2-git-send-email-mreitz@redhat.com> <20140820150734.GH6122@noname.redhat.com> <53F4F1A1.5000600@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <53F4F1A1.5000600@redhat.com> Subject: Re: [Qemu-devel] [PATCH 1/6] block: Add bdrv_refresh_filename() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Max Reitz Cc: qemu-devel@nongnu.org, Stefan Hajnoczi Am 20.08.2014 um 21:06 hat Max Reitz geschrieben: > On 20.08.2014 17:07, Kevin Wolf wrote: > >Am 18.07.2014 um 20:24 hat Max Reitz geschrieben: > >>Some block devices may not have a filename in their BDS; and for some, > >>there may not even be a normal filename at all. To work around this, add > >>a function which tries to construct a valid filename for the > >>BDS.filename field. > >> > >>If a filename exists or a block driver is able to reconstruct a valid > >>filename (which is placed in BDS.exact_filename), this can directly be > >>used. > >> > >>If no filename can be constructed, we can still construct an options > >>QDict which is then converted to a JSON object and prefixed with the > >>"json:" pseudo protocol prefix. The QDict is placed in > >>BDS.full_open_options. > >> > >>For most block drivers, this process can be done automatically; those > >>that need special handling may define a .bdrv_refresh_filename() method > >>to fill BDS.exact_filename and BDS.full_open_options themselves. > >> > >>Signed-off-by: Max Reitz > >>--- > >>In this version, bdrv_refresh_filename() leaves the filename unmodified > >>if neither a new filename nor an options QDict can be generated. Another > >>idea would be to clear the filename in this case as it is probably > >>obsolete then. I was not sure which to pick, so I just used the first > >>version I wrote. > >To be honest, many things in this patch don't feel quite right. This > >isn't necessarily your fault, I can imagine that the infrastructure is > >just lacking the right properties for you to use. > > > >My hope is that soon bs->options would be the only BDS field keeping > >configuration information and that bs->filename would go away. Now with > >this patch series we get both of them duplicated instead. I'm not quite > >sure if this is progress, but it may still be an acceptable intermediate > >step. > > This code just needs some way to cache the filename and I thought it > fine to use the filename field for that purpose. If we remove it, > we'll have to reconstruct it every time (recursively through all the > BDS layers) when someone needs it. Hm. Thinking more about it, the part that I really dislike isn't even that bs->filename exists and is used as a cache. It's just that the filename argument of bdrv_open() is used to initialise it instead of only using the options QDict. But that's an independent problem that isn't made worse by this series. I guess it's time to dig out the next part of my bdrv_open() series and complete that work... > We may be able to cull many such places (where the filename is > needed), but considering the processing time, I don't think it will > get any better than using a cache array (such as BDS.filename). > > So for me, the advantages of dumping BDS.filename would be: We don't > have to consider when the filename field needs to an update; we save > some memory (negligible, imho). > > The advantages of keeping it, on the other hand, are: It's easy to > read the filename (no need to change existing code); we may save > some processing time (probably also negligible, if done right). > > After considering this, I guess we'd have to look at all the places > which use BDS.filename, how many of those we can cull and how often > the rest is invoked. If BDS.filename is only rarely really needed, > we can really remove it and fully replace it by a callback (which > basically is bdrv_refresh_filename()). Most of it is probably in monitor command implementations. Kevin