| Message ID | 1372163039-28332-1-git-send-email-stefanha@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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
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
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 >
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
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
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
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);
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(+)