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

[Vladimir Sementsov-Ogievskiy]

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

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

> > +                       dir_entry_name_notcstr(e),
> > +                       bdrv_get_device_or_node_name(bs));
> > +            goto fail;
> > +        }
> > +    }

...

> > +        if (ret < 0) {
> > +            goto fail;
> > +        }
> > +    }
> > +    s->bitmap_directory_offset = new_offset;
> > +    s->bitmap_directory_size = new_size;
> > +    s->nb_bitmaps = new_nb_bitmaps;
> > +
> > +    ret = update_header_sync(bs);
> > +    if (ret < 0) {
> > +        goto fail;
> > +    }
> > +
> > +    if (old_size) {
> > +        qcow2_free_clusters(bs, old_offset, old_size, QCOW2_DISCARD_ALWAYS);
> > +    }
> > +
> > +    return 0;
> > +
> > +fail:
> > +    if (new_offset > 0) {
> > +        qcow2_free_clusters(bs, new_offset, new_size, QCOW2_DISCARD_ALWAYS);
> > +        s->bitmap_directory_offset = old_offset;
> > +        s->bitmap_directory_size = old_size;
> > +        s->nb_bitmaps = old_nb_bitmaps;
> > +        s->autoclear_features = old_autocl;
> Why are you restoring the autoclear features? From a quick glance I
> can't see any code path that changes this field here, and if there is
> one, it probably has a good reason to do so.

hmm.. it's an artefact from future). should be moved to later patch.



--
Best regards,
Vladimir