Re: [Qemu-devel] [PATCH V11 1/4] add def_value_str and use it in qemu_opts_print

[Dong Xu Wang]

于 2013-1-25 2:26, Markus Armbruster 写道:
> Dong Xu Wang <address@hidden> writes:
> 
>> qemu_opts_print has no user now, so can re-write the function safely.
>>
>> qemu_opts_print will be used while using "qemu-img create", it will
>> produce the same output as previous code.
>>
>> The behavior of this function has changed:
>>
>> 1. Print every possible option, whether a value has been set or not.
>> 2. Option descriptors may provide a default value.
>> 3. Print to stdout instead of stderr.
>>
>> Previously the behavior was to print every option that has been set.
>> Options that have not been set would be skipped.
>>
>> Signed-off-by: Dong Xu Wang <address@hidden>
>> ---
>> v10->v11:
>> 1)  print all values that have actually been assigned while accept-any
>> cases.
>>
>> v7->v8:
>> 1) print "elements => accept any params" while opts_accepts_any() ==
>> true.
>> 2) since def_print_str is the default value if an option isn't set,
>> so rename it to def_value_str.
>>
>>   include/qemu/option.h |  1 +
>>   util/qemu-option.c    | 30 +++++++++++++++++++++++++-----
>>   2 files changed, 26 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/qemu/option.h b/include/qemu/option.h
>> index ba197cd..394170a 100644
>> --- a/include/qemu/option.h
>> +++ b/include/qemu/option.h
>> @@ -96,6 +96,7 @@ typedef struct QemuOptDesc {
>>       const char *name;
>>       enum QemuOptType type;
>>       const char *help;
>> +    const char *def_value_str;
>>   } QemuOptDesc;
>>   
>>   struct QemuOptsList {
>> diff --git a/util/qemu-option.c b/util/qemu-option.c
>> index f532b76..1aed418 100644
>> --- a/util/qemu-option.c
>> +++ b/util/qemu-option.c
>> @@ -863,13 +863,33 @@ void qemu_opts_del(QemuOpts *opts)
>>   int qemu_opts_print(QemuOpts *opts, void *dummy)
>>   {
>>       QemuOpt *opt;
>> +    QemuOptDesc *desc = opts->list->desc;
>>   
>> -    fprintf(stderr, "%s: %s:", opts->list->name,
>> -            opts->id ? opts->id : "<noid>");
>> -    QTAILQ_FOREACH(opt, &opts->head, next) {
>> -        fprintf(stderr, " %s=\"%s\"", opt->name, opt->str);
>> +    if (desc[0].name == NULL) {
>> +        QTAILQ_FOREACH(opt, &opts->head, next) {
>> +            printf("%s=\"%s\" ", opt->name, opt->str);
>> +        }
>> +        return 0;
>> +    }
>> +    for (; desc && desc->name; desc++) {
>> +        const char *value = desc->def_value_str;
>> +        QemuOpt *opt;
>> +
>> +        opt = qemu_opt_find(opts, desc->name);
>> +        if (opt) {
>> +            value = opt->str;
>> +        }
>> +
>> +        if (!value) {
>> +            continue;
>> +        }
>> +
>> +        if (desc->type == QEMU_OPT_STRING) {
>> +            printf("%s='%s' ", desc->name, value);
>> +        } else {
>> +            printf("%s=%s ", desc->name, value);
>> +        }
>>       }
>> -    fprintf(stderr, "\n");
>>       return 0;
>>   }
> 
> I dislike def_value_str intensely, because it's a made up string that
> needn't be related to the actual default value.  Which is supplied by
> the code getting the option.
> 
> If I remember correctly, you want qemu_opts_print() print the default
> value, because you don't want to change output of qemu-img.  Fair
> enough.
> 
> Two sane ways to do that:
> 
> 1. Make the caller to supply the default value.  Just like the caller
>     supplies it when getting option values.
> 
>     Easiest way is to make it set the defaults in the opts whenever it
>     supplies a default.  Like this:
> 
>         mumble = qemu_opt_get(opts, "mumble");
>         if (!mumble) {
>             mumble = "mutter";
>             qemu_opt_set(opts, "mumble", mumble);
>         }
> 
>     Problem: this doesn't set the value in the existing QemuOpt, it
>     appends a new one, which isn't what we want.  We'd need a new
>     function to change the value.
> 
> 2. Actually use def_value_str as default value!  Replacing
> 
>      if (value) {
>          opt->str = g_strdup(value);
>      }
> 
>     in opt_set() by
> 
>      opt->str = g_strdup(value ?: def_value_str);
> 
>     could do.  Looks easier than 1.
> 
> 

Okay, will use 2 in new version patches.