From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45755) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aTYl3-0001se-Od for qemu-devel@nongnu.org; Wed, 10 Feb 2016 12:40:54 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aTYky-0004rj-Vd for qemu-devel@nongnu.org; Wed, 10 Feb 2016 12:40:53 -0500 Received: from mail-wm0-x233.google.com ([2a00:1450:400c:c09::233]:34615) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aTYky-0004qS-IP for qemu-devel@nongnu.org; Wed, 10 Feb 2016 12:40:48 -0500 Received: by mail-wm0-x233.google.com with SMTP id 128so36838512wmz.1 for ; Wed, 10 Feb 2016 09:40:48 -0800 (PST) References: <1454597781-18115-1-git-send-email-alex.bennee@linaro.org> <1454597781-18115-7-git-send-email-alex.bennee@linaro.org> <56B3D9D6.2040600@twiddle.net> From: Alex =?utf-8?Q?Benn=C3=A9e?= In-reply-to: <56B3D9D6.2040600@twiddle.net> Date: Wed, 10 Feb 2016 17:40:45 +0000 Message-ID: <87wpqcv4yq.fsf@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH v5 6/9] qemu-log: new option -dfilter to limit output List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Richard Henderson Cc: peter.maydell@linaro.org, qemu-devel@nongnu.org, dgilbert@redhat.com, crosthwaitepeter@gmail.com, pbonzini@redhat.com, aurelien@aurel32.net Richard Henderson 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