[Top][All Lists]

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

Re: [PATCH 10/14] block/qcow2-bitmap: improve qcow2_load_dirty_bitmaps()

From: Vladimir Sementsov-Ogievskiy
Subject: Re: [PATCH 10/14] block/qcow2-bitmap: improve qcow2_load_dirty_bitmaps() interface
Date: Thu, 17 Sep 2020 21:58:23 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.2.2

17.09.2020 19:35, Alberto Garcia wrote:
On Wed 09 Sep 2020 08:59:26 PM CEST, Vladimir Sementsov-Ogievskiy 
<vsementsov@virtuozzo.com> wrote:
-/* qcow2_load_dirty_bitmaps()
- * Return value is a hint for caller: true means that the Qcow2 header was
- * updated. (false doesn't mean that the header should be updated by the
- * caller, it just means that updating was not needed or the image cannot be
- * written to).
- * On failure the function returns false.
- */
-bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
+/* Return true on success, false on failure. */
+bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, bool *header_updated,
+                              Error **errp)

I think that the documentation should clarify under what conditions
'header_updated' is modified.

      if (s->nb_bitmaps == 0) {
          /* No bitmaps - nothing to do */
-        return false;
+        return true;

Here is it not for example (should it be set to false?).

Ha, I think, it just shows that patch is wrong :) We should set header_updated 
at least on every success path. Or better always (if it is non-NULL of course). 
Thanks for careful review!

-    if (bm_list == NULL) {
+    if (!bm_list) {
          return false;

This looks like a cosmetic change unrelated to the rest of the patch.


Best regards,

reply via email to

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