From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36475) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aTZc9-00036t-OL for qemu-devel@nongnu.org; Wed, 10 Feb 2016 13:35:46 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aTZc5-0003pm-NO for qemu-devel@nongnu.org; Wed, 10 Feb 2016 13:35:45 -0500 Received: from mail-wm0-x231.google.com ([2a00:1450:400c:c09::231]:35914) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aTZc5-0003pT-6h for qemu-devel@nongnu.org; Wed, 10 Feb 2016 13:35:41 -0500 Received: by mail-wm0-x231.google.com with SMTP id p63so41935677wmp.1 for ; Wed, 10 Feb 2016 10:35:41 -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> <87wpqcv4yq.fsf@linaro.org> <56BB7A57.6050502@twiddle.net> From: Alex =?utf-8?Q?Benn=C3=A9e?= In-reply-to: <56BB7A57.6050502@twiddle.net> Date: Wed, 10 Feb 2016 18:35:38 +0000 Message-ID: <87vb5wv2f9.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/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