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

[Eric Blake]

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


>>> +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()?

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

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

signature.asc
Description: OpenPGP digital signature