| Message ID | 1271797792-24571-19-git-send-email-lcapitulino@redhat.com |
|---|---|
| State | New |
| Headers | show |
Am 20.04.2010 23:09, schrieb Luiz Capitulino: > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> > --- > qemu-monitor.hx | 3 ++- > savevm.c | 14 ++++++++++---- > sysemu.h | 2 +- > 3 files changed, 13 insertions(+), 6 deletions(-) > > diff --git a/qemu-monitor.hx b/qemu-monitor.hx > index 5ea5748..71cb1a2 100644 > --- a/qemu-monitor.hx > +++ b/qemu-monitor.hx > @@ -274,7 +274,8 @@ ETEXI > .args_type = "name:s", > .params = "tag|id", > .help = "delete a VM snapshot from its tag or id", > - .mhandler.cmd = do_delvm, > + .user_print = monitor_user_noop, > + .mhandler.cmd_new = do_delvm, > }, > > STEXI > diff --git a/savevm.c b/savevm.c > index 643273e..031eeff 100644 > --- a/savevm.c > +++ b/savevm.c > @@ -1815,24 +1815,30 @@ int load_vmstate(const char *name) > return 0; > } > > -void do_delvm(Monitor *mon, const QDict *qdict) > +int do_delvm(Monitor *mon, const QDict *qdict, QObject **ret_data) > { > + int ret; > DriveInfo *dinfo; > BlockDriverState *bs, *bs1; > const char *name = qdict_get_str(qdict, "name"); > > bs = get_bs_snapshots(); > if (!bs) { > - monitor_printf(mon, "No block device supports snapshots\n"); > - return; > + qerror_report(QERR_SNAPSHOT_NO_DEVICE); > + return -1; > } > > + ret = -1; > + > QTAILQ_FOREACH(dinfo, &drives, next) { > bs1 = dinfo->bdrv; > if (bdrv_has_snapshot(bs1)) { > - delete_snapshot(bs1, name); > + /* FIXME: will report multiple failures in QMP */ > + ret = delete_snapshot(bs1, name); > } > } > + > + return (ret < 0 ? -1 : 0); Doesn't this return success when the first drive fails and the second one succeeds? Kevin
On Wed, 21 Apr 2010 16:18:18 +0200 Kevin Wolf <kwolf@redhat.com> wrote: > Am 20.04.2010 23:09, schrieb Luiz Capitulino: > > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> > > --- > > qemu-monitor.hx | 3 ++- > > savevm.c | 14 ++++++++++---- > > sysemu.h | 2 +- > > 3 files changed, 13 insertions(+), 6 deletions(-) > > > > diff --git a/qemu-monitor.hx b/qemu-monitor.hx > > index 5ea5748..71cb1a2 100644 > > --- a/qemu-monitor.hx > > +++ b/qemu-monitor.hx > > @@ -274,7 +274,8 @@ ETEXI > > .args_type = "name:s", > > .params = "tag|id", > > .help = "delete a VM snapshot from its tag or id", > > - .mhandler.cmd = do_delvm, > > + .user_print = monitor_user_noop, > > + .mhandler.cmd_new = do_delvm, > > }, > > > > STEXI > > diff --git a/savevm.c b/savevm.c > > index 643273e..031eeff 100644 > > --- a/savevm.c > > +++ b/savevm.c > > @@ -1815,24 +1815,30 @@ int load_vmstate(const char *name) > > return 0; > > } > > > > -void do_delvm(Monitor *mon, const QDict *qdict) > > +int do_delvm(Monitor *mon, const QDict *qdict, QObject **ret_data) > > { > > + int ret; > > DriveInfo *dinfo; > > BlockDriverState *bs, *bs1; > > const char *name = qdict_get_str(qdict, "name"); > > > > bs = get_bs_snapshots(); > > if (!bs) { > > - monitor_printf(mon, "No block device supports snapshots\n"); > > - return; > > + qerror_report(QERR_SNAPSHOT_NO_DEVICE); > > + return -1; > > } > > > > + ret = -1; > > + > > QTAILQ_FOREACH(dinfo, &drives, next) { > > bs1 = dinfo->bdrv; > > if (bdrv_has_snapshot(bs1)) { > > - delete_snapshot(bs1, name); > > + /* FIXME: will report multiple failures in QMP */ > > + ret = delete_snapshot(bs1, name); > > } > > } > > + > > + return (ret < 0 ? -1 : 0); > > Doesn't this return success when the first drive fails and the second > one succeeds? Yes, but what's the real status when this happens? Did delvm succeed or not? I think users will ask the same as we'll print error messages. Another question is: is it acceptable to return on the first error?
Am 22.04.2010 15:48, schrieb Luiz Capitulino: > On Wed, 21 Apr 2010 16:18:18 +0200 > Kevin Wolf <kwolf@redhat.com> wrote: > >> Am 20.04.2010 23:09, schrieb Luiz Capitulino: >>> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> >>> --- >>> qemu-monitor.hx | 3 ++- >>> savevm.c | 14 ++++++++++---- >>> sysemu.h | 2 +- >>> 3 files changed, 13 insertions(+), 6 deletions(-) >>> >>> diff --git a/qemu-monitor.hx b/qemu-monitor.hx >>> index 5ea5748..71cb1a2 100644 >>> --- a/qemu-monitor.hx >>> +++ b/qemu-monitor.hx >>> @@ -274,7 +274,8 @@ ETEXI >>> .args_type = "name:s", >>> .params = "tag|id", >>> .help = "delete a VM snapshot from its tag or id", >>> - .mhandler.cmd = do_delvm, >>> + .user_print = monitor_user_noop, >>> + .mhandler.cmd_new = do_delvm, >>> }, >>> >>> STEXI >>> diff --git a/savevm.c b/savevm.c >>> index 643273e..031eeff 100644 >>> --- a/savevm.c >>> +++ b/savevm.c >>> @@ -1815,24 +1815,30 @@ int load_vmstate(const char *name) >>> return 0; >>> } >>> >>> -void do_delvm(Monitor *mon, const QDict *qdict) >>> +int do_delvm(Monitor *mon, const QDict *qdict, QObject **ret_data) >>> { >>> + int ret; >>> DriveInfo *dinfo; >>> BlockDriverState *bs, *bs1; >>> const char *name = qdict_get_str(qdict, "name"); >>> >>> bs = get_bs_snapshots(); >>> if (!bs) { >>> - monitor_printf(mon, "No block device supports snapshots\n"); >>> - return; >>> + qerror_report(QERR_SNAPSHOT_NO_DEVICE); >>> + return -1; >>> } >>> >>> + ret = -1; >>> + >>> QTAILQ_FOREACH(dinfo, &drives, next) { >>> bs1 = dinfo->bdrv; >>> if (bdrv_has_snapshot(bs1)) { >>> - delete_snapshot(bs1, name); >>> + /* FIXME: will report multiple failures in QMP */ >>> + ret = delete_snapshot(bs1, name); >>> } >>> } >>> + >>> + return (ret < 0 ? -1 : 0); >> >> Doesn't this return success when the first drive fails and the second >> one succeeds? > > Yes, but what's the real status when this happens? Did delvm > succeed or not? > > I think users will ask the same as we'll print error messages. Don't know. We probably need ternary logic. :-) I think I'd call it an error. But either way, that it depends on the order of disks just doesn't feel right. The status of "disk1 fails and disk2 succeeds" should not be different from "disk1 succeeds and disk2 fails". > Another question is: is it acceptable to return on the first > error? I guess it is. Kevin
diff --git a/qemu-monitor.hx b/qemu-monitor.hx index 5ea5748..71cb1a2 100644 --- a/qemu-monitor.hx +++ b/qemu-monitor.hx @@ -274,7 +274,8 @@ ETEXI .args_type = "name:s", .params = "tag|id", .help = "delete a VM snapshot from its tag or id", - .mhandler.cmd = do_delvm, + .user_print = monitor_user_noop, + .mhandler.cmd_new = do_delvm, }, STEXI diff --git a/savevm.c b/savevm.c index 643273e..031eeff 100644 --- a/savevm.c +++ b/savevm.c @@ -1815,24 +1815,30 @@ int load_vmstate(const char *name) return 0; } -void do_delvm(Monitor *mon, const QDict *qdict) +int do_delvm(Monitor *mon, const QDict *qdict, QObject **ret_data) { + int ret; DriveInfo *dinfo; BlockDriverState *bs, *bs1; const char *name = qdict_get_str(qdict, "name"); bs = get_bs_snapshots(); if (!bs) { - monitor_printf(mon, "No block device supports snapshots\n"); - return; + qerror_report(QERR_SNAPSHOT_NO_DEVICE); + return -1; } + ret = -1; + QTAILQ_FOREACH(dinfo, &drives, next) { bs1 = dinfo->bdrv; if (bdrv_has_snapshot(bs1)) { - delete_snapshot(bs1, name); + /* FIXME: will report multiple failures in QMP */ + ret = delete_snapshot(bs1, name); } } + + return (ret < 0 ? -1 : 0); } void do_info_snapshots(Monitor *mon) diff --git a/sysemu.h b/sysemu.h index fa921df..ee14b8c 100644 --- a/sysemu.h +++ b/sysemu.h @@ -55,7 +55,7 @@ void qemu_system_reset(void); void do_savevm(Monitor *mon, const QDict *qdict); int load_vmstate(const char *name); -void do_delvm(Monitor *mon, const QDict *qdict); +int do_delvm(Monitor *mon, const QDict *qdict, QObject **ret_data); void do_info_snapshots(Monitor *mon); void cpu_synchronize_all_states(void);
Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> --- qemu-monitor.hx | 3 ++- savevm.c | 14 ++++++++++---- sysemu.h | 2 +- 3 files changed, 13 insertions(+), 6 deletions(-)