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

[Markus Armbruster]

Eric Blake <address@hidden> 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: address@hidden
>> Signed-off-by: Eric Blake <address@hidden>
>> ---
>>  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.