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~

Reply via email to