Eric Blake <ebl...@redhat.com> 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 <arm...@redhat.com> >> --- >> 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 <ebl...@redhat.com> Thanks!