From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50041) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XKBDA-0002Wu-L2 for qemu-devel@nongnu.org; Wed, 20 Aug 2014 15:06:26 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XKBD4-00078x-BE for qemu-devel@nongnu.org; Wed, 20 Aug 2014 15:06:20 -0400 Received: from mx1.redhat.com ([209.132.183.28]:18662) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XKBD4-00078s-2s for qemu-devel@nongnu.org; Wed, 20 Aug 2014 15:06:14 -0400 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s7KJ6Ddr032727 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK) for ; Wed, 20 Aug 2014 15:06:13 -0400 Message-ID: <53F4F1A1.5000600@redhat.com> Date: Wed, 20 Aug 2014 21:06:09 +0200 From: Max Reitz MIME-Version: 1.0 References: <1405707901-8253-1-git-send-email-mreitz@redhat.com> <1405707901-8253-2-git-send-email-mreitz@redhat.com> <20140820150734.GH6122@noname.redhat.com> In-Reply-To: <20140820150734.GH6122@noname.redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit 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: Kevin Wolf Cc: qemu-devel@nongnu.org, Stefan Hajnoczi 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. 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()). Max