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 <vsement...@virtuozzo.com>
> ---
> +++ 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
