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

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

On 15.10.2016 20:03, Max Reitz wrote:
> 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".

Finally I've decided to introduce normal list of normal structures like 
in snapshots..

>
>>>> +     * 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
>

Thanks for explanations, and for the whole review, it's great! Sorry for 
my laziness and for spelling(
O! I've discovered vim spell, so one problem less, I hope.

-- 
Best regards,
Vladimir