[Qemu-devel] Re: [PATCH 18/22] savevm: Convert do_delvm() to QObject, QError

[Kevin Wolf <kwolf@redhat.com>]

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