Re: [Qemu-devel] [PATCH 15/19] nbd/server: use errp instead of LOG

[Eric Blake <eblake@redhat.com>]

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

On 05/30/2017 09:30 AM, Vladimir Sementsov-Ogievskiy wrote:
> Move to modern errp scheme from just LOGging errors.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  nbd/server.c | 257 ++++++++++++++++++++++++++++++++++-------------------------
>  1 file changed, 150 insertions(+), 107 deletions(-)
> 

> @@ -143,30 +143,30 @@ static int nbd_negotiate_send_rep_len(QIOChannel *ioc, uint32_t type,
>            type, opt, len);
>  
>      magic = cpu_to_be64(NBD_REP_MAGIC);
> -    ret = write_sync(ioc, &magic, sizeof(magic), NULL);
> +    ret = write_sync(ioc, &magic, sizeof(magic), errp);
>      if (ret < 0) {
> -        LOG("write failed (rep magic)");
> +        error_prepend(errp, "write failed (rep magic): ");
>          return ret;
>      }

I'm wondering how much we really want to use error_prepend(), or if we
could just get away with using the original error message unchanged.
I'm not saying to rewrite the patch now that you've done the work, so
much as asking aloud whether the additional information in the error
messages will prove useful.

There are definitely some ripple effects from your v2 posting of the
first half of the series that will require you to rebase this, but the
overall idea is sound.

>  /* Send an error reply.
>   * Return -errno on error, 0 on success. */
> -static int GCC_FMT_ATTR(4, 5)
> +static int GCC_FMT_ATTR(5, 6)
>  nbd_negotiate_send_rep_err(QIOChannel *ioc, uint32_t type,
> -                           uint32_t opt, const char *fmt, ...)
> +                           uint32_t opt, Error **errp, const char *fmt, ...)

Having errp not be the last argument is unusual, but I don't see how you
can do any differently with the var-args nature of the call.

> @@ -505,26 +515,33 @@ static int nbd_negotiate_options(NBDClient *client)
>                   * disconnecting, but that we must also tolerate
>                   * guests that don't wait for our reply. */
>                  ret = nbd_negotiate_send_rep(client->ioc, NBD_REP_ACK,
> -                                             clientflags);
> -                return ret < 0 && ret != -EPIPE ? ret : 1;
> +                                             clientflags, &local_err);
> +
> +                if (ret < 0 && ret != -EPIPE) {
> +                    error_propagate(errp, local_err);
> +                    return ret;
> +                }
> +
> +                error_free(local_err);
> +                return 1;

Of course, you'll have to simplify this portion.  This is probably the
one place where you actually DO want to use:

nbd_negotiate_send_rep(client->ioc, NBD_REP_ACK,
                       clientflags, NULL)

because you truly do not care whether you had an error.

> @@ -1090,18 +1113,18 @@ static int nbd_co_receive_request(NBDRequestData *req, NBDRequest *request)
>  
>      /* Sanity checks, part 2. */
>      if (request->from + request->len > client->exp->size) {
> -        LOG("operation past EOF; From: %" PRIu64 ", Len: %" PRIu32
> -            ", Size: %" PRIu64, request->from, request->len,
> -            (uint64_t)client->exp->size);
> +        error_setg(errp, "operation past EOF; From: %" PRIu64 ", Len: %" PRIu32
> +                   ", Size: %" PRIu64, request->from, request->len,
> +                   (uint64_t)client->exp->size);
>          return request->type == NBD_CMD_WRITE ? -ENOSPC : -EINVAL;

Question - now that we are consistently setting a decent errp, do we
have any callers that care about specific -errno return values, or could
we further simplify the functions by just returning -1 (instead of
worrying about -errno) on failures?


> @@ -1244,21 +1268,33 @@ static coroutine_fn void nbd_trip(void *opaque)

>  
>  reply:
> +    if (local_err) {
> +        error_report_err(local_err);
> +        local_err = NULL;
> +    }

This says that after we detect an error, we report it,

> +
> +    if (nbd_co_send_reply(req, &reply, reply_data_len) < 0) {
> +        error_setg(&local_err, "Failed to send reply");
> +        goto disconnect;
> +    }

...but then blindly try to send the reply anyways, forgetting that we
ever detected the original error.  Is that going to bite us?

-- 
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: 604 bytes --]