Re: [Qemu-devel] [PATCH 01/19] nbd/server: get rid of nbd_negotiate_read and friends

[Eric Blake]

On 05/30/2017 09:30 AM, Vladimir Sementsov-Ogievskiy wrote:
> Functions nbd_negotiate_{read,write,drop_sync} were introduced in
> 1a6245a5b, when nbd_wr_syncv was working through qemu_co_sendv_recvv,

There is no qemu_co_sendv_recvv. Did you mean qemu_co_recv/qemu_co_send?

> which just yields, without setting any handlers. But now, nbd_wr_syncv
> works through qio_channel_yield() which sets handlers, so watchers
> are redundant in nbd_negotiate_{read,write,drop_sync}, then, let's just
> use {read,write,drop}_sync functions.

Makes sense to me.  Note that I have a pending patch that was also
related to 1a6245a5, where we introduced an assertion failure (which
later morphed into a segfault) if a client hangs up really early during
negotiation:

https://lists.gnu.org/archive/html/qemu-devel/2017-05/msg06240.html

> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> ---

> +++ b/nbd/common.c
> @@ -69,6 +69,32 @@ ssize_t nbd_wr_syncv(QIOChannel *ioc,
>      return done;
>  }
>  
> +/* Discard length bytes from channel.  Return -errno on failure and 0 on
> + * success*/

Space before */, if you don't mind (yes, I know this was just code
motion, and that the old spot was wrong).

> +int drop_sync(QIOChannel *ioc, size_t size, Error **errp)

As part of moving it into nbd/common.c, please rename this function to
an nbd_ prefix, since non-static functions are more likely to collide
with the rest of the code base if not properly named.  Better yet: do it
in multiple patches:
- rename the static functions and fallout to callers (all of
nbd_drop_sync, nbd_read_sync, and nbd_write_sync)
- code motion (promote nbd_drop_sync from static in client.c to exported
in common.c)
- drop nbd_negotiate_ functions in favor of common nbd_*_sync functions

The idea makes sense, but I think there's too much going on in this one
commit.

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

signature.asc
Description: OpenPGP digital signature