From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:56762)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <stefanha@gmail.com>) id 1VC83D-0002a1-Lr
	for qemu-devel@nongnu.org; Wed, 21 Aug 2013 09:02:25 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <stefanha@gmail.com>) id 1VC835-0007I0-7w
	for qemu-devel@nongnu.org; Wed, 21 Aug 2013 09:02:15 -0400
Received: from mail-wg0-x22d.google.com ([2a00:1450:400c:c00::22d]:51809)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <stefanha@gmail.com>) id 1VC835-0007Hu-24
	for qemu-devel@nongnu.org; Wed, 21 Aug 2013 09:02:07 -0400
Received: by mail-wg0-f45.google.com with SMTP id n12so366696wgh.0
	for <qemu-devel@nongnu.org>; Wed, 21 Aug 2013 06:02:06 -0700 (PDT)
Date: Wed, 21 Aug 2013 15:02:03 +0200
From: Stefan Hajnoczi <stefanha@gmail.com>
Message-ID: <20130821130203.GB11975@stefanha-thinkpad.redhat.com>
References: <1375071932-31627-1-git-send-email-famz@redhat.com>
	<1375071932-31627-4-git-send-email-famz@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1375071932-31627-4-git-send-email-famz@redhat.com>
Subject: Re: [Qemu-devel] [RFC PATCH 3/4] qmp: Add "snapshot=" option to
 nbd-server-add
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Fam Zheng <famz@redhat.com>
Cc: kwolf@redhat.com, pbonzini@redhat.com, imain@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com

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.