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

[Sergio Lopez]

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?
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.

signature.asc
Description: PGP signature