Re: [Qemu-devel] [PATCH v2 2/6] nbd/server: use errp instead of LOG

[Eric Blake]

On 06/21/2017 10:34 AM, Vladimir Sementsov-Ogievskiy wrote:
> Move to modern errp scheme from just LOGging errors.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> ---
>  nbd/server.c | 268 
> +++++++++++++++++++++++++++++++++++------------------------
>  1 file changed, 161 insertions(+), 107 deletions(-)

Unfortunately longer, but I'm okay with that (and we already did it
client-side, so this is just a case of a patch series being spread out
over time).


>  
>  /* 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, ...)
>  {

Markus, this violates our usual idiom of errp last in code that is not
directly operating on an Error (the actual Error implementation in
error.c being the main obvious exception that lists errp first), but I
don't see any better approach. Do you have any thoughts on it?

> -    if (nbd_read(client->ioc, name, length, NULL) < 0) {
> -        LOG("read failed");
> +    if (nbd_read(client->ioc, name, length, errp) < 0) {
> +        error_prepend(errp, "read failed: ");
>          return -EINVAL;
>      }

I'm still not convinced we need quite as many error_prepend() calls (it
may be simpler to just use the underlying error message as-is, without
trying to prepend) - but deciding where it is fluff is a cleanup while
this patch is all about a mechanical conversion, so I'm not going to
hold up this patch while we solve that debate.


>      tioc = qio_channel_tls_new_server(ioc,
>                                        client->tlscreds,
>                                        client->tlsaclname,
> -                                      NULL);
> +                                      errp);

Yay for wiring things up to no longer ignore original error messages.

> @@ -352,14 +367,16 @@ static QIOChannel 
> *nbd_negotiate_handle_starttls(NBDClient *client,
>  /* nbd_negotiate_options
>   * Process all NBD_OPT_* client option commands.
>   * Return:
> - * -errno  on error
> - * 0       on successful negotiation
> - * 1       if client sent NBD_OPT_ABORT, i.e. on legal disconnect
> + * -errno  on error, errp is set
> + * 0       on successful negotiation, errp is not set
> + * 1       if client sent NBD_OPT_ABORT, i.e. on legal disconnect,
> + *         errp is not set

You'll have some rebase churn based on my comments on 1/6 (where I
suggested s/legal/valid/).  I'm not sure the change about documenting
whether errp is set is even necessary (we have enough common usage of
errp, that it is kind of easy to just assume that everyone knows errp is
set only on negative return); but it doesn't hurt to keep it.

> @@ -480,25 +497,33 @@ static int nbd_negotiate_options(NBDClient *client)
>                  /* NBD spec says we must try to reply before
>                   * disconnecting, but that we must also tolerate
>                   * guests that don't wait for our reply. */
> -                nbd_negotiate_send_rep(client->ioc, NBD_REP_ACK, 
> clientflags);
> +                nbd_negotiate_send_rep(client->ioc, NBD_REP_ACK, clientflags,
> +                                       &local_err);
> +
> +                if (local_err != NULL) {
> +                    TRACE("Reply to NBD_OPT_ABORT request failed: %s",
> +                          error_get_pretty(local_err));
> +                    error_free(local_err);
> +                }
> +

Not sure this is necessary.  I would have just used:

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

and then you don't even need a local_err variable.


> @@ -1089,6 +1121,7 @@ static coroutine_fn void nbd_trip(void *opaque)
>      int ret;
>      int flags;
>      int reply_data_len = 0;
> +    Error *local_err = NULL;
>  
>      TRACE("Reading request.");
>      if (client->closing) {
> @@ -1097,7 +1130,7 @@ static coroutine_fn void nbd_trip(void *opaque)

> @@ -1128,7 +1161,7 @@ static coroutine_fn void nbd_trip(void *opaque)
>          if (request.flags & NBD_CMD_FLAG_FUA) {
>              ret = blk_co_flush(exp->blk);
>              if (ret < 0) {
> -                LOG("flush failed");
> +                error_setg_errno(&local_err, -ret, "flush failed");
>                  reply.error = -ret;
>                  break;
>              }

>  reply:
> +    if (local_err) {
> +        /* If we are here local_err is not fatal error, already stored in
> +         * reply.error */
> +        error_report_err(local_err);
> +        local_err = NULL;
> +    }

So we still end up printing the error directly to stderr, the way LOG()
used to do; but in light of the recent CVEs on 'qemu-nbd -t' not being
robust to misbehaved clients, it does feel like the right thing (the
server detecting an error does not mean that the server should quit,
merely that it should log that a client is no longer connected).

Overall the change looks good to me, but I'll wait to see if Markus has
any design advice before giving my R-b.

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

signature.asc
Description: OpenPGP digital signature