| Message ID | 20191010143125.67246-5-slp@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v8,01/15] hw/virtio: Factorize virtio-mmio headers | expand |
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? > + 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); > } >
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. >> + 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); >> } >>
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. 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); >>> } >>>
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); >>>> } >>>>
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; + 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); }
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(-)