qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v9 04/15] hw/i386/pc: replace use of strtol with qemu_strtol


From: Markus Armbruster
Subject: Re: [PATCH v9 04/15] hw/i386/pc: replace use of strtol with qemu_strtol in x86_load_linux()
Date: Fri, 18 Oct 2019 07:05:11 +0200
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/26.2 (gnu/linux)

Sergio Lopez <address@hidden> writes:

> Markus Armbruster <address@hidden> writes:
>
>> Philippe Mathieu-Daudé <address@hidden> 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 <address@hidden>
>>>> ---
>>>>   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.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]