[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v5 6/9] qemu-log: new option -dfilter to limit o
From: |
Alex Bennée |
Subject: |
Re: [Qemu-devel] [PATCH v5 6/9] qemu-log: new option -dfilter to limit output |
Date: |
Wed, 10 Feb 2016 18:35:38 +0000 |
User-agent: |
mu4e 0.9.17; emacs 25.0.90.4 |
Richard Henderson <address@hidden> 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
[Qemu-devel] [PATCH v5 7/9] qemu-log: dfilter-ise exec, out_asm, op and opt_op, Alex Bennée, 2016/02/04
[Qemu-devel] [PATCH v5 9/9] cputlb: modernise the debug support, Alex Bennée, 2016/02/04
[Qemu-devel] [PATCH v5 8/9] target-arm: dfilter support for in_asm, Alex Bennée, 2016/02/04