Richard Henderson <r...@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