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

[Richard Henderson]

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.

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

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


r~