05.04.2019 23:04, Eric Blake wrote: > On 4/5/19 9:39 AM, Vladimir Sementsov-Ogievskiy wrote: >> 03.04.2019 6:05, Eric Blake wrote: >>> We've recently added traces for clients to flag server non-compliance; >>> let's do the same for servers to flag client non-compliance. According > > Thus, s/Trace server/Trace client/ in the subject line. > >>> >>> Qemu as client used to have one spot where it sent non-compliant >>> requests: if the server sends an unaligned reply to >>> NBD_CMD_BLOCK_STATUS, and the client was iterating over the entire >>> disk, the next request would start at that unaligned point; this was >>> fixed in commit a39286dd when the client was taught to work around >>> server non-compliance; but is equally fixed if the server is patched >>> to not send unaligned replies in the first place (the next few patches >>> will address that). > > I'll have to reword this now that we know 4.0 will still have some of > those server bugs in place (as the tail of this series is deferred to 4.1). > > >>> @@ -660,6 +662,8 @@ static int nbd_negotiate_handle_info(NBDClient *client, >>> uint16_t myflags, >>> >>> if (client->opt == NBD_OPT_GO) { >>> client->exp = exp; >>> + client->check_align = blocksize ? >>> + blk_get_request_alignment(exp->blk) : 0; >> >> I think better set in same place, where sizes[0] set, or use sizes[0] here >> or add separate local >> varibale for it, so that, if "sizes[0] =" changes somehow, we will not >> forget to fix this place too. > > I don't want to set client->check_align for NBD_OPT_INFO; but I can > indeed use a temporary variable or hoist the computation so that it is > all in one spot. > > >>> +++ b/nbd/trace-events >>> @@ -71,4 +71,5 @@ nbd_co_send_extents(uint64_t handle, unsigned int >>> extents, uint32_t id, uint64_t >>> nbd_co_send_structured_error(uint64_t handle, int err, const char >>> *errname, const char *msg) "Send structured error reply: handle = %" PRIu64 >>> ", error = %d (%s), msg = '%s'" >>> nbd_co_receive_request_decode_type(uint64_t handle, uint16_t type, const >>> char *name) "Decoding type: handle = %" PRIu64 ", type = %" PRIu16 " (%s)" >>> nbd_co_receive_request_payload_received(uint64_t handle, uint32_t len) >>> "Payload received: handle = %" PRIu64 ", len = %" PRIu32 >>> +nbd_co_receive_align_compliance(const char *op) "client sent non-compliant >>> unaligned %s request" >> >> Don't you want print request->from, request->len and client->check_align as >> well? > > Wouldn't hurt. > >> >>> nbd_trip(void) "Reading request" >>> >> >> Patch seems correct anyway, so if you are in a hurry, it's OK as is: >> >> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com> >> > > Here's what I'll squash in; I think it is obvious enough to still keep > your R-b, if I send the pull request before you reply back. > > diff --git i/nbd/server.c w/nbd/server.c > index 3040ceb5606..cb38dfe6902 100644 > --- i/nbd/server.c > +++ w/nbd/server.c > @@ -535,6 +535,7 @@ static int nbd_negotiate_handle_info(NBDClient > *client, uint16_t myflags, > bool blocksize = false; > uint32_t sizes[3]; > char buf[sizeof(uint64_t) + sizeof(uint16_t)]; > + uint32_t check_align = 0; > > /* Client sends: > 4 bytes: L, name length (can be 0) > @@ -611,7 +612,7 @@ static int nbd_negotiate_handle_info(NBDClient > *client, uint16_t myflags, > * whether this is OPT_INFO or OPT_GO. */ > /* minimum - 1 for back-compat, or actual if client will obey it. */ > if (client->opt == NBD_OPT_INFO || blocksize) { > - sizes[0] = blk_get_request_alignment(exp->blk); > + check_align = sizes[0] = blk_get_request_alignment(exp->blk); > } else { > sizes[0] = 1; > } > @@ -662,8 +663,7 @@ static int nbd_negotiate_handle_info(NBDClient > *client, uint16_t myflags, > > if (client->opt == NBD_OPT_GO) { > client->exp = exp; > - client->check_align = blocksize ? > - blk_get_request_alignment(exp->blk) : 0; > + client->check_align = check_align; > QTAILQ_INSERT_TAIL(&client->exp->clients, client, next); > nbd_export_get(client->exp); > nbd_check_meta_export(client); > @@ -2136,7 +2136,10 @@ static int nbd_co_receive_request(NBDRequestData > *req, NBDRequest *request, > * The block layer gracefully handles unaligned requests, but > * it's still worth tracing client non-compliance > */ > - > trace_nbd_co_receive_align_compliance(nbd_cmd_lookup(request->type)); > + trace_nbd_co_receive_align_compliance(nbd_cmd_lookup(request->type, > + request->from, > + request->len, > + > client->check_align));
something strange here with brackets. > } > valid_flags = NBD_CMD_FLAG_FUA; > if (request->type == NBD_CMD_READ && client->structured_reply) { > diff --git i/nbd/trace-events w/nbd/trace-events > index 87a2b896c35..ec2d46c9247 100644 > --- i/nbd/trace-events > +++ w/nbd/trace-events > @@ -71,5 +71,5 @@ nbd_co_send_extents(uint64_t handle, unsigned int > extents, uint32_t id, uint64_t > nbd_co_send_structured_error(uint64_t handle, int err, const char > *errname, const char *msg) "Send structured error reply: handle = %" > PRIu64 ", error = %d (%s), msg = '%s'" > nbd_co_receive_request_decode_type(uint64_t handle, uint16_t type, > const char *name) "Decoding type: handle = %" PRIu64 ", type = %" PRIu16 > " (%s)" > nbd_co_receive_request_payload_received(uint64_t handle, uint32_t len) > "Payload received: handle = %" PRIu64 ", len = %" PRIu32 > -nbd_co_receive_align_compliance(const char *op) "client sent > non-compliant unaligned %s request" > +nbd_co_receive_align_compliance(const char *op, uint64_t from, uint32_t > len, uint32_t align) "client sent non-compliant unaligned %s request: > from=0x%" PRIx64 ", len=0x%x, align=0x%x" %x or % PRIx32 - doesn't matter? > nbd_trip(void) "Reading request" > > > OK for me, thanks. -- Best regards, Vladimir