Patchwork [18/22] savevm: Convert do_delvm() to QObject, QError

login
register
mail settings
Submitter Luiz Capitulino
Date April 20, 2010, 9:09 p.m.
Message ID <1271797792-24571-19-git-send-email-lcapitulino@redhat.com>
Download mbox | patch
Permalink /patch/50594/
State New
Headers show

Comments

Luiz Capitulino - April 20, 2010, 9:09 p.m.
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(-)
Kevin Wolf - April 21, 2010, 2:18 p.m.
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
Luiz Capitulino - April 22, 2010, 1:48 p.m.
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?
Kevin Wolf - April 22, 2010, 2:31 p.m.
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

Patch

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);