[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-block] [PATCH 6/9] nbd: BLOCK_STATUS for standard get_block_st

From: Vladimir Sementsov-Ogievskiy
Subject: Re: [Qemu-block] [PATCH 6/9] nbd: BLOCK_STATUS for standard get_block_status function: client part
Date: Fri, 9 Mar 2018 21:47:50 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0

16.02.2018 23:40, Eric Blake wrote:
On 02/15/2018 07:51 AM, Vladimir Sementsov-Ogievskiy wrote:
Minimal realization: only one extent in server answer is supported.
Flag NBD_CMD_FLAG_REQ_ONE is used to force this behavior.


+    memcpy(extent, payload, sizeof(*extent));
+    be32_to_cpus(&extent->length);
+    be32_to_cpus(&extent->flags);

Instead of doing a memcpy() and then in-place bit-swizzling, you could do the swapping as part of assignment, for one less function call (and make the code a bit easier to extend, if we later drop our REQ_ONE limitation on only having one extent, because you'll advance payload as needed):

extent->length = payload_advance32(&payload);
extent->flags = payload_advance32(&payload);

Aha, yes. The funny thing is that these are my own helpers.

We should probably validate that the length field is a multiple of min_block (if a server tells us that all operations must be 512-byte aligned, then reports an extent that is smaller than 512 bytes, we have no way to ask for the status of the second half of the sector). Probably also something that needs to be explicitly stated in the NBD spec. [1]

+    if (extent->length > orig_length) {
+        error_setg(errp, "Protocol error: server sent chunk exceeding requested"
+                         " region");
+        return -EINVAL;

That matches the current spec wording, but I'm not sure I agree with it - what's wrong with a server providing a final extent that extends beyond the request, if the information was already available for free (the classic example: if the server never replies with HOLE or ZERO, then the entire file has the same status, so all requests could trivially be replied to by taking the starting offset to the end of the file as the returned length, rather than just clamping at the requested length).

Maybe. But our already released clients not prepared to such change =(

But, on the other hand, this gives us possibility to understand, the the whole target (for backup/mirror) is zero in one request, skipping target-zeroing loop by 4gb chunks.. What about adding such possibility with an additionally negotiated option or something like this? (and don't we now have same possibility with something like INFO?)

+    }
+    return 0;
  /* nbd_parse_error_payload
   * on success @errp contains message describing nbd error reply

Best regards,

reply via email to

[Prev in Thread] Current Thread [Next in Thread]