[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 01/23] qemu-nbd: Replace BSDism <err.h> by er
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH v2 01/23] qemu-nbd: Replace BSDism <err.h> by error_report() |
Date: |
Thu, 17 Dec 2015 19:52:55 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
Eric Blake <address@hidden> 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 <address@hidden>
>> ---
>> 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 <address@hidden>
Thanks!