Re: [Qemu-devel] [PATCH 07/22] qcow2-bitmap: introduce auto-loading bitmaps

[Max Reitz <mreitz@redhat.com>]

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

On 14.10.2016 20:44, Vladimir Sementsov-Ogievskiy wrote:
> On 01.10.2016 19:26, Max Reitz wrote:
>> On 30.09.2016 12:53, Vladimir Sementsov-Ogievskiy wrote:
>>> Auto loading bitmaps are bitmaps in Qcow2, with AUTO flag set. They are
> 
> ...
> 
>> +        goto fail;
>> +    }
>> +
>> +    /* loop is safe because next entry offset is calculated after
>> conversion to
>> Should it be s/safe/unsafe/?
> 
> loop is safe. _unsafe is related to absence of assert in a loop, as it
> loops through BE data. Bad wording, I'll change it somehow..

Yes, I know that the loop is safe in the sense of the word "safe", but I
meant that it's kind of confusing that the loop uses
"for_each_bitmap_dir_entry_unsafe" and thus is "unsafe", too.

Another idea would be to write "This is actually safe" instead of "loop
is safe".

>>> +     * cpu format */
>>> +    for_each_bitmap_dir_entry_unsafe(e, dir, size) {
>>> +        if ((uint8_t *)(e + 1) > dir_end) {
>>> +            goto broken_dir;
>>> +        }
>>> +
>>> +        bitmap_dir_entry_to_cpu(e);
>>> +
>>> +        if ((uint8_t *)next_dir_entry(e) > dir_end) {
>>> +            goto broken_dir;
>>> +        }
>>> +
>>> +        if (e->extra_data_size != 0) {
>>> +            error_setg(errp, "Can't load bitmap '%.*s' from '%s':"
>>> +                       "extra data not supported.", e->name_size,
>> Full stop again.
>>
>> Also, I'm not quite sure why you give the device/node name here. You
>> don't do that anywhere else and I think if we want to emit the
>> information where something failed, it should be added at some previous
>> point in the call chain.
> 
> For example, how? As I understand, I can emit it only by error_setg, so
> actually it would be better to add node and bitmap name to all
> error_setg in the code.. or create helper function for it.

error_prepend() would be the function.

This code will be invoked by any code that is opening an image. There
are of course a couple of places where that can be the case: For
external tools such as qemu-img, it's normally pretty clear which image
is meant (and it additionally uses error_reportf_err() to give you more
information).

For -drive, every error message will at least be prepended by the
corresponding -drive parameter, so that will help a bit.

blockdev-add, unfortunately, doesn't do anything like this. But the user
can of course choose to construct the BDS graph node by node and thus
always know which node an error originates from.

Anyway, if you want to add this information to every error message, you
should probably do so in bdrv_open_inherit(). The issue I'd take with
this is that most users probably won't set the node name themselves
(either they don't at all or it's some management tool that does), so
reporting the node name doesn't actually help them at all (and
management tools are not supposed to parse error messages, so it won't
help in that case either).

Max


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