Eric Blake <ebl...@redhat.com> writes:
> On 06/15/2016 11:27 AM, Markus Armbruster wrote:
>> g_error() is not an acceptable way to report errors to the user:
>>
>> $ qemu-system-x86_64 -dfilter 1000+0
>>
>> ** (process:17187): ERROR **: Failed to parse range in: 1000+0
>> Trace/breakpoint trap (core dumped)
>>
>> g_assert() isn't, either:
>>
>> $ qemu-system-x86_64 -dfilter 1000x+64
>> **
>> ERROR:/work/armbru/qemu/util/log.c:180:qemu_set_dfilter_ranges:
>> assertion failed: (e == range_op)
>> Aborted (core dumped)
>
> I see you're trying to improve my range.h patches, and got dragged into
> this stuff.
Yup, except I'd call it "build upon", not "improve".
>> Convert qemu_set_dfilter_ranges() to Error. Rework its deeply nested
>> control flow. Touch up the error messages. Call it with
>> &error_fatal.
>>
>> This also permits testing without a subprocess, so do that.
>>
>> Signed-off-by: Markus Armbruster <arm...@redhat.com>
>> ---
>> include/qemu/log.h | 2 +-
>> tests/test-logging.c | 49 ++++++++--------------
>> util/log.c | 113
>> ++++++++++++++++++++++++++-------------------------
>> vl.c | 2 +-
>> 4 files changed, 75 insertions(+), 91 deletions(-)
>>
>
>> + qemu_set_dfilter_ranges("0x1000+onehundred", &err);
>> + error_free_or_abort(&err);
>> +
>> + qemu_set_dfilter_ranges("0x1000+0", &err);
>> + error_free_or_abort(&err);
>> }
>>
>
> Maybe also worth testing "0x" and "0x1000+0x" for being invalid?
The former would add a test for the error handling of
qemu_set_dfilter_ranges()'s other use of qemu_strtoull(). If it wasn't
worth testing before my patch... But I can add a test case anyway, if I
need to respin.
The latter would basically test an additional error path within
qemu_strtoull().
>
> Reviewed-by: Eric Blake <ebl...@redhat.com>
Thanks!
