Re: [Qemu-devel] [RFC PATCH 3/4] qmp: Add "snapshot=" option to nbd-server-add

[Stefan Hajnoczi]

On Mon, Jul 29, 2013 at 12:25:31PM +0800, Fam Zheng wrote:
> +/* create a point-in-time snapshot BDS from an existing BDS */
> +static BlockDriverState *nbd_create_snapshot(BlockDriverState *orig_bs)
> +{
> +    int ret;
> +    char filename[1024];
> +    BlockDriver *drv;
> +    BlockDriverState *bs;
> +    QEMUOptionParameter *options;
> +    Error *local_err = NULL;
> +
> +    bs = bdrv_new("");
> +    ret = get_tmp_filename(filename, sizeof(filename));
> +    if (ret < 0) {
> +        goto err;

unlink(filename) is not safe if this function returns an error.

It is simpler if you get the temporary filename first and then do
bdrv_new().

> +    }
> +    drv = bdrv_find_format("qcow2");
> +    if (drv < 0) {
> +        goto err;
> +    }
> +    options = parse_option_parameters("", drv->create_options, NULL);
> +    set_option_parameter_int(options, BLOCK_OPT_SIZE, 
> bdrv_getlength(orig_bs));
> +
> +    ret = bdrv_create(drv, filename, options);

This is handy but only works if the QEMU process has permission to
create temporary files.

>      n = g_malloc0(sizeof(NBDCloseNotifier));
>      n->n.notify = nbd_close_notifier;
>      n->exp = exp;
>      bdrv_add_close_notifier(bs, &n->n);
>      QTAILQ_INSERT_TAIL(&close_notifiers, n, next);
> +    return;

Please drop void return.

> diff --git a/qapi-schema.json b/qapi-schema.json
> index f82d829..bfdbe33 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -3225,7 +3225,8 @@
>  #
>  # Since: 1.3.0
>  ##
> -{ 'command': 'nbd-server-add', 'data': {'device': 'str', '*writable': 
> 'bool'} }
> +{ 'command': 'nbd-server-add', 'data': {'device': 'str', '*writable': 'bool',
> +                                        '*snapshot': 'bool'} }
>  
>  ##
>  # @nbd-server-stop:
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 2e59b0d..e398d88 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -2871,7 +2871,7 @@ EQMP
>      },
>      {
>          .name       = "nbd-server-add",
> -        .args_type  = "device:B,writable:b?",
> +        .args_type  = "device:B,writable:b?,snapshot:b?",
>          .mhandler.cmd_new = qmp_marshal_input_nbd_server_add,
>      },

Missing documentation for the new option.