Re: [Qemu-devel] [PATCH 07/11] qapi: Convert loadvm

[Pavel Hrdina <phrdina@redhat.com>]

On 17.4.2013 01:43, Eric Blake wrote:
> On 04/16/2013 10:05 AM, Pavel Hrdina wrote:
>> QMP command vm-snapshot-load and HMP command loadvm behave similar to
>> vm-snapshot-delete and delvm. The only different is that they will load
>> the snapshot instead of deleting it.
>>
>> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
>> ---
>>   hmp-commands.hx         | 16 +++++-----
>>   hmp.c                   | 35 ++++++++++++++++++++++
>>   hmp.h                   |  1 +
>>   include/sysemu/sysemu.h |  1 -
>>   monitor.c               | 12 --------
>>   qapi-schema.json        | 18 +++++++++++
>>   qmp-commands.hx         | 38 ++++++++++++++++++++++++
>>   savevm.c                | 79 ++++++++++++++++++++++++++++++-------------------
>>   vl.c                    |  7 ++++-
>>   9 files changed, 156 insertions(+), 51 deletions(-)
>>
>> diff --git a/hmp-commands.hx b/hmp-commands.hx
>> index d1701ce..e80410b 100644
>> --- a/hmp-commands.hx
>> +++ b/hmp-commands.hx
>> @@ -324,17 +324,19 @@ ETEXI
>>
>>       {
>>           .name       = "loadvm",
>> -        .args_type  = "name:s",
>> -        .params     = "tag|id",
>> -        .help       = "restore a VM snapshot from its tag or id",
>> -        .mhandler.cmd = do_loadvm,
>> +        .args_type  = "id:-i,name:s",
>> +        .params     = "[-i] tag|[id]",
>> +        .help       = "restore a VM snapshot from its tag or id if -i flag is provided",
>
> Same comments as 4/11 about possibilities of making this look nicer
> ('name' seems nicer than 'tag|[id]').
>
>>   }
>> +
>> +void hmp_vm_snapshot_load(Monitor *mon, const QDict *qdict)
>> +{
>> +    const char *name = qdict_get_try_str(qdict, "name");
>> +    const bool id = qdict_get_try_bool(qdict, "id", false);
>> +    Error *local_err = NULL;
>> +    SnapshotInfo *info = NULL;
>> +
>> +    if (id) {
>> +        info = qmp_vm_snapshot_load(false, NULL, true, name, &local_err);
>> +    } else {
>> +        info = qmp_vm_snapshot_load(true, name, false, NULL, &local_err);
>> +    }
>
> Could be written without if/else:
> qmp_vm_snapshot_load(id, name, !id, name, &local_err);
>
> but doesn't really bother me as written.
>
>> +
>> +    if (info) {
>> +        char buf[256];
>
> Fixed-size buffer...

I'll leave it there for now and Wenchao could update this in his patch 
series.

>
>> +        QEMUSnapshotInfo sn = {
>> +            .vm_state_size = info->vm_state_size,
>> +            .date_sec = info->date_sec,
>> +            .date_nsec = info->date_nsec,
>> +            .vm_clock_nsec = info->vm_clock_sec * 1000000000 +
>> +                             info->vm_clock_nsec,
>> +        };
>> +        pstrcpy(sn.id_str, sizeof(sn.id_str), info->id);
>> +        pstrcpy(sn.name, sizeof(sn.name), info->name);
>> +        monitor_printf(mon, "Loaded snapshot's info:\n");
>> +        monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), NULL));
>> +        monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), &sn));
>
> ...but potentially large amount of information to be placed in it.  I
> would MUCH rather see this code using gstring, or even direct printing.
>   I think you need to base this on top of the ideas here:
>
> https://lists.gnu.org/archive/html/qemu-devel/2013-04/msg02540.html
>
> These two series need to be coordinated to minimize churn and rebase
> conflicts on cleanups such as removing buffer truncation from
> bdrv_snapshot_dump.
>
> Hmm, I probably should have mentioned this on 4/11 as well.
>
>> +    } else if (!error_is_set(&local_err)) {
>
> Same comments as 4/11, in that info == NULL should imply that local_err
> is set.
>
>> +++ b/qapi-schema.json
>> @@ -3522,3 +3522,21 @@
>>   ##
>>   { 'command': 'vm-snapshot-delete', 'data': {'*name': 'str', '*id': 'str'},
>>     'returns': 'SnapshotInfo' }
>> +
>> +##
>> +# @vm-snapshot-load:
>> +#
>> +# Set the whole virtual machine to the snapshot identified by the tag
>> +# or the unique snapshot id or both. It must be a snapshot of a whole VM and
>> +# at least one of the name or id parameter must be specified.
>> +#
>> +# @name: tag of existing snapshot
>> +#
>> +# @id: id of existing snapshot
>
> As in 4/11, mark these with #optional.
>
>> +#
>> +# Returns: SnapshotInfo on success
>> +#
>> +# Since: 1.5
>> +##
>> +{ 'command': 'vm-snapshot-load', 'data': {'*name': 'str', '*id': 'str'},
>> +  'returns': 'SnapshotInfo' }
>
> Same comments as 4/11 on return value design decisions.
>
>> @@ -2393,26 +2393,35 @@ void qmp_xen_save_devices_state(const char *filename, Error **errp)
>>           vm_start();
>>   }
>>
>> -int load_vmstate(const char *name)
>> +SnapshotInfo *qmp_vm_snapshot_load(bool has_name, const char *name,
>> +                                   bool has_id, const char *id, Error **errp)
>>   {
>>       BlockDriverState *bs, *bs_vm_state;
>>       QEMUSnapshotInfo sn;
>>       QEMUFile *f;
>> -    Error *local_err;
>> +    SnapshotInfo *info = NULL;
>> +    int saved_vm_running;
>
> This should be bool.  runstate_is_running() is currently typed as int,
> but it defers to runstate_check() which is bool; fixing runstate_is* to
> consistently use bool would be worthy of a separate cleanup patch.
>
>> +
>> +    if (!has_name && !has_id) {
>> +        error_setg(errp, "name or id must be specified");
>> +        return NULL;
>> +    }
>>
>>       bs_vm_state = bdrv_snapshots();
>>       if (!bs_vm_state) {
>> -        error_report("No block device supports snapshots");
>> -        return -ENOTSUP;
>> +        error_setg(errp, "no block device supports snapshots");
>> +        return NULL;
>>       }
>>
>>       /* Don't even try to load empty VM states */
>> -    if (!bdrv_snapshot_find(bs_vm_state, &sn, name, name, true)) {
>> -        return -ENOENT;
>> -    } else if (sn.vm_state_size == 0) {
>> -        error_report("This is a disk-only snapshot. Revert to it offline "
>> -            "using qemu-img.");
>> -        return -EINVAL;
>> +    if (!bdrv_snapshot_find(bs_vm_state, &sn, name, id, false)) {
>
> Oh, I didn't notice this before, but 4/11 has the same issue.  You are
> not guaranteed that 'name' and 'id' are initialized correctly unless you
> first check 'has_name' and 'has_id'.  And given my suggestion above of
> possibly compressing things to:
>
> qmp_vm_snapshot_load(id, name, !id, name, &local_err);
>
> then it REALLY matters that you pass NULL for the parameters that are
> not provided by the caller.  To be safe, you must use one of:
>
> if (!has_name) {
>      name = NULL;
> }
> if (!has_id) {
>      id = NULL;
> }
> ... bdrv_snapshot_find(bs_vm_state, &sn, name, id, false) ...
>

Thanks for pointing this out, I didn't realize that.

> or:
>
> bdrv_snapshot_find(bs_vm_state, &sn,
>                     has_name ? name : NULL,
>                     has_id ? id : NULL,
>                     false)
>
> unless you take on the bigger task of ensuring that ALL callers pass in
> NULL when has_name is false (and the code generator for QMP currently
> doesn't guarantee that).
>
>> +        return NULL;
>> +    }
>> +
>> +    if (sn.vm_state_size == 0) {
>> +        error_setg(errp, "this is a disk-only snapshot, revert to it offline "
>> +            "using qemu-img");
>
> Indentation is off; a continuation should line up to the character after
> the '(' of the line before.
>
>> +        return NULL;
>>       }
>>
>>       /* Verify if there is any device that doesn't support snapshots and is
>> @@ -2425,29 +2434,28 @@ int load_vmstate(const char *name)
>>           }
>>
>>           if (!bdrv_can_snapshot(bs)) {
>> -            error_report("Device '%s' is writable but does not support snapshots.",
>> -                               bdrv_get_device_name(bs));
>> -            return -ENOTSUP;
>> +            error_setg(errp, "device '%s' is writable but does not support "
>> +                       "snapshots", bdrv_get_device_name(bs));
>> +            return NULL;
>>           }
>
> Pre-existing, but why do we care?  Loading a snapshot doesn't require
> writing, just reading.

For now I'll leave it there. This basically check that there is for 
example some raw disk and therefore the snapshot state wouldn't be 
loaded for all block devices.

We anyway need to improve savevm, loadvm and delvm to do a better 
checking and as you said behave transactionally. This also applies for 
most of your comments about functionality and it should be fixed in 
another patch series.

>
>>
>> -        if (!bdrv_snapshot_find(bs, &sn, name, name, true)) {
>> -            error_report("Device '%s' does not have the requested snapshot '%s'",
>> -                           bdrv_get_device_name(bs), name);
>> -            return -ENOENT;
>> +        if (!bdrv_snapshot_find(bs, &sn, name, id, false)) {
>> +            return NULL;
>
> Missing error report.
>
>>           }
>>       }
>>
>> +    saved_vm_running = runstate_is_running();
>> +    vm_stop(RUN_STATE_RESTORE_VM);
>> +
>>       /* Flush all IO requests so they don't interfere with the new state.  */
>>       bdrv_drain_all();
>>
>>       bs = NULL;
>>       while ((bs = bdrv_next(bs))) {
>>           if (bdrv_can_snapshot(bs)) {
>> -            bdrv_snapshot_goto(bs, name, &local_err);
>> -            if (error_is_set(&local_err)) {
>> -                error_report("%s", error_get_pretty(local_err));
>> -                error_free(local_err);
>> -                return -1;
>> +            bdrv_snapshot_goto(bs, sn.name, errp);
>> +            if (error_is_set(errp)) {
>> +                return NULL;
>
> Pre-existing, but yet another case of partial failure, where it might be
> nicer to let the caller know if we failed halfway through loading a
> located snapshot, differently than complete failure where we didn't
> change any state.
>