Re: [Qemu-devel] [PATCH 02/24] QemuOpts: Assert value string isn't null

[Markus Armbruster <armbru@redhat.com>]

Eric Blake <eblake@redhat.com> writes:

> On 02/14/2017 04:25 AM, Markus Armbruster wrote:
>> Plenty of code relies on QemuOpt member @str not being null, including
>> qemu_opts_print(), qemu_opts_to_qdict(), and callbacks passed to
>> qemu_opt_foreach().
>> 
>
>> 
>> Assert member @str isn't null, so that misuse is caught right away.
>> 
>> Simplify parse_option_bool(), parse_option_number() and
>> parse_option_size() accordingly.  Best viewed with whitespace changes
>> ignored.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  util/qemu-option.c | 89 ++++++++++++++++++++++++------------------------------
>>  1 file changed, 39 insertions(+), 50 deletions(-)
>> 
>
>> @@ -180,39 +172,35 @@ void parse_option_size(const char *name, const char *value,
>>      char *postfix;
>>      double sizef;
>>  
>
>> +    sizef = strtod(value, &postfix);
>> +    if (sizef < 0 || sizef > UINT64_MAX) {
>> +        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name,
>> +                   "a non-negative number below 2^64");
>> +        return;
>> +    }
>> +    switch (*postfix) {
>> +    case 'T':
> ...
>> +    default:
>>          error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name, "a size");
>> +        error_append_hint(errp, "You may use k, M, G or T suffixes for "
>> +                          "kilobytes, megabytes, gigabytes and terabytes.\n");
>> +        return;
>>      }
>
> Unrelated to this patch, but noticing it now: it looks like we blindly
> accept "qemu-system-x86_64 -nodefaults -m 1Mgarbage" as meaning the same
> as "... -m 1M".  Looking back at 1/24, looks like you marked that as one
> of the buggy cases.

Correct.

>                      Good - I guess I'll get to comment more on it in a
> later patch.
>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!