[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-block] [PATCH 07/22] qcow2-bitmap: introduce auto-loading bitm

From: Vladimir Sementsov-Ogievskiy
Subject: Re: [Qemu-block] [PATCH 07/22] qcow2-bitmap: introduce auto-loading bitmaps
Date: Sat, 15 Oct 2016 20:22:43 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.6.0

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

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


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,

reply via email to

[Prev in Thread] Current Thread [Next in Thread]