2014-03-26 5:35 GMT+08:00 Eric Blake <ebl...@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 <cy...@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
>
>
