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

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

To QemuOptionParameter, 0 value to SIZE/NUMBEr is meaningless, same
effect as not setting the option at all. But yes, we can use list->assigned to
differentiate option set with 0 value or non-set option.

 

> +            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->QemuOptsList conversion.
> +     * Will remove after all drivers switch to QemuOptsList.
> +     */
> +    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?   

Different places, patch 4 changes QemuOpts .name and .str to (char *). Here is

QemuOptsDesc, in which .name, .help, .def_value_str are (const char *).

 

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