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.