From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53148) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WFeQH-0002Ah-3K for qemu-devel@nongnu.org; Tue, 18 Feb 2014 01:44:54 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WFeQF-0005lQ-3z for qemu-devel@nongnu.org; Tue, 18 Feb 2014 01:44:53 -0500 Received: from mail-ve0-x236.google.com ([2607:f8b0:400c:c01::236]:60431) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WFeQE-0005lJ-TX for qemu-devel@nongnu.org; Tue, 18 Feb 2014 01:44:51 -0500 Received: by mail-ve0-f182.google.com with SMTP id jy13so13415607veb.27 for ; Mon, 17 Feb 2014 22:44:50 -0800 (PST) MIME-Version: 1.0 Sender: kgrace.liu@gmail.com In-Reply-To: <52FC0DF5.4030307@redhat.com> References: <1392186806-10418-1-git-send-email-cyliu@suse.com> <1392186806-10418-7-git-send-email-cyliu@suse.com> <52FC0DF5.4030307@redhat.com> Date: Tue, 18 Feb 2014 14:44:50 +0800 Message-ID: From: Chunyan Liu Content-Type: multipart/alternative; boundary=047d7bfea07cdfab4104f2a89b5f Subject: Re: [Qemu-devel] [PATCH v20 06/26] change block layer to support both QemuOpts and QEMUOptionParameter List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: Kevin Wolf , Dong Xu Wang , qemu-devel@nongnu.org, stefanha@redhat.com --047d7bfea07cdfab4104f2a89b5f Content-Type: text/plain; charset=UTF-8 2014-02-13 8:12 GMT+08:00 Eric Blake : > On 02/11/2014 11:33 PM, Chunyan Liu wrote: > > Change block layer to support both QemuOpts and QEMUOptionParameter. > > After this patch, it will change backend drivers one by one. At the end, > > QEMUOptionParameter will be removed and only QemuOpts is kept. > > > > Signed-off-by: Dong Xu Wang > > Signed-off-by: Chunyan Liu > > --- > > block.c | 110 > ++++++++++++++++++++++++++++---------------- > > block/cow.c | 2 +- > > block/qcow.c | 2 +- > > block/qcow2.c | 2 +- > > block/qed.c | 2 +- > > block/raw_bsd.c | 2 +- > > block/vhdx.c | 2 +- > > block/vmdk.c | 4 +- > > block/vvfat.c | 2 +- > > include/block/block.h | 4 +- > > include/block/block_int.h | 4 +- > > include/qemu/option.h | 2 + > > qemu-img.c | 87 +++++++++++++++++++++-------------- > > util/qemu-option.c | 111 > +++++++++++++++++++++++++++++++++++++++++++++ > > 14 files changed, 250 insertions(+), 86 deletions(-) > > > > +++ b/include/block/block_int.h > > @@ -118,6 +118,8 @@ struct BlockDriver { > > void (*bdrv_rebind)(BlockDriverState *bs); > > int (*bdrv_create)(const char *filename, QEMUOptionParameter > *options, > > Error **errp); > > + int (*bdrv_create2)(const char *filename, QemuOpts *opts, > > + Error **errp); > > Maybe a FIXME comment that shows we plan on removing the duplicate and > renaming back to a single sane name in a few more commits. > > > +++ b/include/qemu/option.h > > @@ -168,4 +168,6 @@ int qemu_opts_foreach(QemuOptsList *list, > qemu_opts_loopfunc func, void *opaque, > > QemuOptsList *qemu_opts_append(QemuOptsList *dst, QemuOptsList *list); > > void qemu_opts_free(QemuOptsList *list); > > void qemu_opts_print_help(QemuOptsList *list); > > +QEMUOptionParameter *opts_to_params(QemuOpts *opts); > > +QemuOptsList *params_to_opts(QEMUOptionParameter *list); > > I'd split this commit into two - one that adds these two conversion > functions, then the other that adds the create2 callback. > > > +++ b/util/qemu-option.c > > @@ -1396,3 +1396,114 @@ void qemu_opts_print_help(QemuOptsList *list) > > list->desc[i].help : ""); > > } > > } > > + > > +/* convert QEMUOptionParameter to QemuOpts */ > > +QemuOptsList *params_to_opts(QEMUOptionParameter *list) > > +{ > > + QemuOptsList *opts = NULL; > > + size_t num_opts, i = 0; > > + > > + if (!list) { > > + return NULL; > > + } > > + > > + num_opts = count_option_parameters(list); > > + opts = g_malloc0(sizeof(QemuOptsList) + > > + (num_opts + 1) * sizeof(QemuOptDesc)); > > Indentation looks off. > > > + QTAILQ_INIT(&opts->head); > > + opts->desc[i].name = NULL; > > + > > + while (list && list->name) { > > + opts->desc[i].name = strdup(list->name); > > + opts->desc[i].help = strdup(list->help); > > + switch (list->type) { > > + case OPT_FLAG: > > + opts->desc[i].type = QEMU_OPT_BOOL; > > + opts->desc[i].def_value_str = list->value.n ? "on" : "off"; > > + break; > > + > > + case OPT_NUMBER: > > + opts->desc[i].type = QEMU_OPT_NUMBER; > > + if (list->value.n) { > > + char tmp[100]; > > + sprintf(tmp, "%" PRIu64, list->value.n); > > + opts->desc[i].def_value_str = strdup(tmp); > > Eww. Just use g_strdup_printf and avoid tmp[] altogether. > > > + } > > + break; > > + > > + case OPT_SIZE: > > + opts->desc[i].type = QEMU_OPT_SIZE; > > + if (list->value.n) { > > + char tmp[100]; > > + sprintf(tmp, "%" PRIu64, list->value.n); > > + opts->desc[i].def_value_str = strdup(tmp); > > and again > > > + } > > + break; > > + > > + case OPT_STRING: > > + opts->desc[i].type = QEMU_OPT_STRING; > > + if (list->value.s) { > > + opts->desc[i].def_value_str = strdup(list->value.s); > > This is a lot of use of strdup() without checking for malloc failure; > wouldn't it be better to use g_strdup (which can't fail, and which > gracefully handles NULL so you can also drop the 'if') and fixing the > counterpart free to use g_free? > > Will correct. Thanks. > -- > Eric Blake eblake redhat com +1-919-301-3266 > Libvirt virtualization library http://libvirt.org > > --047d7bfea07cdfab4104f2a89b5f Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable



2014-02-13 8:12 GMT+08:00 Eric Blake <eblake@redhat.com>:
On 02/11/2014 11:33 PM, Chun= yan Liu wrote:
> Change block layer to support both QemuOpts and QEMUOptionParameter. > After this patch, it will change backend drivers one by one. At the en= d,
> QEMUOptionParameter will be removed and only QemuOpts is kept.
>
> Signed-off-by: Dong Xu Wang <wdongxu@linux.vnet.ibm.com>
> Signed-off-by: Chunyan Liu <cyliu= @suse.com>
> ---
> =C2=A0block.c =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 | =C2=A0110 ++++++++++++++++++++++++++++----------------
> =C2=A0block/cow.c =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 | = =C2=A0 =C2=A02 +-
> =C2=A0block/qcow.c =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0| = =C2=A0 =C2=A02 +-
> =C2=A0block/qcow2.c =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 | =C2=A0= =C2=A02 +-
> =C2=A0block/qed.c =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 | = =C2=A0 =C2=A02 +-
> =C2=A0block/raw_bsd.c =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 | =C2=A0 =C2= =A02 +-
> =C2=A0block/vhdx.c =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0| = =C2=A0 =C2=A02 +-
> =C2=A0block/vmdk.c =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0| = =C2=A0 =C2=A04 +-
> =C2=A0block/vvfat.c =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 | =C2=A0= =C2=A02 +-
> =C2=A0include/block/block.h =C2=A0 =C2=A0 | =C2=A0 =C2=A04 +-
> =C2=A0include/block/block_int.h | =C2=A0 =C2=A04 +-
> =C2=A0include/qemu/option.h =C2=A0 =C2=A0 | =C2=A0 =C2=A02 +
> =C2=A0qemu-img.c =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0| =C2=A0 87 +++++++++++++++++++++--------------
> =C2=A0util/qemu-option.c =C2=A0 =C2=A0 =C2=A0 =C2=A0| =C2=A0111 ++++++= +++++++++++++++++++++++++++++++++++++++
> =C2=A014 files changed, 250 insertions(+), 86 deletions(-)
>
> +++ b/include/block/block_int.h
> @@ -118,6 +118,8 @@ struct BlockDriver {
> =C2=A0 =C2=A0 =C2=A0void (*bdrv_rebind)(BlockDriverState *bs);
> =C2=A0 =C2=A0 =C2=A0int (*bdrv_create)(const char *filename, QEMUOptio= nParameter *options,
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 Error **errp);
> + =C2=A0 =C2=A0int (*bdrv_create2)(const char *filename, QemuOpts *opt= s,
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 Error **errp);

Maybe a FIXME comment that shows we plan on removing the duplicate an= d
renaming back to a single sane name in a few more commits.

> +++ b/include/qemu/option.h
> @@ -168,4 +168,6 @@ int qemu_opts_foreach(QemuOptsList *list, qemu_opt= s_loopfunc func, void *opaque,
> =C2=A0QemuOptsList *qemu_opts_append(QemuOptsList *dst, QemuOptsList *= list);
> =C2=A0void qemu_opts_free(QemuOptsList *list);
> =C2=A0void qemu_opts_print_help(QemuOptsList *list);
> +QEMUOptionParameter *opts_to_params(QemuOpts *opts);
> +QemuOptsList *params_to_opts(QEMUOptionParameter *list);

I'd split this commit into two - one that adds these two conversi= on
functions, then the other that adds the create2 callback.

> +++ b/util/qemu-option.c
> @@ -1396,3 +1396,114 @@ void qemu_opts_print_help(QemuOptsList *list)<= br> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 list->desc[= i].help : "");
> =C2=A0 =C2=A0 =C2=A0}
> =C2=A0}
> +
> +/* convert QEMUOptionParameter to QemuOpts */
> +QemuOptsList *params_to_opts(QEMUOptionParameter *list)
> +{
> + =C2=A0 =C2=A0QemuOptsList *opts =3D NULL;
> + =C2=A0 =C2=A0size_t num_opts, i =3D 0;
> +
> + =C2=A0 =C2=A0if (!list) {
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0return NULL;
> + =C2=A0 =C2=A0}
> +
> + =C2=A0 =C2=A0num_opts =3D count_option_parameters(list);
> + =C2=A0 =C2=A0opts =3D g_malloc0(sizeof(QemuOptsList) +
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0(num_opts + 1) * sizeof(QemuOptDesc))= ;

Indentation looks off.

> + =C2=A0 =C2=A0QTAILQ_INIT(&opts->head);
> + =C2=A0 =C2=A0opts->desc[i].name =3D NULL;
> +
> + =C2=A0 =C2=A0while (list && list->name) {
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0opts->desc[i].name =3D strdup(list->= ;name);
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0opts->desc[i].help =3D strdup(list->= ;help);
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0switch (list->type) {
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0case OPT_FLAG:
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0opts->desc[i].type =3D Q= EMU_OPT_BOOL;
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0opts->desc[i].def_value_= str =3D list->value.n ? "on" : "off";
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0break;
> +
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0case OPT_NUMBER:
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0opts->desc[i].type =3D Q= EMU_OPT_NUMBER;
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (list->value.n) {
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0char tmp[100]= ;
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0sprintf(tmp, = "%" PRIu64, list->value.n);
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0opts->desc= [i].def_value_str =3D strdup(tmp);

Eww. =C2=A0Just use g_strdup_printf and avoid tmp[] altogether.

> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0}
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0break;
> +
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0case OPT_SIZE:
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0opts->desc[i].type =3D Q= EMU_OPT_SIZE;
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (list->value.n) {
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0char tmp[100]= ;
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0sprintf(tmp, = "%" PRIu64, list->value.n);
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0opts->desc= [i].def_value_str =3D strdup(tmp);

and again

> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0}
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0break;
> +
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0case OPT_STRING:
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0opts->desc[i].type =3D Q= EMU_OPT_STRING;
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (list->value.s) {
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0opts->desc= [i].def_value_str =3D strdup(list->value.s);

This is a lot of use of strdup() without checking for malloc failure;=
wouldn't it be better to use g_strdup (which can't fail, and which<= br> gracefully handles NULL so you can also drop the 'if') and fixing t= he
counterpart free to use g_free?

Will correct. Thanks.
=C2=A0
--
Eric Blake =C2=A0 eblake redhat com =C2=A0 =C2=A0+1-919-301-3266
Libvirt virtualization library http://libvirt.org


--047d7bfea07cdfab4104f2a89b5f--