[Qemu-devel] [PATCH] block: add drive_backup HMP command

[Qemu-devel] [PATCH] block: add drive_backup HMP command

[Stefan Hajnoczi]

Make "drive_backup" available on the HMP monitor:

  drive_backup [-n] [-f] device target [format]

The -n flag requests QEMU to reuse the image found in new-image-file,
instead of recreating it from scratch.

The -f flag requests QEMU to copy the whole disk, so that the result
does not need a backing file.  Note that this flag *must* currently be
passed since the other sync modes ('none' and 'top') have not been
implemented yet.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hmp-commands.hx | 20 ++++++++++++++++++++
 hmp.c           | 33 +++++++++++++++++++++++++++++++++
 hmp.h           |  1 +
 3 files changed, 54 insertions(+)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index 915b0d1..800101b 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1059,6 +1059,26 @@ using the specified target.
 ETEXI
 
     {
+        .name       = "drive_backup",
+        .args_type  = "reuse:-n,full:-f,device:B,target:s,format:s?",
+        .params     = "[-n] [-f] device target [format]",
+        .help       = "initiates a point-in-time\n\t\t\t"
+                      "copy for a device. The device's contents are\n\t\t\t"
+                      "copied to the new image file, excluding data that\n\t\t\t"
+                      "is written after the command is started.\n\t\t\t"
+                      "The -n flag requests QEMU to reuse the image found\n\t\t\t"
+                      "in new-image-file, instead of recreating it from scratch.\n\t\t\t"
+                      "The -f flag requests QEMU to copy the whole disk,\n\t\t\t"
+                      "so that the result does not need a backing file.\n\t\t\t",
+        .mhandler.cmd = hmp_drive_backup,
+    },
+STEXI
+@item drive_backup
+@findex drive_backup
+Start a point-in-time copy of a block device to a specificed target.
+ETEXI
+
+    {
         .name       = "drive_add",
         .args_type  = "pci_addr:s,opts:s",
         .params     = "[[<domain>:]<bus>:]<slot>\n"
diff --git a/hmp.c b/hmp.c
index 494a9aa..cb6a87a 100644
--- a/hmp.c
+++ b/hmp.c
@@ -889,6 +889,39 @@ void hmp_drive_mirror(Monitor *mon, const QDict *qdict)
     hmp_handle_error(mon, &errp);
 }
 
+void hmp_drive_backup(Monitor *mon, const QDict *qdict)
+{
+    const char *device = qdict_get_str(qdict, "device");
+    const char *filename = qdict_get_str(qdict, "target");
+    const char *format = qdict_get_try_str(qdict, "format");
+    int reuse = qdict_get_try_bool(qdict, "reuse", 0);
+    int full = qdict_get_try_bool(qdict, "full", 0);
+    enum NewImageMode mode;
+    Error *errp = NULL;
+
+    if (!filename) {
+        error_set(&errp, QERR_MISSING_PARAMETER, "target");
+        hmp_handle_error(mon, &errp);
+        return;
+    }
+
+    if (reuse) {
+        mode = NEW_IMAGE_MODE_EXISTING;
+    } else {
+        mode = NEW_IMAGE_MODE_ABSOLUTE_PATHS;
+    }
+
+    if (!full) {
+        error_setg(&errp, "-f is not yet implemented");
+        hmp_handle_error(mon, &errp);
+        return;
+    }
+
+    qmp_drive_backup(device, filename, !!format, format,
+                     true, mode, false, 0, false, 0, false, 0, &errp);
+    hmp_handle_error(mon, &errp);
+}
+
 void hmp_snapshot_blkdev(Monitor *mon, const QDict *qdict)
 {
     const char *device = qdict_get_str(qdict, "device");
diff --git a/hmp.h b/hmp.h
index 56d2e92..6c3bdcd 100644
--- a/hmp.h
+++ b/hmp.h
@@ -55,6 +55,7 @@ void hmp_balloon(Monitor *mon, const QDict *qdict);
 void hmp_block_resize(Monitor *mon, const QDict *qdict);
 void hmp_snapshot_blkdev(Monitor *mon, const QDict *qdict);
 void hmp_drive_mirror(Monitor *mon, const QDict *qdict);
+void hmp_drive_backup(Monitor *mon, const QDict *qdict);
 void hmp_migrate_cancel(Monitor *mon, const QDict *qdict);
 void hmp_migrate_set_downtime(Monitor *mon, const QDict *qdict);
 void hmp_migrate_set_speed(Monitor *mon, const QDict *qdict);
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH] block: add drive_backup HMP command

[Kevin Wolf]

Am 25.06.2013 um 14:23 hat Stefan Hajnoczi geschrieben:
> Make "drive_backup" available on the HMP monitor:
> 
>   drive_backup [-n] [-f] device target [format]
> 
> The -n flag requests QEMU to reuse the image found in new-image-file,
> instead of recreating it from scratch.
> 
> The -f flag requests QEMU to copy the whole disk, so that the result
> does not need a backing file.  Note that this flag *must* currently be
> passed since the other sync modes ('none' and 'top') have not been
> implemented yet.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  hmp-commands.hx | 20 ++++++++++++++++++++
>  hmp.c           | 33 +++++++++++++++++++++++++++++++++
>  hmp.h           |  1 +
>  3 files changed, 54 insertions(+)

> --- a/hmp.c
> +++ b/hmp.c
> @@ -889,6 +889,39 @@ void hmp_drive_mirror(Monitor *mon, const QDict *qdict)
>      hmp_handle_error(mon, &errp);
>  }
>  
> +void hmp_drive_backup(Monitor *mon, const QDict *qdict)
> +{
> +    const char *device = qdict_get_str(qdict, "device");
> +    const char *filename = qdict_get_str(qdict, "target");
> +    const char *format = qdict_get_try_str(qdict, "format");
> +    int reuse = qdict_get_try_bool(qdict, "reuse", 0);
> +    int full = qdict_get_try_bool(qdict, "full", 0);
> +    enum NewImageMode mode;
> +    Error *errp = NULL;

> +
> +    if (!filename) {
> +        error_set(&errp, QERR_MISSING_PARAMETER, "target");
> +        hmp_handle_error(mon, &errp);
> +        return;
> +    }
> +
> +    if (reuse) {
> +        mode = NEW_IMAGE_MODE_EXISTING;
> +    } else {
> +        mode = NEW_IMAGE_MODE_ABSOLUTE_PATHS;
> +    }
> +
> +    if (!full) {
> +        error_setg(&errp, "-f is not yet implemented");
> +        hmp_handle_error(mon, &errp);
> +        return;
> +    }

Then why make it a valid option and confuse users in the help text by
describing options that don't really exist?

> +    qmp_drive_backup(device, filename, !!format, format,
> +                     true, mode, false, 0, false, 0, false, 0, &errp);
> +    hmp_handle_error(mon, &errp);
> +}

Kevin

Re: [Qemu-devel] [PATCH] block: add drive_backup HMP command

[Paolo Bonzini]

Il 25/06/2013 15:26, Kevin Wolf ha scritto:
>> > +    if (!full) {
>> > +        error_setg(&errp, "-f is not yet implemented");
>> > +        hmp_handle_error(mon, &errp);
>> > +        return;
>> > +    }
> Then why make it a valid option and confuse users in the help text by
> describing options that don't really exist?
> 

Because otherwise we're stuck with a meaning of the flag that is
different between drive-mirror and block-backup.

Paolo

Re: [Qemu-devel] [PATCH] block: add drive_backup HMP command

[Kevin Wolf]

Am 25.06.2013 um 15:49 hat Paolo Bonzini geschrieben:
> Il 25/06/2013 15:26, Kevin Wolf ha scritto:
> >> > +    if (!full) {
> >> > +        error_setg(&errp, "-f is not yet implemented");
> >> > +        hmp_handle_error(mon, &errp);
> >> > +        return;
> >> > +    }
> > Then why make it a valid option and confuse users in the help text by
> > describing options that don't really exist?
> 
> Because otherwise we're stuck with a meaning of the flag that is
> different between drive-mirror and block-backup.

Do you mean when "otherwise" isn't only "we don't add -f now", but also
"we accidentally add a -f with different meaning later"? Not sure if
there's a real danger of that when we're aware that we want -f with the
same meaning as for mirroring.

Apart from that, it's HMP, so even in the unlikely case that we mess up,
fixing it is still an option.

Kevin

Re: [Qemu-devel] [PATCH] block: add drive_backup HMP command

[Paolo Bonzini]

Il 25/06/2013 16:06, Kevin Wolf ha scritto:
> Am 25.06.2013 um 15:49 hat Paolo Bonzini geschrieben:
>> Il 25/06/2013 15:26, Kevin Wolf ha scritto:
>>>>> +    if (!full) {
>>>>> +        error_setg(&errp, "-f is not yet implemented");
>>>>> +        hmp_handle_error(mon, &errp);
>>>>> +        return;
>>>>> +    }
>>> Then why make it a valid option and confuse users in the help text by
>>> describing options that don't really exist?
>>
>> Because otherwise we're stuck with a meaning of the flag that is
>> different between drive-mirror and block-backup.
> 
> Do you mean when "otherwise" isn't only "we don't add -f now", but also
> "we accidentally add a -f with different meaning later"? Not sure if
> there's a real danger of that when we're aware that we want -f with the
> same meaning as for mirroring.

We have drive-mirror with:
* the default is 'top'
* -f gives 'full'

block-backup for now only implements 'full'.  If we do not force the
user to add -f, the default is 'full' and we should not change it later.

However, I would move the "not yet implemented" error from HMP to QMP.
This way, both drive-mirror and block-backup will have a mandatory
'sync' argument.  We plan to implement it anyway, and it makes sense imo
to avoid gratuitous differences in the APIs.

Paolo

> Apart from that, it's HMP, so even in the unlikely case that we mess up,
> fixing it is still an option.
> 
> Kevin
> 

Re: [Qemu-devel] [PATCH] block: add drive_backup HMP command

[Kevin Wolf]

Am 25.06.2013 um 16:36 hat Paolo Bonzini geschrieben:
> Il 25/06/2013 16:06, Kevin Wolf ha scritto:
> > Am 25.06.2013 um 15:49 hat Paolo Bonzini geschrieben:
> >> Il 25/06/2013 15:26, Kevin Wolf ha scritto:
> >>>>> +    if (!full) {
> >>>>> +        error_setg(&errp, "-f is not yet implemented");
> >>>>> +        hmp_handle_error(mon, &errp);
> >>>>> +        return;
> >>>>> +    }
> >>> Then why make it a valid option and confuse users in the help text by
> >>> describing options that don't really exist?
> >>
> >> Because otherwise we're stuck with a meaning of the flag that is
> >> different between drive-mirror and block-backup.
> > 
> > Do you mean when "otherwise" isn't only "we don't add -f now", but also
> > "we accidentally add a -f with different meaning later"? Not sure if
> > there's a real danger of that when we're aware that we want -f with the
> > same meaning as for mirroring.
> 
> We have drive-mirror with:
> * the default is 'top'
> * -f gives 'full'
> 
> block-backup for now only implements 'full'.  If we do not force the
> user to add -f, the default is 'full' and we should not change it later.

Oh, yes, I read it the wrong way round. The error message is actually
wrong, because the unimplemented thing isn't -f, but _not_ using -f. The
code is actually doing what you describe, though.

> However, I would move the "not yet implemented" error from HMP to QMP.
> This way, both drive-mirror and block-backup will have a mandatory
> 'sync' argument.  We plan to implement it anyway, and it makes sense imo
> to avoid gratuitous differences in the APIs.

I agree.

Kevin

Re: [Qemu-devel] [PATCH] block: add drive_backup HMP command

[Ian Main]

On Tue, Jun 25, 2013 at 04:36:53PM +0200, Paolo Bonzini wrote:
> Il 25/06/2013 16:06, Kevin Wolf ha scritto:
> > Am 25.06.2013 um 15:49 hat Paolo Bonzini geschrieben:
> >> Il 25/06/2013 15:26, Kevin Wolf ha scritto:
> >>>>> +    if (!full) {
> >>>>> +        error_setg(&errp, "-f is not yet implemented");
> >>>>> +        hmp_handle_error(mon, &errp);
> >>>>> +        return;
> >>>>> +    }
> >>> Then why make it a valid option and confuse users in the help text by
> >>> describing options that don't really exist?
> >>
> >> Because otherwise we're stuck with a meaning of the flag that is
> >> different between drive-mirror and block-backup.
> > 
> > Do you mean when "otherwise" isn't only "we don't add -f now", but also
> > "we accidentally add a -f with different meaning later"? Not sure if
> > there's a real danger of that when we're aware that we want -f with the
> > same meaning as for mirroring.
> 
> We have drive-mirror with:
> * the default is 'top'
> * -f gives 'full'
> 
> block-backup for now only implements 'full'.  If we do not force the
> user to add -f, the default is 'full' and we should not change it later.
> 
> However, I would move the "not yet implemented" error from HMP to QMP.
> This way, both drive-mirror and block-backup will have a mandatory
> 'sync' argument.  We plan to implement it anyway, and it makes sense imo
> to avoid gratuitous differences in the APIs.

I'm working on a patch to implement sync modes.  I should be posting it
soon.

	Ian

Re: [Qemu-devel] [PATCH] block: add drive_backup HMP command

[Stefan Hajnoczi]

On Tue, Jun 25, 2013 at 04:36:53PM +0200, Paolo Bonzini wrote:
> Il 25/06/2013 16:06, Kevin Wolf ha scritto:
> > Am 25.06.2013 um 15:49 hat Paolo Bonzini geschrieben:
> >> Il 25/06/2013 15:26, Kevin Wolf ha scritto:
> >>>>> +    if (!full) {
> >>>>> +        error_setg(&errp, "-f is not yet implemented");
> >>>>> +        hmp_handle_error(mon, &errp);
> >>>>> +        return;
> >>>>> +    }
> >>> Then why make it a valid option and confuse users in the help text by
> >>> describing options that don't really exist?
> >>
> >> Because otherwise we're stuck with a meaning of the flag that is
> >> different between drive-mirror and block-backup.
> > 
> > Do you mean when "otherwise" isn't only "we don't add -f now", but also
> > "we accidentally add a -f with different meaning later"? Not sure if
> > there's a real danger of that when we're aware that we want -f with the
> > same meaning as for mirroring.
> 
> We have drive-mirror with:
> * the default is 'top'
> * -f gives 'full'
> 
> block-backup for now only implements 'full'.  If we do not force the
> user to add -f, the default is 'full' and we should not change it later.
> 
> However, I would move the "not yet implemented" error from HMP to QMP.
> This way, both drive-mirror and block-backup will have a mandatory
> 'sync' argument.  We plan to implement it anyway, and it makes sense imo
> to avoid gratuitous differences in the APIs.

Thanks, I should have explained this in the commit message.  Requiring
-f now avoids changing semantics later when 'top' becomes the default to
match drive-mirror.

I'll move the error into qmp_drive_backup().

Stefan