| Message ID | 20191015112346.45554-5-slp@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Introduce the microvm machine type | expand |
Hi Sergio, On 10/15/19 1:23 PM, Sergio Lopez wrote: > Follow checkpatch.pl recommendation and replace the use of strtol with > qemu_strtol in x86_load_linux(). "with qemu_strtoui" > > Signed-off-by: Sergio Lopez <slp@redhat.com> > --- > hw/i386/pc.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > index 77e86bfc3d..c8608b8007 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" > @@ -1202,6 +1203,7 @@ static void x86_load_linux(PCMachineState *pcms, > vmode = strstr(kernel_cmdline, "vga="); > if (vmode) { > unsigned int 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_strtoui(vmode, NULL, 0, &video_mode); > + if (ret != 0) { > + fprintf(stderr, "qemu: can't parse 'vga' parameter: %s\n", > + strerror(-ret)); (Cc'ing Markus/Daniel just in case) I'm wondering if using fprintf() is appropriate, thinking about instantiating a machine via libvirt, is this error reported to the user? I first thought about using error_report() instead: error_report("qemu: can't parse 'vga' parameter: %s", strerror(-ret)); But this API is meaningful when used in console/monitor. We can't get here from the monitor, so: Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> > + exit(1); > + } > } > stw_p(header + 0x1fa, video_mode); > } >
Philippe Mathieu-Daudé <philmd@redhat.com> writes: > Hi Sergio, > > On 10/15/19 1:23 PM, Sergio Lopez wrote: >> Follow checkpatch.pl recommendation and replace the use of strtol with >> qemu_strtol in x86_load_linux(). > > "with qemu_strtoui" > >> >> Signed-off-by: Sergio Lopez <slp@redhat.com> >> --- >> hw/i386/pc.c | 9 ++++++++- >> 1 file changed, 8 insertions(+), 1 deletion(-) >> >> diff --git a/hw/i386/pc.c b/hw/i386/pc.c >> index 77e86bfc3d..c8608b8007 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" >> @@ -1202,6 +1203,7 @@ static void x86_load_linux(PCMachineState *pcms, >> vmode = strstr(kernel_cmdline, "vga="); >> if (vmode) { >> unsigned int 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_strtoui(vmode, NULL, 0, &video_mode); >> + if (ret != 0) { >> + fprintf(stderr, "qemu: can't parse 'vga' parameter: %s\n", >> + strerror(-ret)); > > (Cc'ing Markus/Daniel just in case) > > I'm wondering if using fprintf() is appropriate, thinking about > instantiating a machine via libvirt, is this error reported to the > user? > > I first thought about using error_report() instead: > > error_report("qemu: can't parse 'vga' parameter: %s", > strerror(-ret)); Make that error_report("can't parse 'vga' parameter: %s", strerror(-ret)); > But this API is meaningful when used in console/monitor. We can't get > here from the monitor, True, but error_report() should be used anyway, because (1) it makes intent more obvious, and (2) it uses a uniform, featureful error format. With the proposed fprintf(), we get qemu: can't parse 'vga' parameter: Numerical result out of range With error_report(): * we report the *actual* argv[0] instead of "qemu" * we obey -msg timestamp=on * if "[PATCHv2 1/2] util/qemu-error: add guest name helper with -msg options" gets accepted, we obey -msg guest-name=on, too * we have a common way to point to the offending command line argument or configuration file line (not worth doing here) Please use error_report(). [...]
Markus Armbruster <armbru@redhat.com> writes: > Philippe Mathieu-Daudé <philmd@redhat.com> writes: > >> Hi Sergio, >> >> On 10/15/19 1:23 PM, Sergio Lopez wrote: >>> Follow checkpatch.pl recommendation and replace the use of strtol with >>> qemu_strtol in x86_load_linux(). >> >> "with qemu_strtoui" >> >>> >>> Signed-off-by: Sergio Lopez <slp@redhat.com> >>> --- >>> hw/i386/pc.c | 9 ++++++++- >>> 1 file changed, 8 insertions(+), 1 deletion(-) >>> >>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c >>> index 77e86bfc3d..c8608b8007 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" >>> @@ -1202,6 +1203,7 @@ static void x86_load_linux(PCMachineState *pcms, >>> vmode = strstr(kernel_cmdline, "vga="); >>> if (vmode) { >>> unsigned int 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_strtoui(vmode, NULL, 0, &video_mode); >>> + if (ret != 0) { >>> + fprintf(stderr, "qemu: can't parse 'vga' parameter: %s\n", >>> + strerror(-ret)); >> >> (Cc'ing Markus/Daniel just in case) >> >> I'm wondering if using fprintf() is appropriate, thinking about >> instantiating a machine via libvirt, is this error reported to the >> user? >> >> I first thought about using error_report() instead: >> >> error_report("qemu: can't parse 'vga' parameter: %s", >> strerror(-ret)); > > Make that > > error_report("can't parse 'vga' parameter: %s", strerror(-ret)); > >> But this API is meaningful when used in console/monitor. We can't get >> here from the monitor, > > True, but error_report() should be used anyway, because (1) it makes > intent more obvious, and (2) it uses a uniform, featureful error format. > > With the proposed fprintf(), we get > > qemu: can't parse 'vga' parameter: Numerical result out of range > > With error_report(): > > * we report the *actual* argv[0] instead of "qemu" > > * we obey -msg timestamp=on > > * if "[PATCHv2 1/2] util/qemu-error: add guest name helper with -msg > options" gets accepted, we obey -msg guest-name=on, too > > * we have a common way to point to the offending command line argument > or configuration file line (not worth doing here) > > Please use error_report(). > > [...] But should we use error_report even if other occurrences in the same function are using fprintf? Or are you suggesting to change those too? If so, is it really worth it doing it now or can we do that in a future patch (seems completely unrelated to this patch series)? Thanks, Sergio.
Sergio Lopez <slp@redhat.com> writes: > Markus Armbruster <armbru@redhat.com> writes: > >> Philippe Mathieu-Daudé <philmd@redhat.com> writes: >> >>> Hi Sergio, >>> >>> On 10/15/19 1:23 PM, Sergio Lopez wrote: >>>> Follow checkpatch.pl recommendation and replace the use of strtol with >>>> qemu_strtol in x86_load_linux(). >>> >>> "with qemu_strtoui" >>> >>>> >>>> Signed-off-by: Sergio Lopez <slp@redhat.com> >>>> --- >>>> hw/i386/pc.c | 9 ++++++++- >>>> 1 file changed, 8 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c >>>> index 77e86bfc3d..c8608b8007 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" >>>> @@ -1202,6 +1203,7 @@ static void x86_load_linux(PCMachineState *pcms, >>>> vmode = strstr(kernel_cmdline, "vga="); >>>> if (vmode) { >>>> unsigned int 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_strtoui(vmode, NULL, 0, &video_mode); >>>> + if (ret != 0) { >>>> + fprintf(stderr, "qemu: can't parse 'vga' parameter: %s\n", >>>> + strerror(-ret)); >>> >>> (Cc'ing Markus/Daniel just in case) >>> >>> I'm wondering if using fprintf() is appropriate, thinking about >>> instantiating a machine via libvirt, is this error reported to the >>> user? >>> >>> I first thought about using error_report() instead: >>> >>> error_report("qemu: can't parse 'vga' parameter: %s", >>> strerror(-ret)); >> >> Make that >> >> error_report("can't parse 'vga' parameter: %s", strerror(-ret)); >> >>> But this API is meaningful when used in console/monitor. We can't get >>> here from the monitor, >> >> True, but error_report() should be used anyway, because (1) it makes >> intent more obvious, and (2) it uses a uniform, featureful error format. >> >> With the proposed fprintf(), we get >> >> qemu: can't parse 'vga' parameter: Numerical result out of range >> >> With error_report(): >> >> * we report the *actual* argv[0] instead of "qemu" >> >> * we obey -msg timestamp=on >> >> * if "[PATCHv2 1/2] util/qemu-error: add guest name helper with -msg >> options" gets accepted, we obey -msg guest-name=on, too >> >> * we have a common way to point to the offending command line argument >> or configuration file line (not worth doing here) >> >> Please use error_report(). >> >> [...] > > But should we use error_report even if other occurrences in the same > function are using fprintf? Or are you suggesting to change those too? Change those, too. > If so, is it really worth it doing it now or can we do that in a future > patch (seems completely unrelated to this patch series)? As long as it gets done, which patch series does it is unimportant.
diff --git a/hw/i386/pc.c b/hw/i386/pc.c index 77e86bfc3d..c8608b8007 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" @@ -1202,6 +1203,7 @@ static void x86_load_linux(PCMachineState *pcms, vmode = strstr(kernel_cmdline, "vga="); if (vmode) { unsigned int 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_strtoui(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 | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)