[Qemu-devel] [PATCH]: set up rbd snapshot handling

[Qemu-devel] [PATCH]: set up rbd snapshot handling

[Gregory Farnum]

rbd: wire up snapshot removal and rollback functionality

Signed-off-by: Greg Farnum <gregory.farnum@dreamhost.com>
Reviewed-by: Josh Durgin <josh.durgin@dreamhost.com>
---
 block/rbd.c |   32 ++++++++++++++++++++++++++++++++
 1 files changed, 32 insertions(+), 0 deletions(-)

diff --git a/block/rbd.c b/block/rbd.c
index 7a2384c..f52c1ca 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -787,6 +787,36 @@ static int qemu_rbd_snap_create(BlockDriverState *bs,
     return 0;
 }

+static int qemu_rbd_snap_remove(BlockDriverState *bs,
+                                const char *snapshot_name)
+{
+    BDRVRBDState *s = bs->opaque;
+    int r;
+
+    r = rbd_snap_remove(s->image, snapshot_name);
+    if (r < 0) {
+        error_report("failed to remove snap: %s", strerror(-r));
+        return r;
+    }
+
+    return 0;
+}
+
+static int qemu_rbd_snap_rollback(BlockDriverState *bs,
+                                  const char *snapshot_name)
+{
+    BDRVRBDState *s = bs->opaque;
+    int r;
+
+    r = rbd_snap_rollback(s->image, snapshot_name);
+    if (r < 0) {
+        error_report("failed to rollback to snap: %s", strerror(-r));
+        return r;
+    }
+
+    return 0;
+}
+
 static int qemu_rbd_snap_list(BlockDriverState *bs,
                               QEMUSnapshotInfo **psn_tab)
 {
@@ -860,7 +890,9 @@ static BlockDriver bdrv_rbd = {
     .bdrv_co_flush_to_disk  = qemu_rbd_co_flush,

     .bdrv_snapshot_create   = qemu_rbd_snap_create,
+    .bdrv_snapshot_delete   = qemu_rbd_snap_remove,
     .bdrv_snapshot_list     = qemu_rbd_snap_list,
+    .bdrv_snapshot_goto     = qemu_rbd_snap_rollback,
 };

 static void bdrv_rbd_init(void)
-- 
1.7.2.5

Re: [Qemu-devel] [PATCH]: set up rbd snapshot handling

[Stefan Hajnoczi]

On Tue, Jan 10, 2012 at 8:01 PM, Gregory Farnum
<gregory.farnum@dreamhost.com> wrote:
> +static int qemu_rbd_snap_remove(BlockDriverState *bs,
> +                                const char *snapshot_name)
> +{
> +    BDRVRBDState *s = bs->opaque;
> +    int r;
> +
> +    r = rbd_snap_remove(s->image, snapshot_name);
> +    if (r < 0) {
> +        error_report("failed to remove snap: %s", strerror(-r));
> +        return r;

There's no need to report an error message here.  This function should
return -errno and let the caller decide how to show the error to the
user.  If you look at callers in the codebase they already print an
equivalent error message.

Stefan

Re: [Qemu-devel] [PATCH]: set up rbd snapshot handling

[Christoph Hellwig]

> +static int qemu_rbd_snap_remove(BlockDriverState *bs,
> +                                const char *snapshot_name)
> +{
> +    BDRVRBDState *s = bs->opaque;
> +    int r;
> +
> +    r = rbd_snap_remove(s->image, snapshot_name);

> +    r = rbd_snap_rollback(s->image, snapshot_name);

Have these functions been available since day 1 in librbd or should they
get a version checks like the cache flush call?

Re: [Qemu-devel] [PATCH]: set up rbd snapshot handling

[Gregory Farnum]

On Wed, Jan 11, 2012 at 1:58 AM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Tue, Jan 10, 2012 at 8:01 PM, Gregory Farnum
> <gregory.farnum@dreamhost.com> wrote:
>> +static int qemu_rbd_snap_remove(BlockDriverState *bs,
>> +                                const char *snapshot_name)
>> +{
>> +    BDRVRBDState *s = bs->opaque;
>> +    int r;
>> +
>> +    r = rbd_snap_remove(s->image, snapshot_name);
>> +    if (r < 0) {
>> +        error_report("failed to remove snap: %s", strerror(-r));
>> +        return r;
>
> There's no need to report an error message here.  This function should
> return -errno and let the caller decide how to show the error to the
> user.  If you look at callers in the codebase they already print an
> equivalent error message.
>
> Stefan

Oh yep, guess I was a little too formulaic. Resend in a moment...

On Wed, Jan 11, 2012 at 2:00 AM, Christoph Hellwig <hch@infradead.org> wrote:
>> +static int qemu_rbd_snap_remove(BlockDriverState *bs,
>> +                                const char *snapshot_name)
>> +{
>> +    BDRVRBDState *s = bs->opaque;
>> +    int r;
>> +
>> +    r = rbd_snap_remove(s->image, snapshot_name);
>
>> +    r = rbd_snap_rollback(s->image, snapshot_name);
>
> Have these functions been available since day 1 in librbd or should they
> get a version checks like the cache flush call?

Day 1. :)