[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: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH v2 01/23] qemu-nbd: Replace BSDism <err.h> by error_report() |
Date: |
Thu, 17 Dec 2015 10:17:49 -0700 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 |
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.
>
> 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.
> @@ -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).
> + 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.
> @@ -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).
> + 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?
> }
> 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>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
- Re: [Qemu-devel] [PATCH v2 23/23] s390/sclp: Simplify control flow in sclp_realize(), (continued)
- [Qemu-devel] [PATCH v2 13/23] spapr: Use error_reportf_err(), Markus Armbruster, 2015/12/17
- [Qemu-devel] [PATCH v2 09/23] error: New error_prepend(), error_reportf_err(), Markus Armbruster, 2015/12/17
- [Qemu-devel] [PATCH v2 04/23] error: Use error_report_err() instead of ad hoc prints, Markus Armbruster, 2015/12/17
- [Qemu-devel] [PATCH v2 01/23] qemu-nbd: Replace BSDism <err.h> by error_report(), Markus Armbruster, 2015/12/17
- Re: [Qemu-devel] [PATCH v2 01/23] qemu-nbd: Replace BSDism <err.h> by error_report(),
Eric Blake <=
- [Qemu-devel] [PATCH v2 21/23] error: Clean up errors with embedded newlines (again), Markus Armbruster, 2015/12/17
- [Qemu-devel] [PATCH v2 18/23] vmdk: Clean up "Invalid extent lines" error message, Markus Armbruster, 2015/12/17
- [Qemu-devel] [PATCH v2 22/23] hw/s390x: Rename local variables Error *l_err to just err, Markus Armbruster, 2015/12/17