| Message ID | 1395396763-26081-9-git-send-email-cyliu@suse.com |
|---|---|
| State | New |
| Headers | show |
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 <cyliu@suse.com> > --- > +++ 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? > + 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.
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 to > > QemuOpts. > > > > Signed-off-by: Chunyan Liu <cyliu@suse.com> > > --- > > > +++ 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 > >
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 to > > QemuOpts. > > > > Signed-off-by: Chunyan Liu <cyliu@suse.com> > > --- > > > +++ 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 > >
diff --git a/include/qemu/option.h b/include/qemu/option.h index fbf5dc2..fd6f075 100644 --- a/include/qemu/option.h +++ 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; + const char *name; const char *implied_opt_name; bool merge_lists; /* Merge multiple uses of option into a single list? */ @@ -167,5 +172,8 @@ void qemu_opts_print(QemuOpts *opts); int qemu_opts_foreach(QemuOptsList *list, qemu_opts_loopfunc func, void *opaque, int abort_on_failure); void qemu_opts_print_help(QemuOptsList *list); +void qemu_opts_free(QemuOptsList *list); +QEMUOptionParameter *opts_to_params(QemuOpts *opts); +QemuOptsList *params_to_opts(QEMUOptionParameter *list); #endif diff --git a/util/qemu-option.c b/util/qemu-option.c index 315a7bb..4bdcfbf 100644 --- a/util/qemu-option.c +++ b/util/qemu-option.c @@ -1323,3 +1323,157 @@ int qemu_opts_foreach(QemuOptsList *list, qemu_opts_loopfunc func, void *opaque, loc_pop(&loc); return rc; } + +static size_t count_opts_list(QemuOptsList *list) +{ + QemuOptDesc *desc = NULL; + size_t num_opts = 0; + + if (!list) { + return 0; + } + + desc = list->desc; + while (desc && desc->name) { + num_opts++; + desc++; + } + + return num_opts; +} + +/* Convert QEMUOptionParameter to QemuOpts + * FIXME: this function will be removed after all drivers + * switch 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); + /* (const char *) members will point to malloced space and need to free */ + opts->mallocd = true; + + 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"); + 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); + } + 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); + } + 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; + } + + return opts; +} + +/* convert QemuOpts to QEMUOptionParamter + * 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) +{ + QEMUOptionParameter *dest = NULL; + QemuOptDesc *desc; + size_t num_opts, i = 0; + const char *tmp; + + if (!opts || !opts->list || !opts->list->desc) { + return NULL; + } + assert(!opts_accepts_any(opts)); + + num_opts = count_opts_list(opts->list); + dest = g_malloc0((num_opts + 1) * sizeof(QEMUOptionParameter)); + + desc = opts->list->desc; + while (desc && desc->name) { + dest[i].name = desc->name; + dest[i].help = desc->help; + switch (desc->type) { + case QEMU_OPT_STRING: + dest[i].type = OPT_STRING; + tmp = qemu_opt_get(opts, desc->name); + dest[i].value.s = g_strdup(tmp); + break; + + case QEMU_OPT_BOOL: + dest[i].type = OPT_FLAG; + dest[i].value.n = qemu_opt_get_bool(opts, desc->name, 0) ? 1 : 0; + break; + + case QEMU_OPT_NUMBER: + dest[i].type = OPT_NUMBER; + dest[i].value.n = qemu_opt_get_number(opts, desc->name, 0); + break; + + case QEMU_OPT_SIZE: + dest[i].type = OPT_SIZE; + dest[i].value.n = qemu_opt_get_size(opts, desc->name, 0); + break; + } + + i++; + desc++; + dest[i].name = NULL; + } + + 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); + g_free((char *)desc->def_value_str); + desc++; + } + } + + g_free(list); +}
Add two temp convert functions between QEMUOptionParameter to QemuOpts, 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 <cyliu@suse.com> --- Finally not following Eric's suggestion of adding char tmp[] to QemuOptList to store stringized uint value, but choosing g_strdup and add flag 'mallocd' to QemuOptList to indicate free the mallocd space, since: 1. there could be many unit options need to store, so, adding char tmp[] in QemuOptList couldn't solve problem, but should add char tmp[] in QemuOptDesc. Then, for those not unit type, the char tmp[] space is wasted. 2. in later qemu_opts_append() function, since it accept both QemuOptsList and QEMUOptionParameter as input, for QEMUOptionParamter, it will temp converted to QemuOpts, then append to result list; after that, the converted QemuOpts should be freed. It's risky that some memory that the result list members point to might be freed. So, simply g_strdup is safer, the only thing is to free in this case. include/qemu/option.h | 8 +++ util/qemu-option.c | 154 ++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 162 insertions(+)