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

[Vladimir Sementsov-Ogievskiy]

31.05.2017 17:39, Eric Blake wrote:
> On 05/31/2017 08:12 AM, Vladimir Sementsov-Ogievskiy wrote:
> > 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.
> Oh. I see where I went wrong - I was grepping for 'nbd_co_sendv' and
> coming up short.
>
> > #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);
> > }
> The commit message still makes me chase through several layers of
> indirection, so I still wonder if referring to qemu_co_recv/qemu_co_send

I'll add a note like "(the path is nbd_wr_sync -> qemu_co_{recv/send} -> 
qemu_co_send_recv -> qemu_co_sendv_recvv)',
because I writing that qemu_co_recv yields will be confusing too.

> (which is what is directly used in that older commit for nbd_wr_syncv)
> is any better.  It is probably also worth referring back to commit
> ff82911cd as the point in time where we switched to the qio_channel
> code, thereby no longer needing to manage coroutine handlers quite as
> directly as beforehand.

OK

> > > > +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)
> In fact, does the _sync name buy us anything any more, or can we just go
> with the shorter nbd_drop(), nbd_read(), and nbd_write()?

OK, good idea.

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