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

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3343 bytes --]

On 2018-08-15 10:12, Markus Armbruster wrote:
> Max Reitz <mreitz@redhat.com> 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.

>>> 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.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]