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
