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

[Alex Bennée]

Richard Henderson <address@hidden> writes:

> On 02/05/2016 01:56 AM, Alex Bennée wrote:
>> +            gchar *range_op = g_strstr_len(r, -1, "-");
>
> This is strchr.
>
>> +                range_op = g_strstr_len(r, -1, ".");
>
> Or at least if you're going to make use of strstr, search for "..".
>
>> +                g_strdelimit(range_val, ".", ' ');
>
> Cause this is just weird.  It accepts "1+.2" just the same as "1..2".
>
>> +                err = qemu_strtoul(r, NULL, 0, &range.begin);
>
> This should be strtoull.  You probably broke 32-bit compilation here.

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;
              }
              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);
                      range.end = range.begin + len;
                      break;
                  }
                  case '-':
                  {
                      unsigned long len;
                      err |= qemu_strtoull(r2, NULL, 0, &len);
                      range.end = range.begin;
                      range.begin = range.end - len;
                      break;
                  }
                  case '.':
                      err |= qemu_strtoull(r2, NULL, 0, &range.end);
                      break;
                  default:
                      g_assert_not_reached();
                  }
                  if (err) {
                      g_error("Failed to parse range in: %s, %d", r, err);
                  } else {
                      g_array_append_val(debug_regions, range);
                  }
              } else {
                  g_error("Bad range specifier in: %s", r);
              }
              r = *next++;
          }
          g_strfreev(ranges);
      }
  }


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

>
>> +                case '.':
>> +                    err |= qemu_strtoul(range_val, NULL, 0, &range.end);
>> +                    break;
>
> I'd think multiple dot detection belongs here, and you need not smash them to 
> '
> ' but merely notice that there are two of them and then strtoull
> range_val+1.

I think this is covered with the new code now.

>
>
> r~


--
Alex Bennée