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