Re: [Qemu-devel] [PATCH v22 06/25] add convert functions between QEMUOptionParameter to QemuOpts

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4252 bytes --]

On 03/10/2014 01:31 AM, Chunyan Liu wrote:
> Add two temp convert functions between QEMUOptionParameter to QemuOpts, so that
> next patch can use it. It will simplify next patch for easier review.
> 
> Signed-off-by: Chunyan Liu <cyliu@suse.com>
> ---
>  include/qemu/option.h |   2 +
>  util/qemu-option.c    | 105 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 107 insertions(+)
> 

> +
> +/* 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));
> +    QTAILQ_INIT(&opts->head);
> +    opts->desc[i].name = NULL;

Dead assignment to NULL, thanks to the g_malloc0 above.

> +
> +    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 = list->value.n ? "on" : "off";

Ouch.  The pointer used here points to constant memory...

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

...while the pointer used here points to heap memory.  Yet I see nothing
in the struct that you use to track whether to free the string or not,
which means this is more likely a memory leak, but also a potential for
a crash during an errant call to g_free().  You absolutely must manage
the memory correctly, if you are going to conditionally heap-allocate
def_value_str.

For the sake of conversions between the two types, may I suggest an idea:
in 'struct QemuOpt', add a char[24] buffer (that's large enough to hold
the maximum stringized uint value).  Then, instead of malloc'ing memory
with g_strdup_printf, you instead format integers in place.  You've
already malloc'd the size for the QemuOpt, and now the string
representation also fits within that space without needing a secondary
malloc.

Whether or not you end up keeping the stringizing buffer permanently
part of QemuOpt, or delete it after you delete QEMUOptionParameter,
remains to be seen at the end of the series.

> +        case OPT_STRING:
> +            opts->desc[i].type = QEMU_OPT_STRING;
> +            opts->desc[i].def_value_str = g_strdup(list->value.s);

Here, just declare that your temporary QemuOptsList result from the
function has a shorter lifetime than the input QemuOptionParameter, and
set the def_value_str pointer to the original list->value.s instead of
duplicating it.  Then, you are back to the situation where freeing the
temporary QemuOptsList doesn't leak and doesn't risk double frees.


> +QEMUOptionParameter *opts_to_params(QemuOpts *opts)
> +{
> +    QEMUOptionParameter *dest = NULL;
> +    QemuOptDesc *desc;
> +    size_t num_opts, i = 0;
> +    const char *tmp;
> +
> +    if (!opts || !opts->list || !opts->list->desc) {
> +        return NULL;
> +    }
> +
> +    num_opts = count_opts_list(opts->list);
> +    dest = g_malloc0((num_opts + 1) * sizeof(QEMUOptionParameter));
> +    dest[i].name = NULL;
> +
> +    desc = opts->list->desc;
> +    while (desc && desc->name) {
> +        dest[i].name = g_strdup(desc->name);
> +        dest[i].help = g_strdup(desc->help);

I didn't research QEMUOptionParameter close enough to know if you will
be causing any memory leaks on the reverse conversion - but since the
end goal of the series is to delete QEMUOptionParameter, this method
will eventually disappear, so I guess I could live with leaks here
(although it would be nice to document it in the commit message, if you
do indeed leak).

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]