Re: [Qemu-devel] [PATCH v22 05/25] add some QemuOpts functions for replace work

[Eric Blake]

On 03/10/2014 11:29 PM, Chunyan Liu wrote:
>>
>> Why are your later callers using a common helper function, but
>> qemu_opt_get_del() repeating a lot of the body of qemu_opt_get()?
>> Shouldn't qemu_opt_get() and qemu_opt_get_del() call into a common helper?
>>
>> qemu_opt_get_del must return (char *), but the existing qemu_opt_get
> returns
> (const char *). Could also change qemu_opt_get return value to (char *)
> type,
> then these two functions could use a common helper function. But there are
> too
> many places using qemu_opt_get already, if the return value type changes,
> it will
> affect a lot.

Going from 'char *' to 'const char *' is automatic.  Have your helper
return 'char *', and qemu_opt_get can call it just fine and give its
callers their desired const char *.

>>
>> Same problem as in 4/25 - I don't think you can rely on
>> opt->value.boolean being valid if you don't track the union
>> discriminator, and right now, the union discriminator is tracked only
>> via opt->desc.
> 
> 
> Couldn't find a reliable way if the option is passed through
> opts_accept_any,
> just no way to check the option type.
> 

Like I said in 4/25, if the option is passed through opts_accept_any,
treat it as string only; and make the opt_get_bool function either
reject it outright, or to always do the string-to-bool conversion at the
time of the lookup:

if (opt->desc) {
    assert(opt->desc->type == QEMU_OPT_BOOL);
    return opt->value.boolean;
} else {
    code to parse opt->str
}


>>
> Could be if changing qemu_opt_get return value type, but just as said
> before,
> that will affect many codes.

Also, changing an existing function that returns 'const char *' into now
returning 'char *' will NOT break any callers if the semantics remain
unchanged (what WILL break is if the semantics change where the callers
must now free the result).  But in general, it should still be just fine
to have qemu_opt_get return 'const char *' even if the common helper
returns 'char *'.

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

signature.asc
Description: OpenPGP digital signature