Re: [Qemu-devel] [PATCH v23 08/32] add convert functions between QEMUOptionParameter to QemuOpts

[Chunyan Liu]

2014-03-26 5:35 GMT+08:00 Eric Blake <address@hidden>:

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 <address@hidden>
> ---

> +++ 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