From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45098) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a9dfh-0008O7-M5 for qemu-devel@nongnu.org; Thu, 17 Dec 2015 13:53:02 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a9dfe-0007pz-Bf for qemu-devel@nongnu.org; Thu, 17 Dec 2015 13:53:01 -0500 Received: from mx1.redhat.com ([209.132.183.28]:33207) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a9dfe-0007pg-45 for qemu-devel@nongnu.org; Thu, 17 Dec 2015 13:52:58 -0500 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (Postfix) with ESMTPS id 9299C8E901 for ; Thu, 17 Dec 2015 18:52:57 +0000 (UTC) From: Markus Armbruster References: <1450371004-26866-1-git-send-email-armbru@redhat.com> <1450371004-26866-2-git-send-email-armbru@redhat.com> <5672EE3D.3080901@redhat.com> Date: Thu, 17 Dec 2015 19:52:55 +0100 In-Reply-To: <5672EE3D.3080901@redhat.com> (Eric Blake's message of "Thu, 17 Dec 2015 10:17:49 -0700") Message-ID: <87y4cssy9k.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH v2 01/23] qemu-nbd: Replace BSDism by error_report() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: qemu-devel@nongnu.org Eric Blake writes: > On 12/17/2015 09:49 AM, Markus Armbruster wrote: >> Coccinelle semantic patch >> >> @@ >> expression E; >> expression list ARGS; >> @@ >> - errx(E, ARGS); >> + error_report(ARGS); >> + exit(E); >> @@ >> expression E, FMT; >> expression list ARGS; >> @@ >> - err(E, FMT, ARGS); >> + error_report(FMT /*": %s"*/, ARGS, strerror(errno)); >> + exit(E); >> >> followed by a replace of '"/*": %s"*/' by ' : %s"'. > > Interesting approach to working around a coccinelle shortcoming. > > Hmm. Should we add error_report_errno(), to parallel error_setg_errno()? > But that can be a separate patch. If we have enough places that profit from it. >> Signed-off-by: Markus Armbruster >> --- >> qemu-nbd.c | 119 ++++++++++++++++++++++++++++++++++++++----------------------- >> 1 file changed, 75 insertions(+), 44 deletions(-) >> > >> @@ -491,13 +498,14 @@ int main(int argc, char **argv) >> BLOCKDEV_DETECT_ZEROES_OPTIONS_OFF, >> &local_err); >> if (local_err) { >> - errx(EXIT_FAILURE, "Failed to parse detect_zeroes mode: %s", >> - error_get_pretty(local_err)); >> + error_report("Failed to parse detect_zeroes mode: %s", >> + error_get_pretty(local_err)); >> + exit(EXIT_FAILURE); >> } >> if (detect_zeroes == BLOCKDEV_DETECT_ZEROES_OPTIONS_UNMAP && >> !(flags & BDRV_O_UNMAP)) { >> - errx(EXIT_FAILURE, "setting detect-zeroes to unmap is not allowed " >> - "without setting discard operation to unmap"); >> + error_report("setting detect-zeroes to unmap is not allowed " "without setting discard operation to unmap"); > > You'll want to fix the line-wrap on this. Yes. Coccinelle has difficulties with string literal concatenation. >> @@ -509,10 +517,12 @@ int main(int argc, char **argv) >> case 'o': >> dev_offset = strtoll (optarg, &end, 0); >> if (*end) { >> - errx(EXIT_FAILURE, "Invalid offset `%s'", optarg); >> + error_report("Invalid offset `%s'", optarg); > > Worth cleaning this up to use '' instead of `' at some point in the > series? (Not here, though, since this patch is best when it sticks as > close to the coccinelle script as possible). I tend to clean it up when I touch the message anyway. Perhaps wholesale cleanup would be in order, but not in this series. >> + exit(EXIT_FAILURE); >> } >> if (dev_offset < 0) { >> - errx(EXIT_FAILURE, "Offset must be positive `%s'", optarg); >> + error_report("Offset must be positive `%s'", optarg); > > Obviously, any `' cleanup to '' will have quite a few callers to adjust. Quick grep finds about a hundred. >> @@ -534,16 +545,19 @@ int main(int argc, char **argv) >> case 'P': >> partition = strtol(optarg, &end, 0); >> if (*end) { >> - errx(EXIT_FAILURE, "Invalid partition `%s'", optarg); >> + error_report("Invalid partition `%s'", optarg); >> + exit(EXIT_FAILURE); >> } >> if (partition < 1 || partition > 8) { >> - errx(EXIT_FAILURE, "Invalid partition %d", partition); >> + error_report("Invalid partition %d", partition); >> + exit(EXIT_FAILURE); >> } >> break; >> case 'k': >> sockpath = optarg; >> if (sockpath[0] != '/') { >> - errx(EXIT_FAILURE, "socket path must be absolute\n"); >> + error_report("socket path must be absolute\n"); > > I'm guessing later in the series will kill the trailing newline? If so, > then this patch is fine (again, limiting to just coccinelle here). It's a mistake-preserving patch :) It should be killed in PATCH 21, but isn't, because I forgot to run Coccinelle again after rebasing v1 onto the patches new in v2. I'll fix PATCH 21. >> + exit(EXIT_FAILURE); >> } >> break; >> case 'd': >> @@ -555,10 +569,12 @@ int main(int argc, char **argv) >> case 'e': >> shared = strtol(optarg, &end, 0); >> if (*end) { >> - errx(EXIT_FAILURE, "Invalid shared device number '%s'", optarg); >> + error_report("Invalid shared device number '%s'", optarg); >> + exit(EXIT_FAILURE); >> } >> if (shared < 1) { >> - errx(EXIT_FAILURE, "Shared device number must be greater than 0\n"); >> + error_report("Shared device number must be greater than 0\n"); >> + exit(EXIT_FAILURE); > > And another one. Maybe mention in the commit message any future > cleanups planned for style issues that are exposed by this conversion? What about: A few of the error messages touched have trailing newlines. They'll be stripped later in this series. >> } >> break; >> case 'f': >> @@ -579,21 +595,23 @@ int main(int argc, char **argv) >> exit(0); >> break; >> case '?': >> - errx(EXIT_FAILURE, "Try `%s --help' for more information.", >> - argv[0]); >> + error_report("Try `%s --help' for more information.", argv[0]); >> + exit(EXIT_FAILURE); >> } >> } >> >> if ((argc - optind) != 1) { >> - errx(EXIT_FAILURE, "Invalid number of argument.\n" >> - "Try `%s --help' for more information.", >> - argv[0]); >> + error_report("Invalid number of argument.\n" "Try `%s --help' for more information.", >> + argv[0]); > > Long source line worth wrapping? > > Line wraps and commit message improvements seem obvious, so I'm okay > with adding: > Reviewed-by: Eric Blake Thanks!