Re: [Qemu-devel] [PATCH v5 6/9] qemu-log: new option -dfilter to limit output

["Alex Bennée" <alex.bennee@linaro.org>]

Richard Henderson <rth@twiddle.net> writes:

> On 02/11/2016 04:40 AM, Alex Bennée wrote:
>> OK I think this version is a lot cleaner:
>>
>>    void qemu_set_dfilter_ranges(const char *filter_spec)
>>    {
>>        gchar **ranges = g_strsplit(filter_spec, ",", 0);
>>        if (ranges) {
>>            gchar **next = ranges;
>>            gchar *r = *next++;
>>            debug_regions = g_array_sized_new(FALSE, FALSE,
>>                                              sizeof(Range), g_strv_length(ranges));
>>            while (r) {
>>                gchar *range_op = g_strstr_len(r, -1, "-");
>>                gchar *r2 = range_op ? range_op + 1 : NULL;
>>                if (!range_op) {
>>                    range_op = g_strstr_len(r, -1, "+");
>>                    r2 = range_op ? range_op + 1 : NULL;
>>                }
>>                if (!range_op) {
>>                    range_op = g_strstr_len(r, -1, "..");
>>                    r2 = range_op ? range_op + 2 : NULL;
>>                }
>
> I guess I'll quit quibbling about silly glib functions.  But really, with the
> -1 argument, you gain nothing except obfuscation over using the
> standard C library.

No you are quite right to quibble. It's a hard habit to break because
I've gotten used to glib's arguably more predictable behaviour when
string munging.

>
>>                if (range_op) {
>>                    struct Range range;
>>                    int err;
>>                    const char *e = NULL;
>>
>>                    err = qemu_strtoull(r, &e, 0, &range.begin);
>>
>>                    g_assert(e == range_op);
>>
>>                    switch (*range_op) {
>>                    case '+':
>>                    {
>>                        unsigned long len;
>>                        err |= qemu_strtoull(r2, NULL, 0, &len);
>
> You can't or errno's together and then...
>
>>                        g_error("Failed to parse range in: %s, %d", r, err);
>
> ... expect to get anything meaningful out of them.

True, I'll drop the %d, I was just trying to avoid having multiple error
handling legs.

>
>>>> +                case '+':
>>>> +                {
>>>> +                    unsigned long len;
>>>> +                    err |= qemu_strtoul(range_val, NULL, 0, &len);
>>>> +                    range.end = range.begin + len;
>>>> +                    break;
>>>> +                }
>>>> +                case '-':
>>>> +                {
>>>> +                    unsigned long len;
>>>> +                    err |= qemu_strtoul(range_val, NULL, 0, &len);
>>>> +                    range.end = range.begin;
>>>> +                    range.begin = range.end - len;
>>>> +                    break;
>>>> +                }
>>>
>>> Both of these have off-by-one bugs, since end is inclusive.
>>
>> Sorry I don't quite follow, do you mean the position of range_val (now
>> r2) or the final value of range.end?
>
> Final value of range.end.  In that
>
>     0x1000..0x1000
> and
>     0x1000+1
>
> should both produce a range that covers a single byte at 0x1000.

Ahh OK. I suppose if I'm being good about this I should add some tests
to defend the ranges. I wonder how easy command line parsing unit tests
are in qtest?

>
>
> r~


--
Alex Bennée