From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:52309)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <stefanha@gmail.com>) id 1WNNYa-0001ex-P8
	for qemu-devel@nongnu.org; Tue, 11 Mar 2014 10:21:33 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <stefanha@gmail.com>) id 1WNNYS-0002rP-B1
	for qemu-devel@nongnu.org; Tue, 11 Mar 2014 10:21:24 -0400
Received: from mail-ea0-x229.google.com ([2a00:1450:4013:c01::229]:43419)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <stefanha@gmail.com>) id 1WNNYS-0002rF-3B
	for qemu-devel@nongnu.org; Tue, 11 Mar 2014 10:21:16 -0400
Received: by mail-ea0-f169.google.com with SMTP id h14so4422677eaj.28
	for <qemu-devel@nongnu.org>; Tue, 11 Mar 2014 07:21:15 -0700 (PDT)
Date: Tue, 11 Mar 2014 15:21:12 +0100
From: Stefan Hajnoczi <stefanha@gmail.com>
Message-ID: <20140311142112.GK7761@stefanha-thinkpad.redhat.com>
References: <1394436721-21812-1-git-send-email-cyliu@suse.com>
	<1394436721-21812-13-git-send-email-cyliu@suse.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1394436721-21812-13-git-send-email-cyliu@suse.com>
Subject: Re: [Qemu-devel] [PATCH v22 12/25] qcow2.c: replace
 QEMUOptionParameter with QemuOpts
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: Chunyan Liu <cyliu@suse.com>
Cc: kwolf@redhat.com, Dong Xu Wang <wdongxu@linux.vnet.ibm.com>, qemu-devel@nongnu.org, stefanha@redhat.com

On Mon, Mar 10, 2014 at 03:31:48PM +0800, Chunyan Liu wrote:
> @@ -1636,64 +1636,64 @@ static int qcow2_create(const char *filename, QEMUOptionParameter *options,
>      int ret;
>  
>      /* Read out options */
> -    while (options && options->name) {
> -        if (!strcmp(options->name, BLOCK_OPT_SIZE)) {
> -            sectors = options->value.n / 512;
> -        } else if (!strcmp(options->name, BLOCK_OPT_BACKING_FILE)) {
> -            backing_file = options->value.s;
> -        } else if (!strcmp(options->name, BLOCK_OPT_BACKING_FMT)) {
> -            backing_fmt = options->value.s;
> -        } else if (!strcmp(options->name, BLOCK_OPT_ENCRYPT)) {
> -            flags |= options->value.n ? BLOCK_FLAG_ENCRYPT : 0;
> -        } else if (!strcmp(options->name, BLOCK_OPT_CLUSTER_SIZE)) {
> -            if (options->value.n) {
> -                cluster_size = options->value.n;
> -            }
> -        } else if (!strcmp(options->name, BLOCK_OPT_PREALLOC)) {
> -            if (!options->value.s || !strcmp(options->value.s, "off")) {
> -                prealloc = 0;
> -            } else if (!strcmp(options->value.s, "metadata")) {
> -                prealloc = 1;
> -            } else {
> -                error_setg(errp, "Invalid preallocation mode: '%s'",
> -                           options->value.s);
> -                return -EINVAL;
> -            }
> -        } else if (!strcmp(options->name, BLOCK_OPT_COMPAT_LEVEL)) {
> -            if (!options->value.s) {
> -                /* keep the default */
> -            } else if (!strcmp(options->value.s, "0.10")) {
> -                version = 2;
> -            } else if (!strcmp(options->value.s, "1.1")) {
> -                version = 3;
> -            } else {
> -                error_setg(errp, "Invalid compatibility level: '%s'",
> -                           options->value.s);
> -                return -EINVAL;
> -            }
> -        } else if (!strcmp(options->name, BLOCK_OPT_LAZY_REFCOUNTS)) {
> -            flags |= options->value.n ? BLOCK_FLAG_LAZY_REFCOUNTS : 0;
> -        }
> -        options++;
> +    sectors = qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0) / 512;
> +    backing_file = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FILE);
> +    backing_fmt = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FMT);
> +    if (qemu_opt_get_bool_del(opts, BLOCK_OPT_ENCRYPT, false)) {
> +        flags |= BLOCK_FLAG_ENCRYPT;
> +    }
> +    cluster_size = qemu_opt_get_size_del(opts, BLOCK_OPT_CLUSTER_SIZE,
> +                                         DEFAULT_CLUSTER_SIZE);
> +    buf = qemu_opt_get_del(opts, BLOCK_OPT_PREALLOC);
> +    if (!buf || !strcmp(buf, "off")) {
> +        prealloc = 0;
> +    } else if (!strcmp(buf, "metadata")) {
> +        prealloc = 1;
> +    } else {
> +        fprintf(stderr, "Invalid preallocation mode: '%s'\n", buf);

This should be error_setg(errp, "Invalid preallocation mode: '%s'",
options->value.s).

Please check the other patches too to ensure the error_setg() hasn't
been turned into fprintf().