[Qemu-devel] [PATCH] savevm: Really verify if a drive supports snapshots

[Qemu-devel] [PATCH] savevm: Really verify if a drive supports snapshots

[Miguel Di Ciurcio Filho]

Both bdrv_can_snapshot() and bdrv_has_snapshot() does not work as advertized.

First issue: Their names implies different porpouses, but they do the same thing
and have exactly the same code. Maybe copied and pasted and forgotten?
bdrv_has_snapshot() is called in various places for actually checking if there
is snapshots or not.

Second issue: the way bdrv_can_snapshot() verifies if a block driver supports or
not snapshots does not catch all cases. E.g.: a raw image.

So when do_savevm() is called, first thing it does is to set a global
BlockDriverState to save the VM memory state calling get_bs_snapshots().

static BlockDriverState *get_bs_snapshots(void)
{
    BlockDriverState *bs;
    DriveInfo *dinfo;

    if (bs_snapshots)
        return bs_snapshots;
    QTAILQ_FOREACH(dinfo, &drives, next) {
        bs = dinfo->bdrv;
        if (bdrv_can_snapshot(bs))
            goto ok;
    }
    return NULL;
 ok:
    bs_snapshots = bs;
    return bs;
}

bdrv_can_snapshot() may return a BlockDriverState that does not support
snapshots and do_savevm() goes on.

Later on in do_savevm(), we find:

    QTAILQ_FOREACH(dinfo, &drives, next) {
        bs1 = dinfo->bdrv;
        if (bdrv_has_snapshot(bs1)) {
            /* Write VM state size only to the image that contains the state */
            sn->vm_state_size = (bs == bs1 ? vm_state_size : 0);
            ret = bdrv_snapshot_create(bs1, sn);
            if (ret < 0) {
                monitor_printf(mon, "Error while creating snapshot on '%s'\n",
                               bdrv_get_device_name(bs1));
            }
        }
    }

bdrv_has_snapshot(bs1) is not checking if the device does support or has
snapshots as explained above. Only in bdrv_snapshot_create() the device is
actually checked for snapshot support.

So, in cases where the first device supports snapshots, and the second does not,
the snapshot on the first will happen anyways. I believe this is not a good
behavior. It should be an all or nothing process.

This patch addresses these issues by making bdrv_can_snapshot() and
bdrv_has_snapshot() actually do what they must do and enforces better tests to
avoid errors in the middle of do_savevm().

The functions were moved from savevm.c to block.c. It makes more sense to me.

The bdrv_has_snapshot() is not beaultiful, but it does the job. I think having
this function avaible in the BlockDriver would be the best option.

The loadvm_state() function was updated too to enforce that when loading a VM at
least all writable devices must support snapshots too.

Signed-off-by: Miguel Di Ciurcio Filho <miguel.filho@gmail.com>
---
 block.c  |   47 ++++++++++++++++++++++++++++++++++++++++++-----
 block.h  |    2 ++
 savevm.c |   48 +++++++++++++++++++++++++-----------------------
 3 files changed, 69 insertions(+), 28 deletions(-)

diff --git a/block.c b/block.c
index cd70730..7eddc15 100644
--- a/block.c
+++ b/block.c
@@ -1720,15 +1720,52 @@ void bdrv_debug_event(BlockDriverState *bs, BlkDebugEvent event)
 /**************************************************************/
 /* handling of snapshots */
 
-int bdrv_snapshot_create(BlockDriverState *bs,
-                         QEMUSnapshotInfo *sn_info)
+int bdrv_can_snapshot(BlockDriverState *bs)
 {
     BlockDriver *drv = bs->drv;
-    if (!drv)
+    if (!drv) {
+        return -ENOMEDIUM;
+    }
+
+    if (!drv->bdrv_snapshot_create || bdrv_is_removable(bs) ||
+        bdrv_is_read_only(bs)) {
+        return -ENOTSUP;
+    }
+
+    return 1;
+}
+
+int bdrv_has_snapshot(BlockDriverState *bs)
+{
+    int ret;
+    QEMUSnapshotInfo *sn_tab;
+    BlockDriver *drv = bs->drv;
+    if (!drv) {
         return -ENOMEDIUM;
-    if (!drv->bdrv_snapshot_create)
+    }
+
+    if (!drv->bdrv_snapshot_list) {
         return -ENOTSUP;
-    return drv->bdrv_snapshot_create(bs, sn_info);
+    }
+
+    ret = drv->bdrv_snapshot_list(bs, &sn_tab);
+
+    if (sn_tab) {
+        qemu_free(sn_tab);
+    }
+
+    return ret;
+}
+
+int bdrv_snapshot_create(BlockDriverState *bs,
+                         QEMUSnapshotInfo *sn_info)
+{
+    BlockDriver *drv = bs->drv;
+    if (bdrv_can_snapshot(bs) > 0) {
+        return drv->bdrv_snapshot_create(bs, sn_info);
+    }
+
+    return -1;
 }
 
 int bdrv_snapshot_goto(BlockDriverState *bs,
diff --git a/block.h b/block.h
index 24efeb6..c536f1c 100644
--- a/block.h
+++ b/block.h
@@ -173,6 +173,8 @@ int bdrv_get_info(BlockDriverState *bs, BlockDriverInfo *bdi);
 const char *bdrv_get_encrypted_filename(BlockDriverState *bs);
 void bdrv_get_backing_filename(BlockDriverState *bs,
                                char *filename, int filename_size);
+int bdrv_can_snapshot(BlockDriverState *bs);
+int bdrv_has_snapshot(BlockDriverState *bs);
 int bdrv_snapshot_create(BlockDriverState *bs,
                          QEMUSnapshotInfo *sn_info);
 int bdrv_snapshot_goto(BlockDriverState *bs,
diff --git a/savevm.c b/savevm.c
index dc20390..9bc232f 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1574,22 +1574,6 @@ out:
     return ret;
 }
 
-/* device can contain snapshots */
-static int bdrv_can_snapshot(BlockDriverState *bs)
-{
-    return (bs &&
-            !bdrv_is_removable(bs) &&
-            !bdrv_is_read_only(bs));
-}
-
-/* device must be snapshots in order to have a reliable snapshot */
-static int bdrv_has_snapshot(BlockDriverState *bs)
-{
-    return (bs &&
-            !bdrv_is_removable(bs) &&
-            !bdrv_is_read_only(bs));
-}
-
 static BlockDriverState *get_bs_snapshots(void)
 {
     BlockDriverState *bs;
@@ -1599,7 +1583,7 @@ static BlockDriverState *get_bs_snapshots(void)
         return bs_snapshots;
     QTAILQ_FOREACH(dinfo, &drives, next) {
         bs = dinfo->bdrv;
-        if (bdrv_can_snapshot(bs))
+        if (bdrv_can_snapshot(bs) > 0)
             goto ok;
     }
     return NULL;
@@ -1642,7 +1626,7 @@ static int del_existing_snapshots(Monitor *mon, const char *name)
 
     QTAILQ_FOREACH(dinfo, &drives, next) {
         bs = dinfo->bdrv;
-        if (bdrv_can_snapshot(bs) &&
+        if ((bdrv_can_snapshot(bs) > 0) &&
             bdrv_snapshot_find(bs, snapshot, name) >= 0)
         {
             ret = bdrv_snapshot_delete(bs, name);
@@ -1674,12 +1658,30 @@ void do_savevm(Monitor *mon, const QDict *qdict)
 #endif
     const char *name = qdict_get_try_str(qdict, "name");
 
+    /* Verify if there is have a device that doesn't support snapshots and is writable*/
+    ret = 0;
+    QTAILQ_FOREACH(dinfo, &drives, next) {
+        bs = dinfo->bdrv;
+
+        if (bdrv_is_removable(bs) || bdrv_is_read_only(bs)) {
+            continue;
+        }
+
+        if ((ret = bdrv_can_snapshot(bs)) < 0) {
+            monitor_printf(mon, "Device '%s' is writable but does not support snapshots.\n",
+                               bdrv_get_device_name(bs));
+        }
+    }
+
+    if (ret < 0) {
+        goto the_end;
+    }
+
     bs = get_bs_snapshots();
     if (!bs) {
         monitor_printf(mon, "No block device can accept snapshots\n");
         return;
     }
-
     /* ??? Should this occur after vm_stop?  */
     qemu_aio_flush();
 
@@ -1732,7 +1734,7 @@ void do_savevm(Monitor *mon, const QDict *qdict)
 
     QTAILQ_FOREACH(dinfo, &drives, next) {
         bs1 = dinfo->bdrv;
-        if (bdrv_has_snapshot(bs1)) {
+        if (bdrv_can_snapshot(bs1) > 0) {
             /* Write VM state size only to the image that contains the state */
             sn->vm_state_size = (bs == bs1 ? vm_state_size : 0);
             ret = bdrv_snapshot_create(bs1, sn);
@@ -1767,7 +1769,7 @@ int load_vmstate(const char *name)
 
     QTAILQ_FOREACH(dinfo, &drives, next) {
         bs1 = dinfo->bdrv;
-        if (bdrv_has_snapshot(bs1)) {
+        if (bdrv_has_snapshot(bs1) > 0) {
             ret = bdrv_snapshot_goto(bs1, name);
             if (ret < 0) {
                 switch(ret) {
@@ -1829,7 +1831,7 @@ void do_delvm(Monitor *mon, const QDict *qdict)
 
     QTAILQ_FOREACH(dinfo, &drives, next) {
         bs1 = dinfo->bdrv;
-        if (bdrv_has_snapshot(bs1)) {
+        if (bdrv_has_snapshot(bs1) > 0) {
             ret = bdrv_snapshot_delete(bs1, name);
             if (ret < 0) {
                 if (ret == -ENOTSUP)
@@ -1860,7 +1862,7 @@ void do_info_snapshots(Monitor *mon)
     monitor_printf(mon, "Snapshot devices:");
     QTAILQ_FOREACH(dinfo, &drives, next) {
         bs1 = dinfo->bdrv;
-        if (bdrv_has_snapshot(bs1)) {
+        if (bdrv_can_snapshot(bs1) > 0) {
             if (bs == bs1)
                 monitor_printf(mon, " %s", bdrv_get_device_name(bs1));
         }
-- 
1.7.1

[Qemu-devel] Re: [PATCH] savevm: Really verify if a drive supports snapshots

[Kevin Wolf]

Am 28.05.2010 20:18, schrieb Miguel Di Ciurcio Filho:
> Both bdrv_can_snapshot() and bdrv_has_snapshot() does not work as advertized.
> 
> First issue: Their names implies different porpouses, but they do the same thing
> and have exactly the same code. Maybe copied and pasted and forgotten?
> bdrv_has_snapshot() is called in various places for actually checking if there
> is snapshots or not.
> 
> Second issue: the way bdrv_can_snapshot() verifies if a block driver supports or
> not snapshots does not catch all cases. E.g.: a raw image.
> 
> So when do_savevm() is called, first thing it does is to set a global
> BlockDriverState to save the VM memory state calling get_bs_snapshots().
> 
> static BlockDriverState *get_bs_snapshots(void)
> {
>     BlockDriverState *bs;
>     DriveInfo *dinfo;
> 
>     if (bs_snapshots)
>         return bs_snapshots;
>     QTAILQ_FOREACH(dinfo, &drives, next) {
>         bs = dinfo->bdrv;
>         if (bdrv_can_snapshot(bs))
>             goto ok;
>     }
>     return NULL;
>  ok:
>     bs_snapshots = bs;
>     return bs;
> }
> 
> bdrv_can_snapshot() may return a BlockDriverState that does not support
> snapshots and do_savevm() goes on.
> 
> Later on in do_savevm(), we find:
> 
>     QTAILQ_FOREACH(dinfo, &drives, next) {
>         bs1 = dinfo->bdrv;
>         if (bdrv_has_snapshot(bs1)) {
>             /* Write VM state size only to the image that contains the state */
>             sn->vm_state_size = (bs == bs1 ? vm_state_size : 0);
>             ret = bdrv_snapshot_create(bs1, sn);
>             if (ret < 0) {
>                 monitor_printf(mon, "Error while creating snapshot on '%s'\n",
>                                bdrv_get_device_name(bs1));
>             }
>         }
>     }
> 
> bdrv_has_snapshot(bs1) is not checking if the device does support or has
> snapshots as explained above. Only in bdrv_snapshot_create() the device is
> actually checked for snapshot support.
> 
> So, in cases where the first device supports snapshots, and the second does not,
> the snapshot on the first will happen anyways. I believe this is not a good
> behavior. It should be an all or nothing process.
> 
> This patch addresses these issues by making bdrv_can_snapshot() and
> bdrv_has_snapshot() actually do what they must do and enforces better tests to
> avoid errors in the middle of do_savevm().
> 
> The functions were moved from savevm.c to block.c. It makes more sense to me.
> 
> The bdrv_has_snapshot() is not beaultiful, but it does the job. I think having
> this function avaible in the BlockDriver would be the best option.
> 
> The loadvm_state() function was updated too to enforce that when loading a VM at
> least all writable devices must support snapshots too.
> 
> Signed-off-by: Miguel Di Ciurcio Filho <miguel.filho@gmail.com>

Markus, I think this implements mostly what we discussed the other day.
Not sure if you already have a patch for doing this - if so, maybe could
compare the patches and give it a review this way?

I seem to remember that we came to the conclusion that
bdrv_has_snapshot() isn't needed at all and should be dropped. Any user
should be using bdrv_can_snapshot() instead as this is what they really
want.

> ---
>  block.c  |   47 ++++++++++++++++++++++++++++++++++++++++++-----
>  block.h  |    2 ++
>  savevm.c |   48 +++++++++++++++++++++++++-----------------------
>  3 files changed, 69 insertions(+), 28 deletions(-)
> 
> diff --git a/block.c b/block.c
> index cd70730..7eddc15 100644
> --- a/block.c
> +++ b/block.c
> @@ -1720,15 +1720,52 @@ void bdrv_debug_event(BlockDriverState *bs, BlkDebugEvent event)
>  /**************************************************************/
>  /* handling of snapshots */
>  
> -int bdrv_snapshot_create(BlockDriverState *bs,
> -                         QEMUSnapshotInfo *sn_info)
> +int bdrv_can_snapshot(BlockDriverState *bs)
>  {
>      BlockDriver *drv = bs->drv;
> -    if (!drv)
> +    if (!drv) {
> +        return -ENOMEDIUM;
> +    }
> +
> +    if (!drv->bdrv_snapshot_create || bdrv_is_removable(bs) ||
> +        bdrv_is_read_only(bs)) {
> +        return -ENOTSUP;
> +    }
> +
> +    return 1;
> +}

Returning either 1 or -errno is a strange interface. I'm not sure which
of 1/0 or 0/-errno is better in this case, but I'd suggest to take one
of these.

> +int bdrv_has_snapshot(BlockDriverState *bs)
> +{
> +    int ret;
> +    QEMUSnapshotInfo *sn_tab;
> +    BlockDriver *drv = bs->drv;
> +    if (!drv) {
>          return -ENOMEDIUM;
> -    if (!drv->bdrv_snapshot_create)
> +    }
> +
> +    if (!drv->bdrv_snapshot_list) {
>          return -ENOTSUP;
> -    return drv->bdrv_snapshot_create(bs, sn_info);
> +    }
> +
> +    ret = drv->bdrv_snapshot_list(bs, &sn_tab);
> +
> +    if (sn_tab) {
> +        qemu_free(sn_tab);
> +    }
> +
> +    return ret;
> +}
> +
> +int bdrv_snapshot_create(BlockDriverState *bs,
> +                         QEMUSnapshotInfo *sn_info)
> +{
> +    BlockDriver *drv = bs->drv;
> +    if (bdrv_can_snapshot(bs) > 0) {
> +        return drv->bdrv_snapshot_create(bs, sn_info);
> +    }
> +
> +    return -1;
>  }
>  
>  int bdrv_snapshot_goto(BlockDriverState *bs,
> diff --git a/block.h b/block.h
> index 24efeb6..c536f1c 100644
> --- a/block.h
> +++ b/block.h
> @@ -173,6 +173,8 @@ int bdrv_get_info(BlockDriverState *bs, BlockDriverInfo *bdi);
>  const char *bdrv_get_encrypted_filename(BlockDriverState *bs);
>  void bdrv_get_backing_filename(BlockDriverState *bs,
>                                 char *filename, int filename_size);
> +int bdrv_can_snapshot(BlockDriverState *bs);
> +int bdrv_has_snapshot(BlockDriverState *bs);
>  int bdrv_snapshot_create(BlockDriverState *bs,
>                           QEMUSnapshotInfo *sn_info);
>  int bdrv_snapshot_goto(BlockDriverState *bs,
> diff --git a/savevm.c b/savevm.c
> index dc20390..9bc232f 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -1574,22 +1574,6 @@ out:
>      return ret;
>  }
>  
> -/* device can contain snapshots */
> -static int bdrv_can_snapshot(BlockDriverState *bs)
> -{
> -    return (bs &&
> -            !bdrv_is_removable(bs) &&
> -            !bdrv_is_read_only(bs));
> -}
> -
> -/* device must be snapshots in order to have a reliable snapshot */
> -static int bdrv_has_snapshot(BlockDriverState *bs)
> -{
> -    return (bs &&
> -            !bdrv_is_removable(bs) &&
> -            !bdrv_is_read_only(bs));
> -}
> -
>  static BlockDriverState *get_bs_snapshots(void)
>  {
>      BlockDriverState *bs;
> @@ -1599,7 +1583,7 @@ static BlockDriverState *get_bs_snapshots(void)
>          return bs_snapshots;
>      QTAILQ_FOREACH(dinfo, &drives, next) {
>          bs = dinfo->bdrv;
> -        if (bdrv_can_snapshot(bs))
> +        if (bdrv_can_snapshot(bs) > 0)
>              goto ok;
>      }
>      return NULL;
> @@ -1642,7 +1626,7 @@ static int del_existing_snapshots(Monitor *mon, const char *name)
>  
>      QTAILQ_FOREACH(dinfo, &drives, next) {
>          bs = dinfo->bdrv;
> -        if (bdrv_can_snapshot(bs) &&
> +        if ((bdrv_can_snapshot(bs) > 0) &&
>              bdrv_snapshot_find(bs, snapshot, name) >= 0)
>          {
>              ret = bdrv_snapshot_delete(bs, name);
> @@ -1674,12 +1658,30 @@ void do_savevm(Monitor *mon, const QDict *qdict)
>  #endif
>      const char *name = qdict_get_try_str(qdict, "name");
>  
> +    /* Verify if there is have a device that doesn't support snapshots and is writable*/
> +    ret = 0;
> +    QTAILQ_FOREACH(dinfo, &drives, next) {
> +        bs = dinfo->bdrv;
> +
> +        if (bdrv_is_removable(bs) || bdrv_is_read_only(bs)) {
> +            continue;
> +        }
> +
> +        if ((ret = bdrv_can_snapshot(bs)) < 0) {

This may overwrite the error of the previous loop iteration with a
success return value. It's probably not what you want.

Other than that it looks good to me.

Kevin

[Qemu-devel] Re: [PATCH] savevm: Really verify if a drive supports snapshots

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 28.05.2010 20:18, schrieb Miguel Di Ciurcio Filho:
>> Both bdrv_can_snapshot() and bdrv_has_snapshot() does not work as advertized.
>> 
>> First issue: Their names implies different porpouses, but they do the same thing
>> and have exactly the same code. Maybe copied and pasted and forgotten?
>> bdrv_has_snapshot() is called in various places for actually checking if there
>> is snapshots or not.
>> 
>> Second issue: the way bdrv_can_snapshot() verifies if a block driver supports or
>> not snapshots does not catch all cases. E.g.: a raw image.
>> 
>> So when do_savevm() is called, first thing it does is to set a global
>> BlockDriverState to save the VM memory state calling get_bs_snapshots().
>> 
>> static BlockDriverState *get_bs_snapshots(void)
>> {
>>     BlockDriverState *bs;
>>     DriveInfo *dinfo;
>> 
>>     if (bs_snapshots)
>>         return bs_snapshots;
>>     QTAILQ_FOREACH(dinfo, &drives, next) {
>>         bs = dinfo->bdrv;
>>         if (bdrv_can_snapshot(bs))
>>             goto ok;
>>     }
>>     return NULL;
>>  ok:
>>     bs_snapshots = bs;
>>     return bs;
>> }
>> 
>> bdrv_can_snapshot() may return a BlockDriverState that does not support
>> snapshots and do_savevm() goes on.
>> 
>> Later on in do_savevm(), we find:
>> 
>>     QTAILQ_FOREACH(dinfo, &drives, next) {
>>         bs1 = dinfo->bdrv;
>>         if (bdrv_has_snapshot(bs1)) {
>>             /* Write VM state size only to the image that contains the state */
>>             sn->vm_state_size = (bs == bs1 ? vm_state_size : 0);
>>             ret = bdrv_snapshot_create(bs1, sn);
>>             if (ret < 0) {
>>                 monitor_printf(mon, "Error while creating snapshot on '%s'\n",
>>                                bdrv_get_device_name(bs1));
>>             }
>>         }
>>     }
>> 
>> bdrv_has_snapshot(bs1) is not checking if the device does support or has
>> snapshots as explained above. Only in bdrv_snapshot_create() the device is
>> actually checked for snapshot support.
>> 
>> So, in cases where the first device supports snapshots, and the second does not,
>> the snapshot on the first will happen anyways. I believe this is not a good
>> behavior. It should be an all or nothing process.
>> 
>> This patch addresses these issues by making bdrv_can_snapshot() and
>> bdrv_has_snapshot() actually do what they must do and enforces better tests to
>> avoid errors in the middle of do_savevm().
>> 
>> The functions were moved from savevm.c to block.c. It makes more sense to me.
>> 
>> The bdrv_has_snapshot() is not beaultiful, but it does the job. I think having
>> this function avaible in the BlockDriver would be the best option.
>> 
>> The loadvm_state() function was updated too to enforce that when loading a VM at
>> least all writable devices must support snapshots too.
>> 
>> Signed-off-by: Miguel Di Ciurcio Filho <miguel.filho@gmail.com>
>
> Markus, I think this implements mostly what we discussed the other day.
> Not sure if you already have a patch for doing this - if so, maybe could
> compare the patches and give it a review this way?

Glad I don't, because this one looks pretty good.

> I seem to remember that we came to the conclusion that
> bdrv_has_snapshot() isn't needed at all and should be dropped. Any user
> should be using bdrv_can_snapshot() instead as this is what they really
> want.

Our reasoning adapted to the patched code goes like this.  The remaining
uses of bdrv_has_snapshot() are of the form:

    if (bdrv_has_snapshot(bs)
       some_snapshot_op()

where some_snapshot_op() fails when there are no snapshots, and the code
can recognize that failure as "sorry, no snapshots" (that's what it does
before your patch, when bdrv_has_snapshot() is really no more than a
necessary, but not sufficient condition for "has snapshots").

Therefore, we don't have to check for actual existence of snapshots with
bdrv_has_snapshot().  bdrv_can_snapshot() suffices.

In general, I consider

    if (predict_whether_foo_will_work)
        if (foo() < 0)
            // foo failed
    else
        // foo would have failed (we think)

an anti-pattern.  Just make foo() safe to try, and check its status:

    if (foo() < 0)
        // foo failed

Except when foo() is too expensive to try, or can't be made safe.

>> ---
>>  block.c  |   47 ++++++++++++++++++++++++++++++++++++++++++-----
>>  block.h  |    2 ++
>>  savevm.c |   48 +++++++++++++++++++++++++-----------------------
>>  3 files changed, 69 insertions(+), 28 deletions(-)
>> 
>> diff --git a/block.c b/block.c
>> index cd70730..7eddc15 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -1720,15 +1720,52 @@ void bdrv_debug_event(BlockDriverState *bs, BlkDebugEvent event)
>>  /**************************************************************/
>>  /* handling of snapshots */
>>  
>> -int bdrv_snapshot_create(BlockDriverState *bs,
>> -                         QEMUSnapshotInfo *sn_info)
>> +int bdrv_can_snapshot(BlockDriverState *bs)
>>  {
>>      BlockDriver *drv = bs->drv;
>> -    if (!drv)
>> +    if (!drv) {
>> +        return -ENOMEDIUM;
>> +    }
>> +
>> +    if (!drv->bdrv_snapshot_create || bdrv_is_removable(bs) ||
>> +        bdrv_is_read_only(bs)) {
>> +        return -ENOTSUP;
>> +    }
>> +
>> +    return 1;
>> +}
>
> Returning either 1 or -errno is a strange interface. I'm not sure which

Agree.

> of 1/0 or 0/-errno is better in this case, but I'd suggest to take one
> of these.

Does any existing caller care for the error codes?  If not, just go with
1/0.

[...]

[Qemu-devel] Re: [PATCH] savevm: Really verify if a drive supports snapshots

[Miguel Di Ciurcio Filho]

On Sat, May 29, 2010 at 3:06 AM, Markus Armbruster <armbru@redhat.com> wrote:
>
>> I seem to remember that we came to the conclusion that
>> bdrv_has_snapshot() isn't needed at all and should be dropped. Any user
>> should be using bdrv_can_snapshot() instead as this is what they really
>> want.
>
> Our reasoning adapted to the patched code goes like this.  The remaining
> uses of bdrv_has_snapshot() are of the form:
>
>    if (bdrv_has_snapshot(bs)
>       some_snapshot_op()
>
> where some_snapshot_op() fails when there are no snapshots, and the code
> can recognize that failure as "sorry, no snapshots" (that's what it does
> before your patch, when bdrv_has_snapshot() is really no more than a
> necessary, but not sufficient condition for "has snapshots").
>
> Therefore, we don't have to check for actual existence of snapshots with
> bdrv_has_snapshot().  bdrv_can_snapshot() suffices.

I had this feeling too, but I was conservative and kept
bdrv_has_snapshot(). I will remove it and replace by
bdrv_can_snapshot() where appropriate.

>>> +int bdrv_can_snapshot(BlockDriverState *bs)
>>>  {
>>>      BlockDriver *drv = bs->drv;
>>> -    if (!drv)
>>> +    if (!drv) {
>>> +        return -ENOMEDIUM;
>>> +    }
>>> +
>>> +    if (!drv->bdrv_snapshot_create || bdrv_is_removable(bs) ||
>>> +        bdrv_is_read_only(bs)) {
>>> +        return -ENOTSUP;
>>> +    }
>>> +
>>> +    return 1;
>>> +}
>>
>> Returning either 1 or -errno is a strange interface. I'm not sure which
>
> Agree.
>
>> of 1/0 or 0/-errno is better in this case, but I'd suggest to take one
>> of these.
>
> Does any existing caller care for the error codes?  If not, just go with
> 1/0.

When bdrv_can_snapshot() is called the specific reason for not
supporting snapshots does not matter. So, 1/0 is better. I will fix it
and resend.

Regards,

Miguel