Re: [Qemu-devel] [PATCH v2 01/23] qemu-nbd: Replace BSDism <err.h> by error_report()

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 5669 bytes --]

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 <armbru@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.

> @@ -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 <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]