From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33266) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WShMQ-0000th-RN for qemu-devel@nongnu.org; Wed, 26 Mar 2014 02:30:52 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WShMP-0008Nd-29 for qemu-devel@nongnu.org; Wed, 26 Mar 2014 02:30:50 -0400 Received: from mail-wi0-x22a.google.com ([2a00:1450:400c:c05::22a]:34156) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WShMO-0008NA-NO for qemu-devel@nongnu.org; Wed, 26 Mar 2014 02:30:49 -0400 Received: by mail-wi0-f170.google.com with SMTP id bs8so4270467wib.5 for ; Tue, 25 Mar 2014 23:30:47 -0700 (PDT) MIME-Version: 1.0 Sender: kgrace.liu@gmail.com In-Reply-To: <5331F699.3010502@redhat.com> References: <1395396763-26081-1-git-send-email-cyliu@suse.com> <1395396763-26081-9-git-send-email-cyliu@suse.com> <5331F699.3010502@redhat.com> Date: Wed, 26 Mar 2014 14:30:47 +0800 Message-ID: From: Chunyan Liu Content-Type: multipart/alternative; boundary=f46d0444e81feef33704f57c9ba6 Subject: Re: [Qemu-devel] [PATCH v23 08/32] add convert functions between QEMUOptionParameter to QemuOpts List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: qemu-devel@nongnu.org, Stefan Hajnoczi --f46d0444e81feef33704f57c9ba6 Content-Type: text/plain; charset=UTF-8 2014-03-26 5:35 GMT+08:00 Eric Blake : > On 03/21/2014 04:12 AM, Chunyan Liu wrote: > > Add two temp convert functions between QEMUOptionParameter to QemuOpts, > > s/convert/conversion/ here and in subject > > > so that next patch can use it. It will simplify later patch for easier > > review. And will be finally removed after all backend drivers switch to > > QemuOpts. > > > > Signed-off-by: Chunyan Liu > > --- > > > +++ b/include/qemu/option.h > > @@ -103,6 +103,11 @@ typedef struct QemuOptDesc { > > } QemuOptDesc; > > > > struct QemuOptsList { > > + /* FIXME: Temp used for QEMUOptionParamter->QemuOpts conversion to > > + * indicate free memory. Will remove after all drivers switch to > QemuOpts. > > + */ > > + bool mallocd; > > Spelling looks odd; I might have used 'allocated'. But as it gets > deleted later, I don't care. > > > + num_opts = count_option_parameters(list); > > + opts = g_malloc0(sizeof(QemuOptsList) + > > + (num_opts + 1) * sizeof(QemuOptDesc)); > > [1]... > > > + while (list && list->name) { > > + opts->desc[i].name = g_strdup(list->name); > > + opts->desc[i].help = g_strdup(list->help); > > + switch (list->type) { > > + case OPT_FLAG: > > + opts->desc[i].type = QEMU_OPT_BOOL; > > + opts->desc[i].def_value_str = > > + g_strdup(list->value.n ? "on" : "off"); > > This always sets def_value_str... > Here, to a boolean type, 0 equals to false. > > > + break; > > + > > + case OPT_NUMBER: > > + opts->desc[i].type = QEMU_OPT_NUMBER; > > + if (list->value.n) { > > + opts->desc[i].def_value_str = > > + g_strdup_printf("%" PRIu64, list->value.n); > > + } > > ...whereas this only sets def_value_str for non-zero values. But can't > 0 be a valid setting? Is this mismatch in what gets converted going to > bite us? Should you be paying attention to list->assigned instead or in > addition to just checking for non-zero values? > All places calling params_to_opts() are to convert .create_options to .create_opts, and unify later processing. For all OPT_SIZE or OPT_NUMBER option in .create_options, if value is set, it won't be 0. 0 equals to "not set". And for the calling cases, list->assigned is always false. > > > + break; > > + > > + case OPT_SIZE: > > + opts->desc[i].type = QEMU_OPT_SIZE; > > + if (list->value.n) { > > + opts->desc[i].def_value_str = > > + g_strdup_printf("%" PRIu64, list->value.n); > > + } > > Same question for 0 values. > > > + break; > > + > > + case OPT_STRING: > > + opts->desc[i].type = QEMU_OPT_STRING; > > + opts->desc[i].def_value_str = g_strdup(list->value.s); > > + break; > > + } > > + > > + i++; > > + list++; > > + opts->desc[i].name = NULL; > > ...[1] This assignment is dead code, because you used malloc0 which > guarantees that desc[i].name is already NULL. > > > +/* convert QemuOpts to QEMUOptionParamter > > s/Paramter/Parameter/ > > > + * Note: result QEMUOptionParameter has shorter lifetime than > > + * input QemuOpts. > > + * FIXME: this function will be removed after all drivers > > + * switch to QemuOpts > > + */ > > +QEMUOptionParameter *opts_to_params(QemuOpts *opts) > > +{ > > > + num_opts = count_opts_list(opts->list); > > + dest = g_malloc0((num_opts + 1) * sizeof(QEMUOptionParameter)); > > + > > > + i++; > > + desc++; > > + dest[i].name = NULL; > > Another dead assignment. > > > + } > > + > > + return dest; > > +} > > + > > +void qemu_opts_free(QemuOptsList *list) > > +{ > > + /* List members point to new malloced space and need to free. > > + * FIXME: > > + * Introduced for QEMUOptionParamter->QemuOpts conversion. > > + * Will remove after all drivers switch to QemuOpts. > > + */ > > + if (list && list->mallocd) { > > + QemuOptDesc *desc = list->desc; > > + while (desc && desc->name) { > > + g_free((char *)desc->name); > > + g_free((char *)desc->help); > > Are these casts still necessary, given patch 4? > > > + g_free((char *)desc->def_value_str); > > However, it looks like you are correct that this one is casting away const. > > -- > Eric Blake eblake redhat com +1-919-301-3266 > Libvirt virtualization library http://libvirt.org > > --f46d0444e81feef33704f57c9ba6 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable



2014-03-26 5:35 GMT+08:00 Eric Blake <eblake@redhat.com>:
On 03/21/2014 04:12 AM, Chunyan Liu wrote: > Add two temp convert functions between QEMUOptionParameter to QemuOpts= ,

s/convert/conversion/ here and in subject

> so that next patch can use it. It will simplify later patch for easier=
> review. And will be finally removed after all backend drivers switch t= o
> QemuOpts.
>
> Signed-off-by: Chunyan Liu <cyliu= @suse.com>
> ---

> +++ b/include/qemu/option.h
> @@ -103,6 +103,11 @@ typedef struct QemuOptDesc {
> =C2=A0} QemuOptDesc;
>
> =C2=A0struct QemuOptsList {
> + =C2=A0 =C2=A0/* FIXME: Temp used for QEMUOptionParamter->QemuOpts= conversion to
> + =C2=A0 =C2=A0 * indicate free memory. Will remove after all drivers = switch to QemuOpts.
> + =C2=A0 =C2=A0 */
> + =C2=A0 =C2=A0bool mallocd;

Spelling looks odd; I might have used 'allocated'. =C2=A0But as it = gets
deleted later, I don't care.

> + =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 (num_opts + 1) * sizeof(QemuOptDesc));

[1]...

> + =C2=A0 =C2=A0while (list && list->name) {
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0opts->desc[i].name =3D g_strdup(list-&= gt;name);
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0opts->desc[i].help =3D g_strdup(list-&= gt;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
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0g_strdup(list= ->value.n ? "on" : "off");

This always sets def_value_str...

Here,= to a boolean type, 0 equals to false.
=C2=A0

> + =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=A0opts->desc= [i].def_value_str =3D
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0g_strdup_printf("%" PRIu64, list->value.n);
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0}

...whereas this only sets def_value_str for non-zero values. =C2=A0But can&= #39;t
0 be a valid setting? =C2=A0Is this mismatch in what gets converted going t= o
bite us? =C2=A0Should you be paying attention to list->assigned instead = or in
addition to just checking for non-zero values?

All places calling params_to_opts() are to convert .create_options = to .create_opts,
and unify later processing. For all OPT_SIZE= or OPT_NUMBER option in .create_options,
if value is set, it won't be 0. 0 equals to "not set&qu= ot;. And for the calling cases, list->assigned
is always false.
=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=A0opts->desc= [i].def_value_str =3D
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0g_strdup_printf("%" PRIu64, list->value.n);
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0}

Same question for 0 values.

> + =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=A0opts->desc[i].def_value_= str =3D g_strdup(list->value.s);
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0break;
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0}
> +
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0i++;
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0list++;
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0opts->desc[i].name =3D NULL;

...[1] This assignment is dead code, because you used malloc0 which
guarantees that desc[i].name is already NULL.

> +/* convert QemuOpts to QEMUOptionParamter

s/Paramter/Parameter/

> + * Note: result QEMUOptionParameter has shorter lifetime than
> + * input QemuOpts.
> + * FIXME: this function will be removed after all drivers
> + * switch to QemuOpts
> + */
> +QEMUOptionParameter *opts_to_params(QemuOpts *opts)
> +{

> + =C2=A0 =C2=A0num_opts =3D count_opts_list(opts->list);
> + =C2=A0 =C2=A0dest =3D g_malloc0((num_opts + 1) * sizeof(QEMUOptionPa= rameter));
> +

> + =C2=A0 =C2=A0 =C2=A0 =C2=A0i++;
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0desc++;
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0dest[i].name =3D NULL;

Another dead assignment.

> + =C2=A0 =C2=A0}
> +
> + =C2=A0 =C2=A0return dest;
> +}
> +
> +void qemu_opts_free(QemuOptsList *list)
> +{
> + =C2=A0 =C2=A0/* List members point to new malloced space and need to= free.
> + =C2=A0 =C2=A0 * FIXME:
> + =C2=A0 =C2=A0 * Introduced for QEMUOptionParamter->QemuOpts conve= rsion.
> + =C2=A0 =C2=A0 * Will remove after all drivers switch to QemuOpts. > + =C2=A0 =C2=A0 */
> + =C2=A0 =C2=A0if (list && list->mallocd) {
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0QemuOptDesc *desc =3D list->desc;
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0while (desc && desc->name) { > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0g_free((char *)desc->nam= e);
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0g_free((char *)desc->hel= p);

Are these casts still necessary, given patch 4?

> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0g_free((char *)desc->def= _value_str);

However, it looks like you are correct that this one is casting away const.=

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


--f46d0444e81feef33704f57c9ba6--