Re: [Qemu-devel] [PATCH 01/19] nbd/server: get rid of nbd_negotiate_read and friends
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: 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, , 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. 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 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
Re: [Qemu-devel] [PATCH 01/19] nbd/server: get rid of nbd_negotiate_read and friends
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, , 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
Re: [Qemu-devel] [PATCH 01/19] nbd/server: get rid of nbd_negotiate_read and friends
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, , 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
Re: [Qemu-devel] [PATCH 01/19] nbd/server: get rid of nbd_negotiate_read and friends
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, , 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--- +++ 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
Re: [Qemu-devel] [PATCH 01/19] nbd/server: get rid of nbd_negotiate_read and friends
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? > 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> --- > +++ 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 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
[Qemu-devel] [PATCH 01/19] nbd/server: get rid of nbd_negotiate_read and friends
Functions nbd_negotiate_{read,write,drop_sync} were introduced in 1a6245a5b, when nbd_wr_syncv was working through qemu_co_sendv_recvv, 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. Signed-off-by: Vladimir Sementsov-Ogievskiy--- nbd/client.c | 26 - nbd/common.c | 26 + nbd/nbd-internal.h | 2 + nbd/server.c | 107 +++-- 4 files changed, 50 insertions(+), 111 deletions(-) diff --git a/nbd/client.c b/nbd/client.c index f9e1d75be4..3d15596120 100644 --- a/nbd/client.c +++ b/nbd/client.c @@ -86,32 +86,6 @@ static QTAILQ_HEAD(, NBDExport) exports = QTAILQ_HEAD_INITIALIZER(exports); */ -/* Discard length bytes from channel. Return -errno on failure and 0 on - * success*/ -static int drop_sync(QIOChannel *ioc, size_t size, Error **errp) -{ -ssize_t ret = 0; -char small[1024]; -char *buffer; - -buffer = sizeof(small) >= size ? small : g_malloc(MIN(65536, size)); -while (size > 0) { -ssize_t count = MIN(65536, size); -ret = read_sync(ioc, buffer, MIN(65536, size), errp); - -if (ret < 0) { -goto cleanup; -} -size -= count; -} - - cleanup: -if (buffer != small) { -g_free(buffer); -} -return ret; -} - /* Send an option request. * * The request is for option @opt, with @data containing @len bytes of diff --git a/nbd/common.c b/nbd/common.c index bd81637ab9..e520aae741 100644 --- a/nbd/common.c +++ 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*/ +int drop_sync(QIOChannel *ioc, size_t size, Error **errp) +{ +ssize_t ret = 0; +char small[1024]; +char *buffer; + +buffer = sizeof(small) >= size ? small : g_malloc(MIN(65536, size)); +while (size > 0) { +ssize_t count = MIN(65536, size); +ret = read_sync(ioc, buffer, MIN(65536, size), errp); + +if (ret < 0) { +goto cleanup; +} +size -= count; +} + + cleanup: +if (buffer != small) { +g_free(buffer); +} +return ret; +} + void nbd_tls_handshake(QIOTask *task, void *opaque) diff --git a/nbd/nbd-internal.h b/nbd/nbd-internal.h index d6071640a0..01d5778679 100644 --- a/nbd/nbd-internal.h +++ b/nbd/nbd-internal.h @@ -153,4 +153,6 @@ struct NBDTLSHandshakeData { void nbd_tls_handshake(QIOTask *task, void *opaque); +int drop_sync(QIOChannel *ioc, size_t size, Error **errp); + #endif diff --git a/nbd/server.c b/nbd/server.c index ee59e5d234..26096f14e1 100644 --- a/nbd/server.c +++ b/nbd/server.c @@ -104,69 +104,6 @@ struct NBDClient { static void nbd_client_receive_next_request(NBDClient *client); -static gboolean nbd_negotiate_continue(QIOChannel *ioc, - GIOCondition condition, - void *opaque) -{ -qemu_coroutine_enter(opaque); -return TRUE; -} - -static int nbd_negotiate_read(QIOChannel *ioc, void *buffer, size_t size) -{ -ssize_t ret; -guint watch; - -assert(qemu_in_coroutine()); -/* Negotiation are always in main loop. */ -watch = qio_channel_add_watch(ioc, - G_IO_IN, - nbd_negotiate_continue, - qemu_coroutine_self(), - NULL); -ret = read_sync(ioc, buffer, size, NULL); -g_source_remove(watch); -return ret; - -} - -static int nbd_negotiate_write(QIOChannel *ioc, const void *buffer, size_t size) -{ -ssize_t ret; -guint watch; - -assert(qemu_in_coroutine()); -/* Negotiation are always in main loop. */ -watch = qio_channel_add_watch(ioc, - G_IO_OUT, - nbd_negotiate_continue, - qemu_coroutine_self(), - NULL); -ret = write_sync(ioc, buffer, size, NULL); -g_source_remove(watch); -return ret; -} - -static int nbd_negotiate_drop_sync(QIOChannel *ioc, size_t size) -{ -ssize_t ret; -uint8_t *buffer = g_malloc(MIN(65536, size)); - -while (size > 0) { -size_t count = MIN(65536, size); -ret = nbd_negotiate_read(ioc, buffer, count); -if (ret < 0) { -g_free(buffer); -return ret; -} - -size -= count; -} - -g_free(buffer); -return 0; -} - /* Basic flow for negotiation Server Client @@ -205,22 +142,22 @@ static int nbd_negotiate_send_rep_len(QIOChannel *ioc, uint32_t type,