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

[Stefan Hajnoczi]

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!

> +        .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"

> +                      "  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.)

> @@ -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?

> 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.

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

>  } 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.

> 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.

Stefan