[libvirt] [PATCH v2 2/3] qemu: Add support for reboot-timeout

[Martin Kletzander]

On 09/20/2012 03:32 PM, Peter Krempa wrote:
> On 09/20/12 15:15, Martin Kletzander wrote:
>> On 09/20/2012 12:22 PM, Peter Krempa wrote:
>>>> @@ -8271,6 +8286,19 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr
>>>> caps,
>>>>                            qemuParseCommandLineBootDevs(def, token);
>>>>                        } else if (STRPREFIX(token, "menu=on")) {
>>>>                            def->os.bootmenu = 1;
>>>> +                    } else if (STRPREFIX(token, "reboot-timeout=")) {
>>>> +                        int num;
>>>> +                        char *endptr = strchr(token, ',');
>>>> +                        if (virStrToLong_i(token +
>>>> strlen("reboot-timeout="),
>>>> +                                           &endptr, 0, &num) < 0) {
>>>
>>> This doesn't seem ok. You assign endptr somewhere into the string and
>>> then virStrToLong_i rewrites it, but you never check the return value. I
>>> suppose you wanted to check if after the number is converted the next
>>> char is a comma. You need to do the check after virStrToLong_i.
>>>
>>
>> Yes, sorry for that, you're absolutely right. To be sure, is this
>> alright?
>>
>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>> index f8012ec..4821910 100644
>> --- a/src/qemu/qemu_command.c
>> +++ b/src/qemu/qemu_command.c
>> @@ -8271,6 +8286,20 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr
>> caps,
>>                           qemuParseCommandLineBootDevs(def, token);
>>                       } else if (STRPREFIX(token, "menu=on")) {
>>                           def->os.bootmenu = 1;
>> +                    } else if (STRPREFIX(token, "reboot-timeout=")) {
>> +                        int num;
>> +                        char *endptr;
>> +                        if (virStrToLong_i(token +
>> strlen("reboot-timeout="),
>> +                                           &endptr, 10, &num) < 0) ||
>> +                            endptr != strchr(token, ',') {
>> +                            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> +                                           _("cannot parse
>> reboot-timeout value"));
>> +                            goto error;
>> +                        }
>> +                        if (num > 65535)
>> +                            num = 65535;
> 
> You should probably check for the lower bound too here.
> 
>> +                        def->os.bios.rt_delay = num;
>> +                        def->os.bios.rt_set = true;
>>                       }
>>                       token = strchr(token, ',');
>>                       /* This incrementation has to be done here in
>> order to make it
> 
> ACK to this with the lower bound check.
> 
> Peter
> 
> 

Thanks, fixed that and one more thing (the endptr may point to end of
the string in which case that makes it valid, "thanks tests"), double
checked and pushed.

Martin