From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:56293)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <vsementsov@parallels.com>) id 1XmlHT-0002cp-PH
	for qemu-devel@nongnu.org; Fri, 07 Nov 2014 10:17:01 -0500
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <vsementsov@parallels.com>) id 1XmlHN-0001ib-JV
	for qemu-devel@nongnu.org; Fri, 07 Nov 2014 10:16:55 -0500
Received: from relay.parallels.com ([195.214.232.42]:39586)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <vsementsov@parallels.com>) id 1XmlHN-0001iH-7L
	for qemu-devel@nongnu.org; Fri, 07 Nov 2014 10:16:49 -0500
Message-ID: <545CE25B.9030208@parallels.com>
Date: Fri, 7 Nov 2014 18:16:43 +0300
From: Vladimir Sementsov-Ogievskiy <vsementsov@parallels.com>
MIME-Version: 1.0
References: 1414639364-4499-6-git-send-email-famz@redhat.com
Content-Type: multipart/alternative;
	boundary="------------050900020302080202080505"
Subject: Re: [Qemu-devel] [PATCH v6 05/10] block: Add bdrv_copy_dirty_bitmap
 and	bdrv_reset_dirty_bitmap
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: famz@redhat.com
Cc: Kevin Wolf <kwolf@redhat.com>, Benoit Canet <benoit@irqsave.net>, John Snow <jsnow@redhat.com>, Markus Armbruster <armbru@redhat.com>, qemu-devel@nongnu.org, Max Reitz <mreitz@redhat.com>, Stefan Hajnoczi <stefanha@redhat.com>, Jd <jd_jedi@convirture.com>, Paolo Bonzini <pbonzini@redhat.com>, Luiz Capitulino <lcapitulino@redhat.com>

--------------050900020302080202080505
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit

from [PATCH v6 02/10]
> +void qmp_block_dirty_bitmap_remove(const char *device, const char *name,
> +                                   Error **errp)
> +{
> +    BlockDriverState *bs;
> +    BdrvDirtyBitmap *bitmap;
> +
> +    bs = bdrv_find(device);
> +    if (!bs) {
> +        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
> +        return;
> +    }
> +
> +    if (!name || name[0] == '\0') {
> +        error_setg(errp, "Bitmap name cannot be empty");
> +        return;
> +    }
> +    bitmap = bdrv_find_dirty_bitmap(bs, name);
> +    if (!bitmap) {
> +        error_setg(errp, "Dirty bitmap not found: %s", name);
> +        return;
> +    }
> +
> +    bdrv_dirty_bitmap_make_anon(bs, bitmap);
> +    bdrv_release_dirty_bitmap(bs, bitmap);
> +}

from [PATCH v6 05/10]:
> +void qmp_block_dirty_bitmap_enable(const char *device, const char *name,
> +                                   Error **errp)
> +{
> +    BlockDriverState *bs;
> +    BdrvDirtyBitmap *bitmap;
> +
> +    bs = bdrv_find(device);
> +    if (!bs) {
> +        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
> +        return;
> +    }
> +
> +    bitmap = bdrv_find_dirty_bitmap(bs, name);
> +    if (!bitmap) {
> +        error_setg(errp, "Dirty bitmap not found: %s", name);
> +        return;
> +    }
> +
> +    bdrv_enable_dirty_bitmap(bs, bitmap);
> +}
> +
> +void qmp_block_dirty_bitmap_disable(const char *device, const char *name,
> +                                    Error **errp)
> +{
> +    BlockDriverState *bs;
> +    BdrvDirtyBitmap *bitmap;
> +
> +    bs = bdrv_find(device);
> +    if (!bs) {
> +        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
> +        return;
> +    }
> +
> +    bitmap = bdrv_find_dirty_bitmap(bs, name);
> +    if (!bitmap) {
> +        error_setg(errp, "Dirty bitmap not found: %s", name);
> +        return;
> +    }
> +
> +    bdrv_disable_dirty_bitmap(bs, bitmap);
> +}
> +

there is one inconsistence:

you have check
> +    if (!name || name[0] == '\0') {
> +        error_setg(errp, "Bitmap name cannot be empty");
> +        return;
> +    }
when accessing bitmap in qmp_block_dirty_bitmap_remove, but not in 
qmp_block_dirty_bitmap_{enable,disable}.

Also, I think it'll be better to put similar part of these three 
functions into one separate function to avoid duplicates, like

static BdrvDirtyBitmap *bitmap find_dirty_bitmap(const char *device, const char *name,
                                    Error **errp)
{
     BlockDriverState *bs;
     BdrvDirtyBitmap *bitmap;

     // most simple error condition earlier
     if (!name || name[0] == '\0') {
         error_setg(errp, "Bitmap name cannot be empty");
         return NULL;
     }

     bs = bdrv_find(device);
     if (!bs) {
         error_set(errp, QERR_DEVICE_NOT_FOUND, device);
         return NULL;
     }

     bitmap = bdrv_find_dirty_bitmap(bs, name);
     if (!bitmap) {
         error_setg(errp, "Dirty bitmap not found: %s", name);
         return NULL;
     }
     
     return bitmap;
}


-- 
Best regards,
Vladimir


--------------050900020302080202080505
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: 7bit

<html>
  <head>

    <meta http-equiv="content-type" content="text/html; charset=UTF-8">
  </head>
  <body text="#000000" bgcolor="#FFFFFF">
    from [PATCH v6 02/10] <br>
    <blockquote type="cite">
      <meta http-equiv="content-type" content="text/html; charset=UTF-8">
      <pre style="color: rgb(0, 0, 0); font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;">+void qmp_block_dirty_bitmap_remove(const char *device, const char *name,
+                                   Error **errp)
+{
+    BlockDriverState *bs;
+    BdrvDirtyBitmap *bitmap;
+
+    bs = bdrv_find(device);
+    if (!bs) {
+        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
+        return;
+    }
+
+    if (!name || name[0] == '\0') {
+        error_setg(errp, "Bitmap name cannot be empty");
+        return;
+    }
+    bitmap = bdrv_find_dirty_bitmap(bs, name);
+    if (!bitmap) {
+        error_setg(errp, "Dirty bitmap not found: %s", name);
+        return;
+    }
+
+    bdrv_dirty_bitmap_make_anon(bs, bitmap);
+    bdrv_release_dirty_bitmap(bs, bitmap);
+}</pre>
    </blockquote>
    <br>
    from [PATCH v6 05/10]:<br>
    <blockquote type="cite">
      <meta http-equiv="content-type" content="text/html; charset=UTF-8">
      <pre style="color: rgb(0, 0, 0); font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;">+void qmp_block_dirty_bitmap_enable(const char *device, const char *name,
+                                   Error **errp)
+{
+    BlockDriverState *bs;
+    BdrvDirtyBitmap *bitmap;
+
+    bs = bdrv_find(device);
+    if (!bs) {
+        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
+        return;
+    }
+
+    bitmap = bdrv_find_dirty_bitmap(bs, name);
+    if (!bitmap) {
+        error_setg(errp, "Dirty bitmap not found: %s", name);
+        return;
+    }
+
+    bdrv_enable_dirty_bitmap(bs, bitmap);
+}
+
+void qmp_block_dirty_bitmap_disable(const char *device, const char *name,
+                                    Error **errp)
+{
+    BlockDriverState *bs;
+    BdrvDirtyBitmap *bitmap;
+
+    bs = bdrv_find(device);
+    if (!bs) {
+        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
+        return;
+    }
+
+    bitmap = bdrv_find_dirty_bitmap(bs, name);
+    if (!bitmap) {
+        error_setg(errp, "Dirty bitmap not found: %s", name);
+        return;
+    }
+
+    bdrv_disable_dirty_bitmap(bs, bitmap);
+}
+</pre>
    </blockquote>
    <br>
    there is one inconsistence:<br>
    <br>
    you have check<br>
    <blockquote type="cite">
      <pre style="color: rgb(0, 0, 0); font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;">+    if (!name || name[0] == '\0') {
+        error_setg(errp, "Bitmap name cannot be empty");
+        return;
+    }</pre>
    </blockquote>
    when accessing bitmap in qmp_block_dirty_bitmap_remove, but not in
    qmp_block_dirty_bitmap_{enable,disable}.<br>
    <br>
    Also, I think it'll be better to put similar part of these three
    functions into one separate function to avoid duplicates, like<br>
    <br>
    <pre style="color: rgb(0, 0, 0); font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;">static BdrvDirtyBitmap *bitmap find_dirty_bitmap(const char *device, const char *name,
                                   Error **errp)
{
    BlockDriverState *bs;
    BdrvDirtyBitmap *bitmap;

    // most simple error condition earlier
    if (!name || name[0] == '\0') {
        error_setg(errp, "Bitmap name cannot be empty");
        return NULL;
    }

    bs = bdrv_find(device);
    if (!bs) {
        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
        return NULL;
    }

    bitmap = bdrv_find_dirty_bitmap(bs, name);
    if (!bitmap) {
        error_setg(errp, "Dirty bitmap not found: %s", name);
        return NULL;
    }
    
    return bitmap;
}</pre>
    <br>
    <pre class="moz-signature" cols="72">-- 
Best regards,
Vladimir
</pre>
  </body>
</html>

--------------050900020302080202080505--