Re: [Qemu-devel] [PATCH 1/6] block: Add bdrv_refresh_filename()

[Max Reitz]

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 <address@hidden>
> > ---
> > 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