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

[Markus Armbruster]

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!