Re: [Qemu-block] [PATCH] block: allow best-effort query

[Max Reitz]

On 26.10.2015 22:34, John Snow wrote:
> 
> 
> On 10/26/2015 05:18 PM, Max Reitz wrote:
>> On 26.10.2015 19:12, John Snow wrote:
>>> For more complex BDS trees that can be created under normal circumstances,
>>> we lose the ability to issue query commands because of our inability to
>>> re-construct the absolute filename.
>>>
>>> Instead, omit this field when it is a problem and present as much 
>>> information
>>> as we can.
>>>
>>> This will change the expected output in iotest 110, where we will now see a
>>> json filename and the lack of an absolute filename instead of an error.
>>>
>>> Signed-off-by: John Snow <address@hidden>
>>> ---
>>>  block/qapi.c               | 10 ++++++----
>>>  tests/qemu-iotests/110.out |  5 ++++-
>>>  2 files changed, 10 insertions(+), 5 deletions(-)
>>
>> One problem I see now is that qemu-img --backing-chain uses the full
>> backing name if it is available and if it isn't, it resorts to the
>> non-full backing name (which was good until this patch, as far as I can
>> see). But now that's not necessarily a valid substitution anymore:
>>
>> $ mkdir foo
>> $ ./qemu-img create -f qcow2 foo/base.qcow2 64M
>> $ ./qemu-img create -f qcow2 -b base.qcow2 foo/top.qcow2
>> $ ./qemu-img create -f qcow2 base.qcow2 64M
>> $ ./qemu-img info --backing-chain foo/top.qcow2
>> image: foo/top.qcow2
>> file format: qcow2
>> virtual size: 64M (67108864 bytes)
>> disk size: 196K
>> cluster_size: 65536
>> backing file: base.qcow2 (actual path: foo/base.qcow2)
>> Format specific information:
>>     compat: 1.1
>>     lazy refcounts: false
>>     refcount bits: 16
>>     corrupt: false
>>
>> image: foo/base.qcow2
>> file format: qcow2
>> virtual size: 64M (67108864 bytes)
>> disk size: 196K
>> cluster_size: 65536
>> Format specific information:
>>     compat: 1.1
>>     lazy refcounts: false
>>     refcount bits: 16
>>     corrupt: false
>> $ ./qemu-img info --backing-chain \
>> "json:{'file.filename':'foo/top.qcow2',
>>        'driver':'qcow2','lazy-refcounts':true}"
>> image: json:{"lazy-refcounts": true, "driver": "qcow2", "file":
>> {"driver": "file", "filename": "foo/top.qcow2"}}
>> file format: qcow2
>> virtual size: 64M (67108864 bytes)
>> disk size: 196K
>> cluster_size: 65536
>> backing file: base.qcow2
>> Format specific information:
>>     compat: 1.1
>>     lazy refcounts: false
>>     refcount bits: 16
>>     corrupt: false
>>
>> image: base.qcow2
>> file format: qcow2
>> virtual size: 32M (33554432 bytes)
>> disk size: 196K
>> cluster_size: 65536
>> Format specific information:
>>     compat: 1.1
>>     lazy refcounts: false
>>     refcount bits: 16
>>     corrupt: false
>>
>> As you can see, in the second case, the wrong base.qcow2 was used. I
>> think qemu-img info --backing-chain should abort once it hits a point
>> where has_full_backing_filename is false; but for that to work, we need
>> to set info->full_backing_filename even if the "relative" and the
>> absolute backing filename (backing_filename and backing_filename2) are
>> equal.
>>
>> (By the way: It's actually some kind of a bug that you can open images
>> with JSON filenames and backing files; this only works because
>> bdrv_open_backing_file() is called before bdrv_refresh_filename().
>> Actually, after my "Drop BDS.filename" series it will no longer work.
>> This is bad. I need to fix this before I can continue the series...)
>>
>> ((It is bad because that means:
>> $ x86_64-softmmu/qemu-system-x86_64 \
>>   -drive if=none,file.filename=foo/top.qcow2,lazy-refcounts=on
>> qemu-system-x86_64: -drive
>> if=none,file.filename=foo/top.qcow2,lazy-refcounts=on: Cannot use
>> relative backing file names for 'json:{"lazy-refcounts": "on", "driver":
>> "qcow2", "file": {"driver": "file", "filename": "foo/top.qcow2"}}'
>>
>> And that's bad.))
>>
>> (((So for this patch that means I guess we should just fix
>> bdrv_get_full_backing_filename() instead.)))
>>
>> Max
>>
> 
> Sigh, I guess there's no easy fixes within ten miles of this fifty car
> pileup.
> 
> What's your opinion for what a meaningful fix to
> bdrv_get_full_backing_filename might be?
> 
> It's not really rational for query to ever _fail_; I would be tempted to
> fix any users of query information to understand what to do if that
> field is absent.

After having thought about it, my current plan is this: Every protocol
driver can provide a function which basically returns a base directory
for a given BDS. If no such function is provided, we use something like
dirname(BDS.options['filename']) + '/'. A format BDS's base directory is
its file's base directory.

Then, if we have a relative backing filename, we use that function to
query the base directory. If we don't receive one (there may be block
drivers where it's simply not applicable, like quorum), then we still
fail. But we'd fail much less often, and only if there's really no way
around it.

So I think this patch is in principle still fine because
bdrv_get_full_backing_filename() can still fail (if your top BDS is
handled by a block driver which simply cannot use normal filenames, but
you are trying to attach a backing file with a relative filename).
Especially since we won't get the other stuff in before 2.5.

But I'd still like the qemu-img info --backing-chain stuff fixed. It's
not critical, but it shouldn't be too hard to fix it either.

Max

signature.asc
Description: OpenPGP digital signature