|
From: | Vladimir Sementsov-Ogievskiy |
Subject: | Re: [Qemu-devel] [PATCH 01/19] nbd/server: get rid of nbd_negotiate_read and friends |
Date: | Wed, 31 May 2017 18:48:59 +0300 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.1.1 |
31.05.2017 17:56, Vladimir Sementsov-Ogievskiy wrote:
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:There is no qemu_co_sendv_recvv. Did you mean qemu_co_recv/qemu_co_send?Functions nbd_negotiate_{read,write,drop_sync} were introduced in 1a6245a5b, when nbd_wr_syncv was working through qemu_co_sendv_recvv,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_sendI'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 collidewith the rest of the code base if not properly named. Better yet: do itin 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.
nbd_wr_syncv shoud be renamed then too.As I understand, sync here is related to the fact that on EAGAIN from socket the function doesn't return. now it is also true (but instead of looping the function yields)..
On the other hand, it is normal for r/w functions to by synchronous, so having additional suffix for it looks redundant (contrariwise, we have
_aio suffix for async functions).also, _sync suffix in block layer is used when function does flush (so using it for other thing is confusing a bit).
- code motion (promote nbd_drop_sync from static in client.c to exportedin common.c)- drop nbd_negotiate_ functions in favor of common nbd_*_sync functionsOKThe idea makes sense, but I think there's too much going on in this onecommit.
-- Best regards, Vladimir
[Prev in Thread] | Current Thread | [Next in Thread] |