* [Qemu-devel] [PATCH for-2.11] nbd/client: Use error_prepend() correctly
@ 2017-11-13 15:24 Eric Blake
2017-11-13 15:32 ` Eric Blake
` (3 more replies)
0 siblings, 4 replies; 9+ messages in thread
From: Eric Blake @ 2017-11-13 15:24 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-block, vsementsov, qemu-stable, Paolo Bonzini
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-stable@nongnu.org
Signed-off-by: Eric Blake <eblake@redhat.com>
---
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(&req.length, len);
if (nbd_write(ioc, &req, 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, &namelen, 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, &type, 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, &info->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(&info->size);
if (nbd_read(ioc, &info->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, &info->min_block, sizeof(info->min_block),
errp) < 0) {
- error_prepend(errp, "failed to read info minimum block size");
+ error_prepend(errp, "failed to read info minimum block size: ");
nbd_send_opt_abort(ioc);
return -1;
}
@@ -441,7 +442,8 @@ static int nbd_opt_go(QIOChannel *ioc, const char *wantname,
}
if (nbd_read(ioc, &info->opt_block, sizeof(info->opt_block),
errp) < 0) {
- error_prepend(errp, "failed to read info preferred block size");
+ error_prepend(errp,
+ "failed to read info preferred block size: ");
nbd_send_opt_abort(ioc);
return -1;
}
@@ -455,7 +457,7 @@ static int nbd_opt_go(QIOChannel *ioc, const char *wantname,
}
if (nbd_read(ioc, &info->max_block, sizeof(info->max_block),
errp) < 0) {
- error_prepend(errp, "failed to read info maximum block size");
+ error_prepend(errp, "failed to read info maximum block size: ");
nbd_send_opt_abort(ioc);
return -1;
}
@@ -467,7 +469,7 @@ static int nbd_opt_go(QIOChannel *ioc, const char *wantname,
default:
trace_nbd_opt_go_info_unknown(type, nbd_info_lookup(type));
if (nbd_drop(ioc, len, errp) < 0) {
- error_prepend(errp, "Failed to read info payload");
+ error_prepend(errp, "Failed to read info payload: ");
nbd_send_opt_abort(ioc);
return -1;
}
@@ -618,7 +620,7 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name,
}
if (nbd_read(ioc, buf, 8, errp) < 0) {
- error_prepend(errp, "Failed to read data");
+ error_prepend(errp, "Failed to read data: ");
goto fail;
}
@@ -637,7 +639,7 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name,
}
if (nbd_read(ioc, &magic, sizeof(magic), errp) < 0) {
- error_prepend(errp, "Failed to read magic");
+ error_prepend(errp, "Failed to read magic: ");
goto fail;
}
magic = be64_to_cpu(magic);
@@ -649,7 +651,7 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name,
bool fixedNewStyle = false;
if (nbd_read(ioc, &globalflags, sizeof(globalflags), errp) < 0) {
- error_prepend(errp, "Failed to read server flags");
+ error_prepend(errp, "Failed to read server flags: ");
goto fail;
}
globalflags = be16_to_cpu(globalflags);
@@ -665,7 +667,7 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name,
/* client requested flags */
clientflags = cpu_to_be32(clientflags);
if (nbd_write(ioc, &clientflags, sizeof(clientflags), errp) < 0) {
- error_prepend(errp, "Failed to send clientflags field");
+ error_prepend(errp, "Failed to send clientflags field: ");
goto fail;
}
if (tlscreds) {
@@ -727,13 +729,13 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name,
/* Read the response */
if (nbd_read(ioc, &info->size, sizeof(info->size), errp) < 0) {
- error_prepend(errp, "Failed to read export length");
+ error_prepend(errp, "Failed to read export length: ");
goto fail;
}
be64_to_cpus(&info->size);
if (nbd_read(ioc, &info->flags, sizeof(info->flags), errp) < 0) {
- error_prepend(errp, "Failed to read export flags");
+ error_prepend(errp, "Failed to read export flags: ");
goto fail;
}
be16_to_cpus(&info->flags);
@@ -750,13 +752,13 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name,
}
if (nbd_read(ioc, &info->size, sizeof(info->size), errp) < 0) {
- error_prepend(errp, "Failed to read export length");
+ error_prepend(errp, "Failed to read export length: ");
goto fail;
}
be64_to_cpus(&info->size);
if (nbd_read(ioc, &oldflags, sizeof(oldflags), errp) < 0) {
- error_prepend(errp, "Failed to read export flags");
+ error_prepend(errp, "Failed to read export flags: ");
goto fail;
}
be32_to_cpus(&oldflags);
@@ -772,7 +774,7 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name,
trace_nbd_receive_negotiate_size_flags(info->size, info->flags);
if (zeroes && nbd_drop(ioc, 124, errp) < 0) {
- error_prepend(errp, "Failed to read reserved block");
+ error_prepend(errp, "Failed to read reserved block: ");
goto fail;
}
rc = 0;
--
2.13.6
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.11] nbd/client: Use error_prepend() correctly
2017-11-13 15:24 [Qemu-devel] [PATCH for-2.11] nbd/client: Use error_prepend() correctly Eric Blake
@ 2017-11-13 15:32 ` Eric Blake
2017-11-13 16:23 ` Vladimir Sementsov-Ogievskiy
2017-11-13 17:10 ` Markus Armbruster
2017-11-13 16:27 ` Vladimir Sementsov-Ogievskiy
` (2 subsequent siblings)
3 siblings, 2 replies; 9+ messages in thread
From: Eric Blake @ 2017-11-13 15:32 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, vsementsov, qemu-stable, qemu-block, Markus Armbruster
[-- Attachment #1: Type: text/plain, Size: 1546 bytes --]
[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-stable@nongnu.org
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
> 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(&req.length, len);
>
> if (nbd_write(ioc, &req, 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
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.11] nbd/client: Use error_prepend() correctly
2017-11-13 15:32 ` Eric Blake
@ 2017-11-13 16:23 ` Vladimir Sementsov-Ogievskiy
2017-11-13 17:10 ` Markus Armbruster
1 sibling, 0 replies; 9+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2017-11-13 16:23 UTC (permalink / raw)
To: Eric Blake, qemu-devel
Cc: Paolo Bonzini, qemu-stable, qemu-block, Markus Armbruster
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-stable@nongnu.org
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> ---
>> 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(&req.length, len);
>>
>> if (nbd_write(ioc, &req, 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.11] nbd/client: Use error_prepend() correctly
2017-11-13 15:24 [Qemu-devel] [PATCH for-2.11] nbd/client: Use error_prepend() correctly Eric Blake
2017-11-13 15:32 ` Eric Blake
@ 2017-11-13 16:27 ` Vladimir Sementsov-Ogievskiy
2017-11-13 17:14 ` Markus Armbruster
2017-11-13 18:21 ` Eric Blake
3 siblings, 0 replies; 9+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2017-11-13 16:27 UTC (permalink / raw)
To: Eric Blake, qemu-devel; +Cc: qemu-block, qemu-stable, Paolo Bonzini
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-stable@nongnu.org
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
> 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(&req.length, len);
>
> if (nbd_write(ioc, &req, 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, &namelen, 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, &type, 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, &info->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(&info->size);
> if (nbd_read(ioc, &info->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, &info->min_block, sizeof(info->min_block),
> errp) < 0) {
> - error_prepend(errp, "failed to read info minimum block size");
> + error_prepend(errp, "failed to read info minimum block size: ");
> nbd_send_opt_abort(ioc);
> return -1;
> }
> @@ -441,7 +442,8 @@ static int nbd_opt_go(QIOChannel *ioc, const char *wantname,
> }
> if (nbd_read(ioc, &info->opt_block, sizeof(info->opt_block),
> errp) < 0) {
> - error_prepend(errp, "failed to read info preferred block size");
> + error_prepend(errp,
> + "failed to read info preferred block size: ");
> nbd_send_opt_abort(ioc);
> return -1;
> }
> @@ -455,7 +457,7 @@ static int nbd_opt_go(QIOChannel *ioc, const char *wantname,
> }
> if (nbd_read(ioc, &info->max_block, sizeof(info->max_block),
> errp) < 0) {
> - error_prepend(errp, "failed to read info maximum block size");
> + error_prepend(errp, "failed to read info maximum block size: ");
> nbd_send_opt_abort(ioc);
> return -1;
> }
> @@ -467,7 +469,7 @@ static int nbd_opt_go(QIOChannel *ioc, const char *wantname,
> default:
> trace_nbd_opt_go_info_unknown(type, nbd_info_lookup(type));
> if (nbd_drop(ioc, len, errp) < 0) {
> - error_prepend(errp, "Failed to read info payload");
> + error_prepend(errp, "Failed to read info payload: ");
> nbd_send_opt_abort(ioc);
> return -1;
> }
> @@ -618,7 +620,7 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name,
> }
>
> if (nbd_read(ioc, buf, 8, errp) < 0) {
> - error_prepend(errp, "Failed to read data");
> + error_prepend(errp, "Failed to read data: ");
> goto fail;
> }
>
> @@ -637,7 +639,7 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name,
> }
>
> if (nbd_read(ioc, &magic, sizeof(magic), errp) < 0) {
> - error_prepend(errp, "Failed to read magic");
> + error_prepend(errp, "Failed to read magic: ");
> goto fail;
> }
> magic = be64_to_cpu(magic);
> @@ -649,7 +651,7 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name,
> bool fixedNewStyle = false;
>
> if (nbd_read(ioc, &globalflags, sizeof(globalflags), errp) < 0) {
> - error_prepend(errp, "Failed to read server flags");
> + error_prepend(errp, "Failed to read server flags: ");
> goto fail;
> }
> globalflags = be16_to_cpu(globalflags);
> @@ -665,7 +667,7 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name,
> /* client requested flags */
> clientflags = cpu_to_be32(clientflags);
> if (nbd_write(ioc, &clientflags, sizeof(clientflags), errp) < 0) {
> - error_prepend(errp, "Failed to send clientflags field");
> + error_prepend(errp, "Failed to send clientflags field: ");
> goto fail;
> }
> if (tlscreds) {
> @@ -727,13 +729,13 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name,
>
> /* Read the response */
> if (nbd_read(ioc, &info->size, sizeof(info->size), errp) < 0) {
> - error_prepend(errp, "Failed to read export length");
> + error_prepend(errp, "Failed to read export length: ");
> goto fail;
> }
> be64_to_cpus(&info->size);
>
> if (nbd_read(ioc, &info->flags, sizeof(info->flags), errp) < 0) {
> - error_prepend(errp, "Failed to read export flags");
> + error_prepend(errp, "Failed to read export flags: ");
> goto fail;
> }
> be16_to_cpus(&info->flags);
> @@ -750,13 +752,13 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name,
> }
>
> if (nbd_read(ioc, &info->size, sizeof(info->size), errp) < 0) {
> - error_prepend(errp, "Failed to read export length");
> + error_prepend(errp, "Failed to read export length: ");
> goto fail;
> }
> be64_to_cpus(&info->size);
>
> if (nbd_read(ioc, &oldflags, sizeof(oldflags), errp) < 0) {
> - error_prepend(errp, "Failed to read export flags");
> + error_prepend(errp, "Failed to read export flags: ");
> goto fail;
> }
> be32_to_cpus(&oldflags);
> @@ -772,7 +774,7 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name,
>
> trace_nbd_receive_negotiate_size_flags(info->size, info->flags);
> if (zeroes && nbd_drop(ioc, 124, errp) < 0) {
> - error_prepend(errp, "Failed to read reserved block");
> + error_prepend(errp, "Failed to read reserved block: ");
> goto fail;
> }
> rc = 0;
If you go that way,
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.11] nbd/client: Use error_prepend() correctly
2017-11-13 15:32 ` Eric Blake
2017-11-13 16:23 ` Vladimir Sementsov-Ogievskiy
@ 2017-11-13 17:10 ` Markus Armbruster
1 sibling, 0 replies; 9+ messages in thread
From: Markus Armbruster @ 2017-11-13 17:10 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, Paolo Bonzini, vsementsov, qemu-stable, qemu-block
Eric Blake <eblake@redhat.com> 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-stable@nongnu.org
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> ---
>> 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(&req.length, len);
>>
>> if (nbd_write(ioc, &req, 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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.11] nbd/client: Use error_prepend() correctly
2017-11-13 15:24 [Qemu-devel] [PATCH for-2.11] nbd/client: Use error_prepend() correctly Eric Blake
2017-11-13 15:32 ` Eric Blake
2017-11-13 16:27 ` Vladimir Sementsov-Ogievskiy
@ 2017-11-13 17:14 ` Markus Armbruster
2017-11-13 18:19 ` Eric Blake
2017-11-13 18:21 ` Eric Blake
3 siblings, 1 reply; 9+ messages in thread
From: Markus Armbruster @ 2017-11-13 17:14 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, Paolo Bonzini, vsementsov, qemu-stable, qemu-block
Eric Blake <eblake@redhat.com> 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-stable@nongnu.org
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
> 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(&req.length, len);
>
> if (nbd_write(ioc, &req, 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, &namelen, 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, &type, 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, &info->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(&info->size);
> if (nbd_read(ioc, &info->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, &info->min_block, sizeof(info->min_block),
> errp) < 0) {
> - error_prepend(errp, "failed to read info minimum block size");
> + error_prepend(errp, "failed to read info minimum block size: ");
> nbd_send_opt_abort(ioc);
> return -1;
> }
> @@ -441,7 +442,8 @@ static int nbd_opt_go(QIOChannel *ioc, const char *wantname,
> }
> if (nbd_read(ioc, &info->opt_block, sizeof(info->opt_block),
> errp) < 0) {
> - error_prepend(errp, "failed to read info preferred block size");
> + error_prepend(errp,
> + "failed to read info preferred block size: ");
> nbd_send_opt_abort(ioc);
> return -1;
> }
> @@ -455,7 +457,7 @@ static int nbd_opt_go(QIOChannel *ioc, const char *wantname,
> }
> if (nbd_read(ioc, &info->max_block, sizeof(info->max_block),
> errp) < 0) {
> - error_prepend(errp, "failed to read info maximum block size");
> + error_prepend(errp, "failed to read info maximum block size: ");
> nbd_send_opt_abort(ioc);
> return -1;
> }
> @@ -467,7 +469,7 @@ static int nbd_opt_go(QIOChannel *ioc, const char *wantname,
> default:
> trace_nbd_opt_go_info_unknown(type, nbd_info_lookup(type));
> if (nbd_drop(ioc, len, errp) < 0) {
> - error_prepend(errp, "Failed to read info payload");
> + error_prepend(errp, "Failed to read info payload: ");
> nbd_send_opt_abort(ioc);
> return -1;
> }
> @@ -618,7 +620,7 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name,
> }
>
> if (nbd_read(ioc, buf, 8, errp) < 0) {
> - error_prepend(errp, "Failed to read data");
> + error_prepend(errp, "Failed to read data: ");
> goto fail;
> }
>
> @@ -637,7 +639,7 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name,
> }
>
> if (nbd_read(ioc, &magic, sizeof(magic), errp) < 0) {
> - error_prepend(errp, "Failed to read magic");
> + error_prepend(errp, "Failed to read magic: ");
> goto fail;
> }
> magic = be64_to_cpu(magic);
> @@ -649,7 +651,7 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name,
> bool fixedNewStyle = false;
>
> if (nbd_read(ioc, &globalflags, sizeof(globalflags), errp) < 0) {
> - error_prepend(errp, "Failed to read server flags");
> + error_prepend(errp, "Failed to read server flags: ");
> goto fail;
> }
> globalflags = be16_to_cpu(globalflags);
> @@ -665,7 +667,7 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name,
> /* client requested flags */
> clientflags = cpu_to_be32(clientflags);
> if (nbd_write(ioc, &clientflags, sizeof(clientflags), errp) < 0) {
> - error_prepend(errp, "Failed to send clientflags field");
> + error_prepend(errp, "Failed to send clientflags field: ");
> goto fail;
> }
> if (tlscreds) {
> @@ -727,13 +729,13 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name,
>
> /* Read the response */
> if (nbd_read(ioc, &info->size, sizeof(info->size), errp) < 0) {
> - error_prepend(errp, "Failed to read export length");
> + error_prepend(errp, "Failed to read export length: ");
> goto fail;
> }
> be64_to_cpus(&info->size);
>
> if (nbd_read(ioc, &info->flags, sizeof(info->flags), errp) < 0) {
> - error_prepend(errp, "Failed to read export flags");
> + error_prepend(errp, "Failed to read export flags: ");
> goto fail;
> }
> be16_to_cpus(&info->flags);
> @@ -750,13 +752,13 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name,
> }
>
> if (nbd_read(ioc, &info->size, sizeof(info->size), errp) < 0) {
> - error_prepend(errp, "Failed to read export length");
> + error_prepend(errp, "Failed to read export length: ");
> goto fail;
> }
> be64_to_cpus(&info->size);
>
> if (nbd_read(ioc, &oldflags, sizeof(oldflags), errp) < 0) {
> - error_prepend(errp, "Failed to read export flags");
> + error_prepend(errp, "Failed to read export flags: ");
> goto fail;
> }
> be32_to_cpus(&oldflags);
> @@ -772,7 +774,7 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name,
>
> trace_nbd_receive_negotiate_size_flags(info->size, info->flags);
> if (zeroes && nbd_drop(ioc, 124, errp) < 0) {
> - error_prepend(errp, "Failed to read reserved block");
> + error_prepend(errp, "Failed to read reserved block: ");
> goto fail;
> }
> rc = 0;
Reviewed-by: Markus Armbruster <armbru@redhat.com>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.11] nbd/client: Use error_prepend() correctly
2017-11-13 17:14 ` Markus Armbruster
@ 2017-11-13 18:19 ` Eric Blake
2017-11-14 7:28 ` Markus Armbruster
0 siblings, 1 reply; 9+ messages in thread
From: Eric Blake @ 2017-11-13 18:19 UTC (permalink / raw)
To: Markus Armbruster
Cc: qemu-devel, Paolo Bonzini, vsementsov, qemu-stable, qemu-block
[-- Attachment #1: Type: text/plain, Size: 1413 bytes --]
On 11/13/2017 11:14 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> 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 <armbru@redhat.com>
>
Thanks
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.11] nbd/client: Use error_prepend() correctly
2017-11-13 15:24 [Qemu-devel] [PATCH for-2.11] nbd/client: Use error_prepend() correctly Eric Blake
` (2 preceding siblings ...)
2017-11-13 17:14 ` Markus Armbruster
@ 2017-11-13 18:21 ` Eric Blake
3 siblings, 0 replies; 9+ messages in thread
From: Eric Blake @ 2017-11-13 18:21 UTC (permalink / raw)
To: qemu-devel; +Cc: Paolo Bonzini, vsementsov, qemu-stable, qemu-block
[-- Attachment #1: Type: text/plain, Size: 927 bytes --]
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-stable@nongnu.org
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
> 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
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.11] nbd/client: Use error_prepend() correctly
2017-11-13 18:19 ` Eric Blake
@ 2017-11-14 7:28 ` Markus Armbruster
0 siblings, 0 replies; 9+ messages in thread
From: Markus Armbruster @ 2017-11-14 7:28 UTC (permalink / raw)
To: Eric Blake; +Cc: Paolo Bonzini, vsementsov, qemu-devel, qemu-block, qemu-stable
Eric Blake <eblake@redhat.com> writes:
> On 11/13/2017 11:14 AM, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> 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 <armbru@redhat.com>
>>
>
> Thanks
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-11-14 7:28 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-11-13 15:24 [Qemu-devel] [PATCH for-2.11] nbd/client: Use error_prepend() correctly Eric Blake
2017-11-13 15:32 ` Eric Blake
2017-11-13 16:23 ` Vladimir Sementsov-Ogievskiy
2017-11-13 17:10 ` Markus Armbruster
2017-11-13 16:27 ` Vladimir Sementsov-Ogievskiy
2017-11-13 17:14 ` Markus Armbruster
2017-11-13 18:19 ` Eric Blake
2017-11-14 7:28 ` Markus Armbruster
2017-11-13 18:21 ` Eric Blake
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.