Re: [Qemu-devel] [PATCH for-2.11] nbd/client: Use error_prepend() correctly

[Markus Armbruster]

Eric Blake  writes:

> On 11/13/2017 11:14 AM, Markus Armbruster wrote:
>> Eric Blake  writes:
>> 
>>> When using error prepend(), it is necessary to end with a space
>>> in the format string; otherwise, messages come out incorrectly,
>>> such as when connecting to a socket that hangs up immediately:
>>>
>>> can't open device nbd://localhost:10809/: Failed to read dataUnexpected 
>>> end-of-file before all bytes were read
>>>
>
>> Preexisting: inconsistent capitalization (Failed vs. failed).
>> 
>> In general, prepend chains looks slightly less ugly when each link
>> starts with a lower case letter.  Compare:
>> 
>> can't open device nbd://localhost:10809/: failed to read data: 
>> unexpected end-of-file before all bytes were read
>> Can't open device nbd://localhost:10809/: Failed to read data: 
>> Unexpected end-of-file before all bytes were read
>> 
>> Neither message is really good, but the second one is ugly to boot.
>
> A tree-wide search shows that we have no strong preference for
> capitalization or not; but I can do a followup patch for at least NBD
> code to prefer lower-case, and enforce that style in future NBD-related
> patches.  Not sure if that followup would be 2.11 material, though.

Tree-wide consistency would take consensus on the new rule, a tree-wide
patch (always a bother) to fix up the code, and a checkpatch patch to
catch regressions.  We got bigger fish to fry.

Local consistency is much easier.  Maintainer's discretion (here:
yours).

>> Reviewed-by: Markus Armbruster 
>> 
>
> Thanks

Re: [Qemu-devel] [PATCH for-2.11] nbd/client: Use error_prepend() correctly

[Eric Blake]

On 11/13/2017 09:24 AM, Eric Blake wrote:
> When using error prepend(), it is necessary to end with a space
> in the format string; otherwise, messages come out incorrectly,
> such as when connecting to a socket that hangs up immediately:
> 
> can't open device nbd://localhost:10809/: Failed to read dataUnexpected 
> end-of-file before all bytes were read
> 
> Originally botched in commit e44ed99d, then several more instances
> added in the meantime.
> 
> CC: qemu-sta...@nongnu.org
> Signed-off-by: Eric Blake 
> ---
>  nbd/client.c | 50 ++
>  1 file changed, 26 insertions(+), 24 deletions(-)

Queued on my NBD tree; will be in 2.11 (I may do another pull request
prior to -rc1, otherwise it will be -rc2).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH for-2.11] nbd/client: Use error_prepend() correctly

[Eric Blake]

On 11/13/2017 11:14 AM, Markus Armbruster wrote:
> Eric Blake  writes:
> 
>> When using error prepend(), it is necessary to end with a space
>> in the format string; otherwise, messages come out incorrectly,
>> such as when connecting to a socket that hangs up immediately:
>>
>> can't open device nbd://localhost:10809/: Failed to read dataUnexpected 
>> end-of-file before all bytes were read
>>

> Preexisting: inconsistent capitalization (Failed vs. failed).
> 
> In general, prepend chains looks slightly less ugly when each link
> starts with a lower case letter.  Compare:
> 
> can't open device nbd://localhost:10809/: failed to read data: unexpected 
> end-of-file before all bytes were read
> Can't open device nbd://localhost:10809/: Failed to read data: Unexpected 
> end-of-file before all bytes were read
> 
> Neither message is really good, but the second one is ugly to boot.

A tree-wide search shows that we have no strong preference for
capitalization or not; but I can do a followup patch for at least NBD
code to prefer lower-case, and enforce that style in future NBD-related
patches.  Not sure if that followup would be 2.11 material, though.

> 
> Reviewed-by: Markus Armbruster 
> 

Thanks

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH for-2.11] nbd/client: Use error_prepend() correctly

[Markus Armbruster]

Eric Blake  writes:

> When using error prepend(), it is necessary to end with a space
> in the format string; otherwise, messages come out incorrectly,
> such as when connecting to a socket that hangs up immediately:
>
> can't open device nbd://localhost:10809/: Failed to read dataUnexpected 
> end-of-file before all bytes were read
>
> Originally botched in commit e44ed99d, then several more instances
> added in the meantime.
>
> CC: qemu-sta...@nongnu.org
> Signed-off-by: Eric Blake 
> ---
>  nbd/client.c | 50 ++
>  1 file changed, 26 insertions(+), 24 deletions(-)
>
> diff --git a/nbd/client.c b/nbd/client.c
> index 1880103d2a..4e15fc484d 100644
> --- a/nbd/client.c
> +++ b/nbd/client.c
> @@ -79,12 +79,12 @@ static int nbd_send_option_request(QIOChannel *ioc, 
> uint32_t opt,
>  stl_be_p(, len);
>
>  if (nbd_write(ioc, , sizeof(req), errp) < 0) {
> -error_prepend(errp, "Failed to send option request header");
> +error_prepend(errp, "Failed to send option request header: ");
>  return -1;
>  }
>
>  if (len && nbd_write(ioc, (char *) data, len, errp) < 0) {
> -error_prepend(errp, "Failed to send option request data");
> +error_prepend(errp, "Failed to send option request data: ");
>  return -1;
>  }
>
> @@ -113,7 +113,7 @@ static int nbd_receive_option_reply(QIOChannel *ioc, 
> uint32_t opt,
>  {
>  QEMU_BUILD_BUG_ON(sizeof(*reply) != 20);
>  if (nbd_read(ioc, reply, sizeof(*reply), errp) < 0) {
> -error_prepend(errp, "failed to read option reply");
> +error_prepend(errp, "failed to read option reply: ");
>  nbd_send_opt_abort(ioc);
>  return -1;
>  }

Preexisting: inconsistent capitalization (Failed vs. failed).

In general, prepend chains looks slightly less ugly when each link
starts with a lower case letter.  Compare:

can't open device nbd://localhost:10809/: failed to read data: unexpected 
end-of-file before all bytes were read
Can't open device nbd://localhost:10809/: Failed to read data: Unexpected 
end-of-file before all bytes were read

Neither message is really good, but the second one is ugly to boot.

> @@ -166,7 +166,7 @@ static int nbd_handle_reply_err(QIOChannel *ioc, 
> nbd_opt_reply *reply,
>  msg = g_malloc(reply->length + 1);
>  if (nbd_read(ioc, msg, reply->length, errp) < 0) {
>  error_prepend(errp, "failed to read option error 0x%" PRIx32
> -  " (%s) message",
> +  " (%s) message: ",
>reply->type, nbd_rep_lookup(reply->type));
>  goto cleanup;
>  }
> @@ -277,7 +277,7 @@ static int nbd_receive_list(QIOChannel *ioc, const char 
> *want, bool *match,
>  return -1;
>  }
>  if (nbd_read(ioc, , sizeof(namelen), errp) < 0) {
> -error_prepend(errp, "failed to read option name length");
> +error_prepend(errp, "failed to read option name length: ");
>  nbd_send_opt_abort(ioc);
>  return -1;
>  }
> @@ -290,7 +290,8 @@ static int nbd_receive_list(QIOChannel *ioc, const char 
> *want, bool *match,
>  }
>  if (namelen != strlen(want)) {
>  if (nbd_drop(ioc, len, errp) < 0) {
> -error_prepend(errp, "failed to skip export name with wrong 
> length");
> +error_prepend(errp,
> +  "failed to skip export name with wrong length: ");
>  nbd_send_opt_abort(ioc);
>  return -1;
>  }
> @@ -299,14 +300,14 @@ static int nbd_receive_list(QIOChannel *ioc, const char 
> *want, bool *match,
>
>  assert(namelen < sizeof(name));
>  if (nbd_read(ioc, name, namelen, errp) < 0) {
> -error_prepend(errp, "failed to read export name");
> +error_prepend(errp, "failed to read export name: ");
>  nbd_send_opt_abort(ioc);
>  return -1;
>  }
>  name[namelen] = '\0';
>  len -= namelen;
>  if (nbd_drop(ioc, len, errp) < 0) {
> -error_prepend(errp, "failed to read export description");
> +error_prepend(errp, "failed to read export description: ");
>  nbd_send_opt_abort(ioc);
>  return -1;
>  }
> @@ -390,7 +391,7 @@ static int nbd_opt_go(QIOChannel *ioc, const char 
> *wantname,
>  return -1;
>  }
>  if (nbd_read(ioc, , sizeof(type), errp) < 0) {
> -error_prepend(errp, "failed to read info type");
> +error_prepend(errp, "failed to read info type: ");
>  nbd_send_opt_abort(ioc);
>  return -1;
>  }
> @@ -405,13 +406,13 @@ static int nbd_opt_go(QIOChannel *ioc, const char 
> *wantname,
>  return -1;
>  }
>  if (nbd_read(ioc, >size, sizeof(info->size), errp) < 0) {
> -error_prepend(errp, "failed to read info size");
> +

Re: [Qemu-devel] [PATCH for-2.11] nbd/client: Use error_prepend() correctly

[Markus Armbruster]

Eric Blake  writes:

> [adding Markus as error maintainer]
>
> On 11/13/2017 09:24 AM, Eric Blake wrote:
>> When using error prepend(), it is necessary to end with a space
>> in the format string; otherwise, messages come out incorrectly,
>> such as when connecting to a socket that hangs up immediately:
>> 
>> can't open device nbd://localhost:10809/: Failed to read dataUnexpected 
>> end-of-file before all bytes were read
>> 
>> Originally botched in commit e44ed99d, then several more instances
>> added in the meantime.
>>
>> CC: qemu-sta...@nongnu.org
>> Signed-off-by: Eric Blake 
>> ---
>>  nbd/client.c | 50 ++
>>  1 file changed, 26 insertions(+), 24 deletions(-)
>> 
>> diff --git a/nbd/client.c b/nbd/client.c
>> index 1880103d2a..4e15fc484d 100644
>> --- a/nbd/client.c
>> +++ b/nbd/client.c
>> @@ -79,12 +79,12 @@ static int nbd_send_option_request(QIOChannel *ioc, 
>> uint32_t opt,
>>  stl_be_p(, len);
>> 
>>  if (nbd_write(ioc, , sizeof(req), errp) < 0) {
>> -error_prepend(errp, "Failed to send option request header");
>> +error_prepend(errp, "Failed to send option request header: ");
>
> A quick grep of the tree noticed that most (all?) error_prepend()
> callers use trailing ": " in their format string.  Should we refactor
> that to be done automatically by error_prepend() itself, rather than at
> every callsite?

error_prepend() becomes less general, but perhaps a bit harder to
misuse.

When I created it, I considered having it add ": ".  I rejected that,
because adding it in the caller is not much of a burden, and I assumed
that even the most basic testing would catch mistakes.

Turns out we can't be bothered to test new errors even once.

Missing colons in error messages is the least of my worries about
untested error paths.

I'd prefer to leave error_prepend() as it is.

Re: [Qemu-devel] [PATCH for-2.11] nbd/client: Use error_prepend() correctly

[Vladimir Sementsov-Ogievskiy]

13.11.2017 18:24, Eric Blake wrote:

When using error prepend(), it is necessary to end with a space
in the format string; otherwise, messages come out incorrectly,
such as when connecting to a socket that hangs up immediately:

can't open device nbd://localhost:10809/: Failed to read dataUnexpected 
end-of-file before all bytes were read

Originally botched in commit e44ed99d, then several more instances
added in the meantime.

CC: qemu-sta...@nongnu.org
Signed-off-by: Eric Blake 
---
  nbd/client.c | 50 ++
  1 file changed, 26 insertions(+), 24 deletions(-)

diff --git a/nbd/client.c b/nbd/client.c
index 1880103d2a..4e15fc484d 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -79,12 +79,12 @@ static int nbd_send_option_request(QIOChannel *ioc, 
uint32_t opt,
  stl_be_p(, len);

  if (nbd_write(ioc, , sizeof(req), errp) < 0) {
-error_prepend(errp, "Failed to send option request header");
+error_prepend(errp, "Failed to send option request header: ");
  return -1;
  }

  if (len && nbd_write(ioc, (char *) data, len, errp) < 0) {
-error_prepend(errp, "Failed to send option request data");
+error_prepend(errp, "Failed to send option request data: ");
  return -1;
  }

@@ -113,7 +113,7 @@ static int nbd_receive_option_reply(QIOChannel *ioc, 
uint32_t opt,
  {
  QEMU_BUILD_BUG_ON(sizeof(*reply) != 20);
  if (nbd_read(ioc, reply, sizeof(*reply), errp) < 0) {
-error_prepend(errp, "failed to read option reply");
+error_prepend(errp, "failed to read option reply: ");
  nbd_send_opt_abort(ioc);
  return -1;
  }
@@ -166,7 +166,7 @@ static int nbd_handle_reply_err(QIOChannel *ioc, 
nbd_opt_reply *reply,
  msg = g_malloc(reply->length + 1);
  if (nbd_read(ioc, msg, reply->length, errp) < 0) {
  error_prepend(errp, "failed to read option error 0x%" PRIx32
-  " (%s) message",
+  " (%s) message: ",
reply->type, nbd_rep_lookup(reply->type));
  goto cleanup;
  }
@@ -277,7 +277,7 @@ static int nbd_receive_list(QIOChannel *ioc, const char 
*want, bool *match,
  return -1;
  }
  if (nbd_read(ioc, , sizeof(namelen), errp) < 0) {
-error_prepend(errp, "failed to read option name length");
+error_prepend(errp, "failed to read option name length: ");
  nbd_send_opt_abort(ioc);
  return -1;
  }
@@ -290,7 +290,8 @@ static int nbd_receive_list(QIOChannel *ioc, const char 
*want, bool *match,
  }
  if (namelen != strlen(want)) {
  if (nbd_drop(ioc, len, errp) < 0) {
-error_prepend(errp, "failed to skip export name with wrong 
length");
+error_prepend(errp,
+  "failed to skip export name with wrong length: ");
  nbd_send_opt_abort(ioc);
  return -1;
  }
@@ -299,14 +300,14 @@ static int nbd_receive_list(QIOChannel *ioc, const char 
*want, bool *match,

  assert(namelen < sizeof(name));
  if (nbd_read(ioc, name, namelen, errp) < 0) {
-error_prepend(errp, "failed to read export name");
+error_prepend(errp, "failed to read export name: ");
  nbd_send_opt_abort(ioc);
  return -1;
  }
  name[namelen] = '\0';
  len -= namelen;
  if (nbd_drop(ioc, len, errp) < 0) {
-error_prepend(errp, "failed to read export description");
+error_prepend(errp, "failed to read export description: ");
  nbd_send_opt_abort(ioc);
  return -1;
  }
@@ -390,7 +391,7 @@ static int nbd_opt_go(QIOChannel *ioc, const char *wantname,
  return -1;
  }
  if (nbd_read(ioc, , sizeof(type), errp) < 0) {
-error_prepend(errp, "failed to read info type");
+error_prepend(errp, "failed to read info type: ");
  nbd_send_opt_abort(ioc);
  return -1;
  }
@@ -405,13 +406,13 @@ static int nbd_opt_go(QIOChannel *ioc, const char 
*wantname,
  return -1;
  }
  if (nbd_read(ioc, >size, sizeof(info->size), errp) < 0) {
-error_prepend(errp, "failed to read info size");
+error_prepend(errp, "failed to read info size: ");
  nbd_send_opt_abort(ioc);
  return -1;
  }
  be64_to_cpus(>size);
  if (nbd_read(ioc, >flags, sizeof(info->flags), errp) < 0) {
-error_prepend(errp, "failed to read info flags");
+error_prepend(errp, "failed to read info flags: ");
  nbd_send_opt_abort(ioc);
  return -1;
  }
@@ -428,7 +429,7 @@ static int nbd_opt_go(QIOChannel *ioc, const char *wantname,
  }
  if (nbd_read(ioc, >min_block, sizeof(info->min_block),

Re: [Qemu-devel] [PATCH for-2.11] nbd/client: Use error_prepend() correctly

[Vladimir Sementsov-Ogievskiy]

13.11.2017 18:32, Eric Blake wrote:

[adding Markus as error maintainer]

On 11/13/2017 09:24 AM, Eric Blake wrote:

When using error prepend(), it is necessary to end with a space
in the format string; otherwise, messages come out incorrectly,
such as when connecting to a socket that hangs up immediately:

can't open device nbd://localhost:10809/: Failed to read dataUnexpected 
end-of-file before all bytes were read

Originally botched in commit e44ed99d, then several more instances
added in the meantime.

CC: qemu-sta...@nongnu.org
Signed-off-by: Eric Blake 
---
  nbd/client.c | 50 ++
  1 file changed, 26 insertions(+), 24 deletions(-)

diff --git a/nbd/client.c b/nbd/client.c
index 1880103d2a..4e15fc484d 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -79,12 +79,12 @@ static int nbd_send_option_request(QIOChannel *ioc, 
uint32_t opt,
  stl_be_p(, len);

  if (nbd_write(ioc, , sizeof(req), errp) < 0) {
-error_prepend(errp, "Failed to send option request header");
+error_prepend(errp, "Failed to send option request header: ");

A quick grep of the tree noticed that most (all?) error_prepend()
callers use trailing ": " in their format string.  Should we refactor
that to be done automatically by error_prepend() itself, rather than at
every callsite?



Sounds good.

--
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH for-2.11] nbd/client: Use error_prepend() correctly

[Eric Blake]

[adding Markus as error maintainer]

On 11/13/2017 09:24 AM, Eric Blake wrote:
> When using error prepend(), it is necessary to end with a space
> in the format string; otherwise, messages come out incorrectly,
> such as when connecting to a socket that hangs up immediately:
> 
> can't open device nbd://localhost:10809/: Failed to read dataUnexpected 
> end-of-file before all bytes were read
> 
> Originally botched in commit e44ed99d, then several more instances
> added in the meantime.
> 
> CC: qemu-sta...@nongnu.org
> Signed-off-by: Eric Blake 
> ---
>  nbd/client.c | 50 ++
>  1 file changed, 26 insertions(+), 24 deletions(-)
> 
> diff --git a/nbd/client.c b/nbd/client.c
> index 1880103d2a..4e15fc484d 100644
> --- a/nbd/client.c
> +++ b/nbd/client.c
> @@ -79,12 +79,12 @@ static int nbd_send_option_request(QIOChannel *ioc, 
> uint32_t opt,
>  stl_be_p(, len);
> 
>  if (nbd_write(ioc, , sizeof(req), errp) < 0) {
> -error_prepend(errp, "Failed to send option request header");
> +error_prepend(errp, "Failed to send option request header: ");

A quick grep of the tree noticed that most (all?) error_prepend()
callers use trailing ": " in their format string.  Should we refactor
that to be done automatically by error_prepend() itself, rather than at
every callsite?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature