Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> On 10/11/19 9:26 AM, Sergio Lopez wrote:
>> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
>>
>>> Hi Sergio,
>>>
>>> On 10/10/19 4:31 PM, Sergio Lopez wrote:
>>>> Follow checkpatch.pl recommendation and replace the use of strtol with
>>>> qemu_strtol in x86_load_linux().
>>>>
>>>> Signed-off-by: Sergio Lopez <slp@redhat.com>
>>>> ---
>>>>    hw/i386/pc.c | 11 +++++++++--
>>>>    1 file changed, 9 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>>>> index 77e86bfc3d..e6bcc3ff42 100644
>>>> --- a/hw/i386/pc.c
>>>> +++ b/hw/i386/pc.c
>>>> @@ -68,6 +68,7 @@
>>>>    #include "qemu/config-file.h"
>>>>    #include "qemu/error-report.h"
>>>>    #include "qemu/option.h"
>>>> +#include "qemu/cutils.h"
>>>>    #include "hw/acpi/acpi.h"
>>>>    #include "hw/acpi/cpu_hotplug.h"
>>>>    #include "hw/boards.h"
>>>> @@ -1201,7 +1202,8 @@ static void x86_load_linux(PCMachineState *pcms,
>>>>        /* handle vga= parameter */
>>>>        vmode = strstr(kernel_cmdline, "vga=");
>>>>        if (vmode) {
>>>> -        unsigned int video_mode;
>>>> +        long video_mode;
>>>
>>> Why do you change 'video_mode' to a signed type?
>>
>> qemu_strtol fourth argument is a pointer to long int. According to
>> "linux/Documentation/admin-guide/svga.rst", valid video modes are in the
>> in the range of 0x0 to 0xffff (matching the stw_p below), so this change
>> shouldn't be a problem.
>
> Why not simply use qemu_strtoui() then? Later stw_p() implicitly cast
> this to uint16_t anyway.

I don't think there's a significant difference in functionality, but
that would probably look better. I'll prepare a v9.

Thanks!
Sergio.

> Any thought from other reviewers? Do we care? I'm feeling being a pain
> with Sergio :/
>
>>>> +        int ret;
>>>>            /* skip "vga=" */
>>>>            vmode += 4;
>>>>            if (!strncmp(vmode, "normal", 6)) {
>>>> @@ -1211,7 +1213,12 @@ static void x86_load_linux(PCMachineState *pcms,
>>>>            } else if (!strncmp(vmode, "ask", 3)) {
>>>>                video_mode = 0xfffd;
>>>>            } else {
>>>> -            video_mode = strtol(vmode, NULL, 0);
>>>> +            ret = qemu_strtol(vmode, NULL, 0, &video_mode);
>>>> +            if (ret != 0) {
>>>> +                fprintf(stderr, "qemu: can't parse 'vga' parameter: %s\n",
>>>> +                        strerror(-ret));
>>>> +                exit(1);
>>>> +            }
>>>>            }
>>>>            stw_p(header + 0x1fa, video_mode);
>>>>        }
>>>>