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 <mre...@redhat.com> > >>--- > >>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
