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

[Eric Blake <eblake@redhat.com>]

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

On 05/31/2017 11:55 AM, Vladimir Sementsov-Ogievskiy wrote:
> Functions nbd_negotiate_{read,write,drop_sync} were introduced in
> 1a6245a5b, when nbd_rwv (was nbd_wr_sync) was working through
> qemu_co_sendv_recvv (the path is nbd_wr_sync -> qemu_co_{recv/send} ->
> qemu_co_send_recv -> qemu_co_sendv_recvv), which just yields, without
> setting any handlers. But starting from ff82911cd nbd_rwv (was
> 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 nbd_{read,write,drop} functions.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  nbd/server.c | 107 ++++++++++++-----------------------------------------------
>  1 file changed, 22 insertions(+), 85 deletions(-)


>     Server         Client
> @@ -205,22 +142,22 @@ static int nbd_negotiate_send_rep_len(QIOChannel *ioc, uint32_t type,
>            type, opt, len);
>  
>      magic = cpu_to_be64(NBD_REP_MAGIC);
> -    if (nbd_negotiate_write(ioc, &magic, sizeof(magic)) < 0) {
> +    if (nbd_write(ioc, &magic, sizeof(magic), NULL) < 0) {
>          LOG("write failed (rep magic)");
>          return -EINVAL;

This says that you are ignoring errors (via errp) rather than reporting
them (because we LOG() it instead).  You do clean it up later, but it
would be nice to mention that in the commit message.

But because we have the later error fixups, I'm okay with:

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
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 --]