| Message ID | 20170530143052.165002-2-vsementsov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
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 <vsementsov@virtuozzo.com> > --- > +++ 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.
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 <vsementsov@virtuozzo.com> >> --- >> +++ 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. >
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. >> >
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. >>>
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, &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. 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. >>>> >
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, type, opt, len); magic = cpu_to_be64(NBD_REP_MAGIC); - if (nbd_negotiate_write(ioc, &magic, sizeof(magic)) < 0) { + if (write_sync(ioc, &magic, sizeof(magic), NULL) < 0) { LOG("write failed (rep magic)"); return -EINVAL; } opt = cpu_to_be32(opt); - if (nbd_negotiate_write(ioc, &opt, sizeof(opt)) < 0) { + if (write_sync(ioc, &opt, sizeof(opt), NULL) < 0) { LOG("write failed (rep opt)"); return -EINVAL; } type = cpu_to_be32(type); - if (nbd_negotiate_write(ioc, &type, sizeof(type)) < 0) { + if (write_sync(ioc, &type, sizeof(type), NULL) < 0) { LOG("write failed (rep type)"); return -EINVAL; } len = cpu_to_be32(len); - if (nbd_negotiate_write(ioc, &len, sizeof(len)) < 0) { + if (write_sync(ioc, &len, sizeof(len), NULL) < 0) { LOG("write failed (rep data length)"); return -EINVAL; } @@ -255,7 +192,7 @@ nbd_negotiate_send_rep_err(QIOChannel *ioc, uint32_t type, if (ret < 0) { goto out; } - if (nbd_negotiate_write(ioc, msg, len) < 0) { + if (write_sync(ioc, msg, len, NULL) < 0) { LOG("write failed (error message)"); ret = -EIO; } else { @@ -286,15 +223,15 @@ static int nbd_negotiate_send_rep_list(QIOChannel *ioc, NBDExport *exp) } len = cpu_to_be32(name_len); - if (nbd_negotiate_write(ioc, &len, sizeof(len)) < 0) { + if (write_sync(ioc, &len, sizeof(len), NULL) < 0) { LOG("write failed (name length)"); return -EINVAL; } - if (nbd_negotiate_write(ioc, name, name_len) < 0) { + if (write_sync(ioc, name, name_len, NULL) < 0) { LOG("write failed (name buffer)"); return -EINVAL; } - if (nbd_negotiate_write(ioc, desc, desc_len) < 0) { + if (write_sync(ioc, desc, desc_len, NULL) < 0) { LOG("write failed (description buffer)"); return -EINVAL; } @@ -308,7 +245,7 @@ static int nbd_negotiate_handle_list(NBDClient *client, uint32_t length) NBDExport *exp; if (length) { - if (nbd_negotiate_drop_sync(client->ioc, length) < 0) { + if (drop_sync(client->ioc, length, NULL) < 0) { return -EIO; } return nbd_negotiate_send_rep_err(client->ioc, @@ -339,7 +276,7 @@ static int nbd_negotiate_handle_export_name(NBDClient *client, uint32_t length) LOG("Bad length received"); goto fail; } - if (nbd_negotiate_read(client->ioc, name, length) < 0) { + if (read_sync(client->ioc, name, length, NULL) < 0) { LOG("read failed"); goto fail; } @@ -372,7 +309,7 @@ static QIOChannel *nbd_negotiate_handle_starttls(NBDClient *client, TRACE("Setting up TLS"); ioc = client->ioc; if (length) { - if (nbd_negotiate_drop_sync(ioc, length) < 0) { + if (drop_sync(ioc, length, NULL) < 0) { return NULL; } nbd_negotiate_send_rep_err(ioc, NBD_REP_ERR_INVALID, NBD_OPT_STARTTLS, @@ -436,7 +373,7 @@ static int nbd_negotiate_options(NBDClient *client) ... Rest of request */ - if (nbd_negotiate_read(client->ioc, &flags, sizeof(flags)) < 0) { + if (read_sync(client->ioc, &flags, sizeof(flags), NULL) < 0) { LOG("read failed"); return -EIO; } @@ -462,7 +399,7 @@ static int nbd_negotiate_options(NBDClient *client) uint32_t clientflags, length; uint64_t magic; - if (nbd_negotiate_read(client->ioc, &magic, sizeof(magic)) < 0) { + if (read_sync(client->ioc, &magic, sizeof(magic), NULL) < 0) { LOG("read failed"); return -EINVAL; } @@ -472,15 +409,15 @@ static int nbd_negotiate_options(NBDClient *client) return -EINVAL; } - if (nbd_negotiate_read(client->ioc, &clientflags, - sizeof(clientflags)) < 0) + if (read_sync(client->ioc, &clientflags, + sizeof(clientflags), NULL) < 0) { LOG("read failed"); return -EINVAL; } clientflags = be32_to_cpu(clientflags); - if (nbd_negotiate_read(client->ioc, &length, sizeof(length)) < 0) { + if (read_sync(client->ioc, &length, sizeof(length), NULL) < 0) { LOG("read failed"); return -EINVAL; } @@ -510,7 +447,7 @@ static int nbd_negotiate_options(NBDClient *client) return -EINVAL; default: - if (nbd_negotiate_drop_sync(client->ioc, length) < 0) { + if (drop_sync(client->ioc, length, NULL) < 0) { return -EIO; } ret = nbd_negotiate_send_rep_err(client->ioc, @@ -548,7 +485,7 @@ static int nbd_negotiate_options(NBDClient *client) return nbd_negotiate_handle_export_name(client, length); case NBD_OPT_STARTTLS: - if (nbd_negotiate_drop_sync(client->ioc, length) < 0) { + if (drop_sync(client->ioc, length, NULL) < 0) { return -EIO; } if (client->tlscreds) { @@ -567,7 +504,7 @@ static int nbd_negotiate_options(NBDClient *client) } break; default: - if (nbd_negotiate_drop_sync(client->ioc, length) < 0) { + if (drop_sync(client->ioc, length, NULL) < 0) { return -EIO; } ret = nbd_negotiate_send_rep_err(client->ioc, @@ -656,12 +593,12 @@ static coroutine_fn int nbd_negotiate(NBDClientNewData *data) TRACE("TLS cannot be enabled with oldstyle protocol"); goto fail; } - if (nbd_negotiate_write(client->ioc, buf, sizeof(buf)) < 0) { + if (write_sync(client->ioc, buf, sizeof(buf), NULL) < 0) { LOG("write failed"); goto fail; } } else { - if (nbd_negotiate_write(client->ioc, buf, 18) < 0) { + if (write_sync(client->ioc, buf, 18, NULL) < 0) { LOG("write failed"); goto fail; } @@ -676,7 +613,7 @@ static coroutine_fn int nbd_negotiate(NBDClientNewData *data) stq_be_p(buf + 18, client->exp->size); stw_be_p(buf + 26, client->exp->nbdflags | myflags); len = client->no_zeroes ? 10 : sizeof(buf) - 18; - if (nbd_negotiate_write(client->ioc, buf + 18, len) < 0) { + if (write_sync(client->ioc, buf + 18, len, NULL) < 0) { LOG("write failed"); goto fail; }
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 <vsementsov@virtuozzo.com> --- nbd/client.c | 26 ------------- nbd/common.c | 26 +++++++++++++ nbd/nbd-internal.h | 2 + nbd/server.c | 107 +++++++++++------------------------------------------ 4 files changed, 50 insertions(+), 111 deletions(-)