From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:54057) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TRgZo-000866-DZ for qemu-devel@nongnu.org; Fri, 26 Oct 2012 05:51:41 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TRgZk-0008Mm-8I for qemu-devel@nongnu.org; Fri, 26 Oct 2012 05:51:40 -0400 Received: from mail-bk0-f45.google.com ([209.85.214.45]:38137) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TRgZk-0008Md-0r for qemu-devel@nongnu.org; Fri, 26 Oct 2012 05:51:36 -0400 Received: by mail-bk0-f45.google.com with SMTP id jf3so990676bkc.4 for ; Fri, 26 Oct 2012 02:51:35 -0700 (PDT) Date: Fri, 26 Oct 2012 11:51:32 +0200 From: Stefan Hajnoczi Message-ID: <20121026095132.GF19272@stefanha-thinkpad.redhat.com> References: <1351169848-28223-1-git-send-email-wdongxu@linux.vnet.ibm.com> <1351169848-28223-10-git-send-email-wdongxu@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1351169848-28223-10-git-send-email-wdongxu@linux.vnet.ibm.com> Subject: Re: [Qemu-devel] [PATCH V4 09/10] Use QemuOpts support in block layer List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Dong Xu Wang Cc: kwolf@redhat.com, qemu-devel@nongnu.org On Thu, Oct 25, 2012 at 08:57:27PM +0800, Dong Xu Wang wrote: > @@ -177,9 +177,7 @@ struct BlockDriver { > unsigned long int req, void *buf, > BlockDriverCompletionFunc *cb, void *opaque); > > - /* List of options for creating images, terminated by name == NULL */ > - QEMUOptionParameter *create_options; > - > + QemuOptsList *(*bdrv_create_options)(void); Why is .bdrv_create_options() a function? It would be less code for BlockDrivers to have a pointer: QemuOptsList *. > /* Check if compression is supported */ > if (compress) { > - QEMUOptionParameter *encryption = > - get_option_parameter(param, BLOCK_OPT_ENCRYPT); > - QEMUOptionParameter *preallocation = > - get_option_parameter(param, BLOCK_OPT_PREALLOC); > + const char *encryption = > + qemu_opt_get(param, BLOCK_OPT_ENCRYPT); > + const char *preallocation = > + qemu_opt_get(param, BLOCK_OPT_PREALLOC); > > if (!drv->bdrv_write_compressed) { > error_report("Compression not supported for this file format"); > @@ -847,15 +842,15 @@ static int img_convert(int argc, char **argv) > goto out; > } > > - if (encryption && encryption->value.n) { > + if (encryption) { encryption=off will print an error here. We need to use qemu_opt_get_bool(). > diff --git a/qemu-option.c b/qemu-option.c > index ab270f0..c391f7d 100644 > --- a/qemu-option.c > +++ b/qemu-option.c > @@ -553,10 +553,15 @@ bool qemu_opt_get_bool(QemuOpts *opts, const char *name, bool defval) > > uint64_t qemu_opt_get_number(QemuOpts *opts, const char *name, uint64_t defval) > { > - QemuOpt *opt = qemu_opt_find(opts, name); > + QemuOpt *opt; > + if (!opts) { > + return defval; > + } > + opt = qemu_opt_find(opts, name); > > - if (opt == NULL) > + if (opt == NULL) { > return defval; > + } > assert(opt->desc && opt->desc->type == QEMU_OPT_NUMBER); > return opt->value.uint; > } This should be a separate patch, but there is a problem with the change anyway: all qemu_opt_*() functions assume QemuOpts *opts is non-NULL. This change makes qemu_opt_get_number() inconsistent. This will lead to bugs when people assume qemu_opt_get_bool() also handles opts == NULL, for example. Can the qemu-img/block callers make sure opts is never NULL or will that add a lot of extra checks? Stefan