Re: [Qemu-block] [Qemu-devel] [RFC PATCH] rbd: Don't convert keypairs to JSON and back

[Markus Armbruster]

Max Reitz <address@hidden> writes:

> On 2018-08-15 10:12, Markus Armbruster wrote:
>> Max Reitz <address@hidden> writes:
>
> [...]
>
>>> To me personally the issue is that if you can specify a plain filename,
>>> bdrv_refresh_filename() should give you that plain filename back.  So
>>> rbd's implementation of that is lacking.  Well, it just doesn't exist.
>> 
>> I'm not even sure I understand what you're talking about.
>
> We have this bdrv_refresh_filename() thing which can do this:
>
> $ qemu-img info \
>     "json:{'driver':'raw',
>            'file':{'driver':'nbd','host':'localhost'}}"
> image: nbd://localhost:10809
> [...]
>
> So it can reconstruct a plain filename even if you specify it as options
> instead of just using a plain filename.
>
>
> Now here's my fault: I thought it might be necessary for a driver to
> implement that function (which rbd doesn't) so that you'd get a nice
> filename back (instead of just json:{} garbled things).   But you don't.
>  For protocol drivers, you'll just get the initial filename back.  (So
> my comment was just wrong.)
>
> So what I was thinking about was some case where you specified a normal
> plain filename and qemu would give you back json:{}.  (If rbd
> implemented bdrv_refresh_filename(), that wouldn't happen, because it
> would reconstruct a nice normal filename.)  It turns out, I don't think
> that can happen so easily.  You'll just get your filename back.
>
> Because here's what I'm thinking: If someone uses an option that is
> undocumented and starts with =, well, too bad.  If someone uses a normal
> filename, but gets back a json:{} filename...  Then they are free to use
> that anywhere, and their use of "=" is legitimized.
>
>
> Now that issue kind of reappears when you open an RBD volume, and then
> e.g. take a blockdev-snapshot.  Then your overlay has an overridden
> backing file (one that may be different from what its image header says)
> and its filename may well become a json:{} one (to arrange for the
> overridden backing file options).  Of course, if you opened the RBD
> volume with a filename with some of the options warranting
> =keyvalue-pairs, then your json:{} filename will contain those options
> under =keyvalue-pairs.
>
> So...  I'm not quite sure what I want to say?  I think there are edge
> cases where the user may not have put any weird option into qemu, but
> they do get a json:{} filename with =keyvalue-pairs out of it.  And I
> think users are free to use json:{} filenames qemu spews at them, and we
> can't blame them for it.

Makes sense.

More reason to deprecate key-value pairs in pseudo-filenames.

The only alternative would be to also provide them in QAPI
BlockdevOptionsRbd.  I find that as distasteful as ever, but if the
block maintainers decide we need it, I'll hold my nose.

>>>> If so, and we are comfortable changing the output the way this patch does
>>>> (technically altering ABI anyway), we might as well go all the way and
>>>> filter it out completely.  That would be preferable to cleaning up the json
>>>> output of the internal key/value pairs, IMO.
>>>
>>> Well, this filtering at least is done by my "Fix some filename
>>> generation issues" series.
>> 
>> Likewise.
>
> The series overhauls quite a bit of the bdrv_refresh_filename()
> infrastructure.  That function is also responsible for generating
> json:{} filenames.
>
> One thing it introduces is a BlockDriver field where a driver can
> specify which of the runtime options are actually important.  The rest
> is omitted from the generated json:{} filename.
>
> I may have taken the liberty not to include =keyvalue-pairs in RBD's
> "strong runtime options" list.

I see.

Permit me to digress a bit.

I understand one application for generating a json: filename is for
putting it into an image file header (say as a COW's backing image).

Having image file headers point to other images is not as simple as it
may look at first glance.  The basic case of image format plus plain
filename (not containing '/') is straightforward enough.  But if I make
the filename absolute (with a leading '/'), the image becomes less easy
to move to another machine.

Similarly, certain Ceph configuration bits may make no sense on another
machine, and putting them into an image file header may not be a good
idea.

Rather vague, I'm afraid.