Re: [Qemu-devel] [PATCH 6/6] snapshot: human monitor interface

[Wenchao Xia <xiawenc@linux.vnet.ibm.com>]

 > On Mon, Dec 17, 2012 at 02:25:09PM +0800, Wenchao Xia wrote:
>> @@ -983,17 +983,22 @@ ETEXI
>>
>>       {
>>           .name       = "snapshot_blkdev",
>> -        .args_type  = "reuse:-n,device:B,snapshot-file:s?,format:s?",
>> -        .params     = "[-n] device [new-image-file] [format]",
>> -        .help       = "initiates a live snapshot\n\t\t\t"
>> -                      "of device. If a new image file is specified, the\n\t\t\t"
>> -                      "new image file will become the new root image.\n\t\t\t"
>> -                      "If format is specified, the snapshot file will\n\t\t\t"
>> -                      "be created in that format. Otherwise the\n\t\t\t"
>> -                      "snapshot will be internal! (currently unsupported).\n\t\t\t"
>> -                      "The default format is qcow2.  The -n flag requests QEMU\n\t\t\t"
>> -                      "to reuse the image found in new-image-file, instead of\n\t\t\t"
>> -                      "recreating it from scratch.",
>> +        .args_type  = "internal:-i,reuse:-n,device:B,snapshot-file:s?,format:s?",
>> +        .params     = "[-i] [-n] device [new-image-file] [format]",
>
> Please rename snapshot-file and new-image-file because it is now either
> the external snapshot filename or the internal snapshot name - it's not
> always a file!
>
   OK.

>> +        .help       = "initiates a live snapshot of device.\n\t\t\t"
>> +                      "  The -i flag requests QEMU to create internal snapshot\n\t\t\t"
>> +                      "instead of external one.\n\t\t\t"
>> +                      "  The -n flag requests QEMU to use existing snapshot\n\t\t\t"
>> +                      "instead of creating new snapshot, which would fails if\n\t\t\t"
>> +                      "snapshot does not exist ahead.\n\t\t\t"
>
> "which fails if the snapshot does not exist already"
>
   Will use this, thanks.

>> +                      "  new-image-file is the snapshot's name, in external case\n\t\t\t"
>> +                      "it is the new image's name which will become the new root\n\t\t\t"
>> +                      "image and must be specified, in internal case it is the\n\t\t\t"
>> +                      "record's name and if not specified QEMU will create\n\t\t\t"
>> +                      "internal snapshot with name generated according to time.\n\t\t\t"
>> +                      "  format is only valid in external case, which is the new\n\t\t\t"
>> +                      "snapshot image's format. If not sepcified default format\n\t\t\t"
>> +                      "qcow2 will be used.",
>
> "If not specified, the default format is qcow2."
>
>>           .mhandler.cmd = hmp_snapshot_blkdev,
>>       },
>>
>> @@ -1004,6 +1009,25 @@ Snapshot device, using snapshot file as target if provided
>>   ETEXI
>>
>>       {
>> +        .name       = "snapshot_delete_blkdev",
>
> Internal snapshots can already be deleted with delvm but there is no
> existing command for external snapshots.
>
>> +        .args_type  = "internal:-i,device:B,snapshot-file:s",
>> +        .params     = "[-i] device new-image-file",
>> +        .help       = "delete a snapshot  synchronous.\n\t\t\t"
>
> "synchronous" is implied for HMP commands, they are all like this so I
> don't think it's necessary to mention it.
>
>> +                      "  The -i flag requests QEMU to delete internal snapshot\n\t\t\t"
>> +                      "instead of external one.\n\t\t\t"
>> +                      "  new-image-file is the snapshot's name, in external case\n\t\t\t"
>> +                      "it is the image's name which is not supported now.\n\t\t\t"
>
> I'm not sure how useful this interface is.  If we have no implementation
> for deleting external snapshots and libvirt already uses delvm, then I'd
> prefer you drop this command from the patch series.
>
> Later on, when there is code to implement external snapshot deletion it
> can be added.  Then there is no risk that this command design doesn't
> work and we have to change it again.  (Remember libvirt already uses
> delvm so adding snapshot_delete_blkdev without external snapshot
> deletion just adds code churn.)
>
   OK, this interface will be dropped to make things simpler.

>> @@ -1486,8 +1510,8 @@ ETEXI
>>
>>       {
>>           .name       = "info",
>> -        .args_type  = "item:s?",
>> -        .params     = "[subcommand]",
>> +        .args_type  = "item:s?,params:s?",
>> +        .params     = "[subcommand] [params]",
>>           .help       = "show various information about the system state",
>>           .mhandler.cmd = do_info,
>>       },
>
> What does this change do?
>
   Pls ignore this, another serial was sent which extend hmp info
infra first.

>> diff --git a/hmp.c b/hmp.c
>> index 180ba2b..f247f51 100644
>> --- a/hmp.c
>> +++ b/hmp.c
>> @@ -806,20 +806,40 @@ void hmp_snapshot_blkdev(Monitor *mon, const QDict *qdict)
>>       const char *filename = qdict_get_try_str(qdict, "snapshot-file");
>>       const char *format = qdict_get_try_str(qdict, "format");
>>       int reuse = qdict_get_try_bool(qdict, "reuse", 0);
>> +    int internal = qdict_get_try_bool(qdict, "internal", 0);
>>       enum NewImageMode mode;
>> +    enum SnapshotType type;
>>       Error *errp = NULL;
>>
>> -    if (!filename) {
>> -        /* In the future, if 'snapshot-file' is not specified, the snapshot
>> -           will be taken internally. Today it's actually required. */
>> +    if ((!internal) && (!filename)) {
>> +        /* in external case filename must be set, should we generate
>> +         it automatically? */
>
> Picking a filename would mainly be useful to humans.  libvirt and other
> management tools will want full control anyway.  So the question is: is
> there a naming policy that will be useful to most human users?
>
> If yes, please implement it.  If no, please drop this comment.
>
   It will be dropped.

>> diff --git a/monitor.c b/monitor.c
>> index c0e32d6..81de470 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -124,11 +124,14 @@ typedef struct mon_cmd_t {
>>       void (*user_print)(Monitor *mon, const QObject *data);
>>       union {
>>           void (*info)(Monitor *mon);
>> +        void (*info_qdict)(Monitor *mon, const QDict *qdict);
>>           void (*cmd)(Monitor *mon, const QDict *qdict);
>> -        int  (*cmd_new)(Monitor *mon, const QDict *params, QObject **ret_data);
>> +        int  (*cmd_new)(Monitor *mon, const QDict *params,
>> +                        QObject **ret_data);
>>           int  (*cmd_async)(Monitor *mon, const QDict *params,
>>                             MonitorCompletion *cb, void *opaque);
>>       } mhandler;
>> +    int info_cmd_need_qdict;
>>       int flags;
>
> The union discriminator is the flags field (e.g.  MONITOR_CMD_ASYNC).
> Please follow that style.
>
> (If you use a boolean variable, please use the bool type instead of
> int.)
>
   There would be another way to extend hmp info command.

>>   } mon_cmd_t;
>>
>> @@ -824,7 +827,11 @@ static void do_info(Monitor *mon, const QDict *qdict)
>>           goto help;
>>       }
>>
>> -    cmd->mhandler.info(mon);
>> +    if (cmd->info_cmd_need_qdict) {
>> +        cmd->mhandler.info_qdict(mon, qdict);
>> +    } else {
>> +        cmd->mhandler.info(mon);
>> +    }
>>       return;
>>
>>   help:
>
> The generic monitor changes need to go in a separate commit.
>
>> @@ -2605,10 +2612,12 @@ static mon_cmd_t info_cmds[] = {
>>       },
>>       {
>>           .name       = "snapshots",
>> -        .args_type  = "",
>> -        .params     = "",
>> -        .help       = "show the currently saved VM snapshots",
>> -        .mhandler.info = do_info_snapshots,
>> +        .args_type  = "device:B?",
>> +        .params     = "[device]",
>> +        .help       = "show the currently saved VM snapshots or snapshots on "
>> +                      "a single device.",
>> +        .mhandler.info_qdict = do_info_snapshots,
>> +        .info_cmd_need_qdict = 1,
>>       },
>>       {
>>           .name       = "status",
>
> This change to the info snapshots command needs to go in a separate
> commit.
>
   It will go to hmp serial.

>> diff --git a/savevm.c b/savevm.c
>> index c027529..5982aa9 100644
>> --- a/savevm.c
>> +++ b/savevm.c
>> @@ -2336,7 +2336,7 @@ void do_delvm(Monitor *mon, const QDict *qdict)
>>       }
>>   }
>>
>> -void do_info_snapshots(Monitor *mon)
>> +static void do_info_snapshots_vm(Monitor *mon)
>>   {
>>       BlockDriverState *bs, *bs1;
>>       QEMUSnapshotInfo *sn_tab, *sn, s, *sn_info = &s;
>> @@ -2400,6 +2400,59 @@ void do_info_snapshots(Monitor *mon)
>>
>>   }
>>
>> +static void do_info_snapshots_blk(Monitor *mon, const char *device)
>> +{
>> +    BlockDriverState *bs;
>> +    QEMUSnapshotInfo *sn_tab, *sn;
>> +    int nb_sns, i;
>> +    char buf[256];
>> +
>> +    /* find the target bs */
>> +    bs = bdrv_find(device);
>> +    if (!bs) {
>> +        monitor_printf(mon, "Device '%s' not found.\n", device);
>> +        return ;
>> +    }
>> +
>> +    if (!bdrv_can_snapshot(bs)) {
>> +        monitor_printf(mon, "Device '%s' can't have snapshot.\n", device);
>> +        return ;
>> +    }
>> +
>> +    nb_sns = bdrv_snapshot_list(bs, &sn_tab);
>> +    if (nb_sns < 0) {
>> +        monitor_printf(mon, "Device %s bdrv_snapshot_list: error %d\n",
>> +                       device, nb_sns);
>> +        return;
>> +    }
>> +
>> +    if (nb_sns == 0) {
>> +        monitor_printf(mon, "There is no snapshot available.\n");
>> +        return;
>> +    }
>> +
>> +    monitor_printf(mon, "Device %s:\n", device);
>> +    monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), NULL));
>> +    for (i = 0; i < nb_sns; i++) {
>> +        sn = &sn_tab[i];
>> +        monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), sn));
>> +    }
>> +    g_free(sn_tab);
>> +    return;
>> +}
>
> Return at the end of a void function is not necessary and QEMU code
> doesn't use it.  Please use QEMU style.
>
   OK.

> Stefan
>


-- 
Best Regards

Wenchao Xia