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

[Vladimir Sementsov-Ogievskiy]

30.05.2017 23:10, Eric Blake wrote:
> 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?

qemu_co_recv is a macro, qemu_co_sendv_recvv - is an actual function, 
which do something.

#define qemu_co_recv(sockfd, buf, bytes) \
  qemu_co_send_recv(sockfd, buf, bytes, false)

ssize_t coroutine_fn
qemu_co_send_recv(int sockfd, void *buf, size_t bytes, bool do_send)
{
    struct iovec iov = { .iov_base = buf, .iov_len = bytes };
    return qemu_co_sendv_recvv(sockfd, &iov, 1, 0, bytes, do_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

OK

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

--
Best regards,
Vladimir