| Message ID | 20171012095319.136610-5-vsementsov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
| Series | nbd minimal structured read | expand |
On 10/12/2017 04:53 AM, Vladimir Sementsov-Ogievskiy wrote: > Use packed structure instead of pointer arithmetics. English is fun! In the subject line, I'm fairly certain that "structurize" is not likely to be in any dictionary, yet it is a perfect word describing the patch, so I'm not touching it ;) > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > include/block/nbd.h | 6 ++++++ > nbd/server.c | 36 ++++++++++++++---------------------- > 2 files changed, 20 insertions(+), 22 deletions(-) > > +++ b/nbd/server.c > @@ -902,26 +902,6 @@ static int nbd_receive_request(QIOChannel *ioc, NBDRequest *request, > return 0; > } > > -static int nbd_send_reply(QIOChannel *ioc, NBDReply *reply, Error **errp) > -{ > - uint8_t buf[NBD_REPLY_SIZE]; > - > - reply->error = system_errno_to_nbd_errno(reply->error); > - > - trace_nbd_send_reply(reply->error, reply->handle); We lose a trace here... > static int nbd_co_send_simple_reply(NBDRequestData *req, NBDReply *reply, > int len, Error **errp) > { > NBDClient *client = req->client; > + NBDSimpleReply simple_reply; > int ret; > > g_assert(qemu_in_coroutine()); > > trace_nbd_co_send_simple_reply(reply->handle, reply->error, len); > > + set_be_simple_reply(&simple_reply, system_errno_to_nbd_errno(reply->error), > + reply->handle); > + ...but it always occurred immediately after another trace that has redundant information (well, the trace you kept shows pre- rather than post-translation of errno value to NBD wire value, and the trace you drop didn't show length). I'm fine with the reduction, but it needs a tweak to trace-events to reap the dead trace. With that change, Reviewed-by: Eric Blake <eblake@redhat.com>
On 10/12/2017 04:42 PM, Eric Blake wrote: > On 10/12/2017 04:53 AM, Vladimir Sementsov-Ogievskiy wrote: >> Use packed structure instead of pointer arithmetics. > >> + set_be_simple_reply(&simple_reply, system_errno_to_nbd_errno(reply->error), >> + reply->handle); >> + > > ...but it always occurred immediately after another trace that has > redundant information (well, the trace you kept shows pre- rather than > post-translation of errno value to NBD wire value, On second thought, tracing what gets sent over the wire is probably nicer than what we had internally (especially if the client traces what it receives - it's nice to match up values on both sides of the wire). > > With that change, > Reviewed-by: Eric Blake <eblake@redhat.com> > So here's what I'm squashing in: diff --git i/nbd/server.c w/nbd/server.c index 69cd2cda76..65c08fa1cc 100644 --- i/nbd/server.c +++ w/nbd/server.c @@ -1201,14 +1201,13 @@ static int nbd_co_send_simple_reply(NBDRequestData *req, NBDReply *reply, { NBDClient *client = req->client; NBDSimpleReply simple_reply; + int nbd_err = system_errno_to_nbd_errno(reply->error); int ret; g_assert(qemu_in_coroutine()); - trace_nbd_co_send_simple_reply(reply->handle, reply->error, len); - - set_be_simple_reply(&simple_reply, system_errno_to_nbd_errno(reply->error), - reply->handle); + trace_nbd_co_send_simple_reply(reply->handle, nbd_err, len); + set_be_simple_reply(&simple_reply, nbd_err, reply->handle); qemu_co_mutex_lock(&client->send_lock); client->send_coroutine = qemu_coroutine_self(); diff --git i/nbd/trace-events w/nbd/trace-events index 4d6f86c2d4..e27614f050 100644 --- i/nbd/trace-events +++ w/nbd/trace-events @@ -51,7 +51,6 @@ nbd_negotiate_old_style(uint64_t size, unsigned flags) "advertising size %" PRIu nbd_negotiate_new_style_size_flags(uint64_t size, unsigned flags) "advertising size %" PRIu64 " and flags 0x%x" nbd_negotiate_success(void) "Negotiation succeeded" nbd_receive_request(uint32_t magic, uint16_t flags, uint16_t type, uint64_t from, uint32_t len) "Got request: { magic = 0x%" PRIx32 ", .flags = 0x%" PRIx16 ", .type = 0x%" PRIx16 ", from = %" PRIu64 ", len = %" PRIu32 " }" -nbd_send_reply(int32_t error, uint64_t handle) "Sending response to client: { .error = %" PRId32 ", handle = %" PRIu64 " }" nbd_blk_aio_attached(const char *name, void *ctx) "Export %s: Attaching clients to AIO context %p\n" nbd_blk_aio_detach(const char *name, void *ctx) "Export %s: Detaching clients from AIO context %p\n" nbd_co_send_simple_reply(uint64_t handle, uint32_t error, int len) "Send simple reply: handle = %" PRIu64 ", error = %" PRIu32 ", len = %d"
On 10/12/2017 04:53 AM, Vladimir Sementsov-Ogievskiy wrote: > Use packed structure instead of pointer arithmetics. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > include/block/nbd.h | 6 ++++++ > nbd/server.c | 36 ++++++++++++++---------------------- > 2 files changed, 20 insertions(+), 22 deletions(-) > > if (!len) { > - ret = nbd_send_reply(client->ioc, reply, errp); > + ret = nbd_write(client->ioc, &simple_reply, sizeof(simple_reply), NULL); > } else { > qio_channel_set_cork(client->ioc, true); > - ret = nbd_send_reply(client->ioc, reply, errp); > + ret = nbd_write(client->ioc, &simple_reply, sizeof(simple_reply), NULL); One more thing: this should be errp, not NULL - we don't want to lose the error, even if the next patch changes things yet again.
diff --git a/include/block/nbd.h b/include/block/nbd.h index 707fd37575..49008980f4 100644 --- a/include/block/nbd.h +++ b/include/block/nbd.h @@ -63,6 +63,12 @@ struct NBDReply { }; typedef struct NBDReply NBDReply; +typedef struct NBDSimpleReply { + uint32_t magic; /* NBD_SIMPLE_REPLY_MAGIC */ + uint32_t error; + uint64_t handle; +} QEMU_PACKED NBDSimpleReply; + /* Transmission (export) flags: sent from server to client during handshake, but describe what will happen during transmission */ #define NBD_FLAG_HAS_FLAGS (1 << 0) /* Flags are there */ diff --git a/nbd/server.c b/nbd/server.c index bc7f9f0fd6..43ade30ba3 100644 --- a/nbd/server.c +++ b/nbd/server.c @@ -902,26 +902,6 @@ static int nbd_receive_request(QIOChannel *ioc, NBDRequest *request, return 0; } -static int nbd_send_reply(QIOChannel *ioc, NBDReply *reply, Error **errp) -{ - uint8_t buf[NBD_REPLY_SIZE]; - - reply->error = system_errno_to_nbd_errno(reply->error); - - trace_nbd_send_reply(reply->error, reply->handle); - - /* Reply - [ 0 .. 3] magic (NBD_SIMPLE_REPLY_MAGIC) - [ 4 .. 7] error (0 == no error) - [ 7 .. 15] handle - */ - stl_be_p(buf, NBD_SIMPLE_REPLY_MAGIC); - stl_be_p(buf + 4, reply->error); - stq_be_p(buf + 8, reply->handle); - - return nbd_write(ioc, buf, sizeof(buf), errp); -} - #define MAX_NBD_REQUESTS 16 void nbd_client_get(NBDClient *client) @@ -1208,24 +1188,36 @@ void nbd_export_close_all(void) } } +static inline void set_be_simple_reply(NBDSimpleReply *reply, uint64_t error, + uint64_t handle) +{ + stl_be_p(&reply->magic, NBD_SIMPLE_REPLY_MAGIC); + stl_be_p(&reply->error, error); + stq_be_p(&reply->handle, handle); +} + static int nbd_co_send_simple_reply(NBDRequestData *req, NBDReply *reply, int len, Error **errp) { NBDClient *client = req->client; + NBDSimpleReply simple_reply; int ret; g_assert(qemu_in_coroutine()); trace_nbd_co_send_simple_reply(reply->handle, reply->error, len); + set_be_simple_reply(&simple_reply, system_errno_to_nbd_errno(reply->error), + reply->handle); + qemu_co_mutex_lock(&client->send_lock); client->send_coroutine = qemu_coroutine_self(); if (!len) { - ret = nbd_send_reply(client->ioc, reply, errp); + ret = nbd_write(client->ioc, &simple_reply, sizeof(simple_reply), NULL); } else { qio_channel_set_cork(client->ioc, true); - ret = nbd_send_reply(client->ioc, reply, errp); + ret = nbd_write(client->ioc, &simple_reply, sizeof(simple_reply), NULL); if (ret == 0) { ret = nbd_write(client->ioc, req->data, len, errp); if (ret < 0) {
Use packed structure instead of pointer arithmetics. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> --- include/block/nbd.h | 6 ++++++ nbd/server.c | 36 ++++++++++++++---------------------- 2 files changed, 20 insertions(+), 22 deletions(-)