Re: [Qemu-devel] [PATCH] block: fix backing file segfault

[Kevin Wolf]

Am 10.01.2014 um 19:05 hat Max Reitz geschrieben:
> On 10.01.2014 18:55, Kevin Wolf wrote:
> >Ok, if you're happy with it, I'll apply it. Can I put your Reviewed-by
> >there?
> 
> Yes, feel free to.

Thanks, applied to the block branch.

Peter, no need for a second version of the patch then. :-)

> >In the long run, we need to get rid of all this copying anyway. I'm
> >imagining a BlockDriver function that returns a file name to reproduce
> >the same setup, and a removal of bs->backing_file and bs->file_name.
> >
> >For some drivers, the returned filename would be a URL or some other
> >string that that particular driver can parse.
> >
> >While doing that, we might also consider a fake protocol that handles
> >filenames like 'json:{"driver":"qcow2","lazy-refcounts":"on",...}',
> >because for some drivers this might be the only thing that comes close
> >to a filename as it is a single string at least...
> 
> Urgh. *g*
> 
> I'm not sure if we should force every BDS to have a clearly defining
> file name. If there are options, which completely change the
> behavior of the block driver (I wouldn't consider lazy-refcounts one
> of them since it doesn't change the contents of the block device),
> I'd rather return NULL when asked for a file name. But then again,
> maybe an ugly filename is better than none at all…

We need filenames for backing file relationships. For example, when you
take a live snapshot, we need to reference the old image. If you don't
use the filename, but driver-specific options, I believe this fails
today.

You might also want to set some options for the backing file in images
created with qemu-img.

The alternative would be to extend qcow2 to have something more complex
than a string to describe backing files. However, this would mean that
qcow2 is the only possible format for live snapshots.

> In general, I'd prefer abandoning filenames* (especially protocol
> filenames) altogether. The set of options with which to recreate the
> same BDS is already available.
> 
> Max
> 
> *Of course, we need filenames for, well, opening files, but I'm
> referring to have an explicit string "filename" in addition to the
> option dicts (nearly) everywhere.

Agreed. The reason why filenames are still passed separately and not
converted to a file.filename QDict entry is the convenience magic that
they enable (at least protocol names) and that file.filename doesn't
have in order to have less special cases with blockdev-add.

So what you'd need to do is to parse the protocol names in the top-level
function bdrv_open() and convert them into the right QDict entries.
Perhaps this is also a better place for the .bdrv_parse_filename()
calls. And then you could call a new bdrv_file_open() that doesn't take
a separate filename argument any more.

Kevin