Re: [Qemu-block] [PATCH v2 5/8] nbd: BLOCK_STATUS for standard get_block_status function: client part

[Eric Blake]

On 03/12/2018 10:21 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.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---

v2: - drop iotests changes, as server is fixed in 03
 - rebase to byte-based block status
 - use payload_advance32
 - check extent->length for zero and for alignment (not sure about
   zero, but, we do not send block status with zero-length, so
   reply should not be zero-length too)


The NBD spec needs to be clarified that a zero-length request is bogus; 
once that is done, then the server can be required to make progress (if 
it succeeds, at least one non-zero extent was reported per namespace), 
as that is the most useful behavior (if a server replies with 0 extents 
or 0-length extents, the client could go into an inf-loop re-requesting 
the same status).



 - handle max_block in nbd_client_co_block_status
 - handle zero-length request in nbd_client_co_block_status
 - do not use magic numbers in nbd_negotiate_simple_meta_context

 ? Hm, don't remember, what we decided about DATA/HOLE flags mapping..


At this point, it's still up in the air for me to fix the complaints 
Kevin had, but those are bug fixes on top of this series (and thus okay 
during soft freeze), so your initial implementation is adequate for a 
first commit.



+++ b/block/nbd-client.c
@@ -228,6 +228,47 @@ static int 
nbd_parse_offset_hole_payload(NBDStructuredReplyChunk *chunk,
  return 0;
  }
  
+/* nbd_parse_blockstatus_payload

+ * support only one extent in reply and only for
+ * base:allocation context
+ */
+static int nbd_parse_blockstatus_payload(NBDClientSession *client,
+ NBDStructuredReplyChunk *chunk,
+ uint8_t *payload, uint64_t 
orig_length,
+ NBDExtent *extent, Error **errp)
+{
+uint32_t context_id;
+
+if (chunk->length != sizeof(context_id) + sizeof(extent)) {
+error_setg(errp, "Protocol error: invalid payload for "
+ "NBD_REPLY_TYPE_BLOCK_STATUS");
+return -EINVAL;
+}
+
+context_id = payload_advance32();
+if (client->info.meta_base_allocation_id != context_id) {
+error_setg(errp, "Protocol error: unexpected context id: %d for "


s/id:/id/


+ "NBD_REPLY_TYPE_BLOCK_STATUS, when negotiated context 
"
+ "id is %d", context_id,
+ client->info.meta_base_allocation_id);
+return -EINVAL;
+}
+
+extent->length = payload_advance32();
+extent->flags = payload_advance32();
+
+if (extent->length == 0 ||
+extent->length % client->info.min_block != 0 ||
+extent->length > orig_length)
+{
+/* TODO: clarify in NBD spec the second requirement about min_block */


Yeah, the spec wording can be tightened, but the intent is obvious: the 
server better not be reporting status on anything smaller than what you 
can address with read or write.  But I think we can address that on the 
NBD list without a TODO here.


However, you do have a bug: the server doesn't have to report min_block, 
so the value can still be zero (see nbd_refresh_limits, for example) - 
and %0 is a bad idea.  I'll do the obvious cleanup of checking for a 
non-zero min_block.



+error_setg(errp, "Protocol error: server sent chunk of invalid 
length");


Maybe insert 'status' in there?

  
+static int nbd_co_receive_blockstatus_reply(NBDClientSession *s,

+uint64_t handle, uint64_t length,
+NBDExtent *extent, Error **errp)
+{
+NBDReplyChunkIter iter;
+NBDReply reply;
+void *payload = NULL;
+Error *local_err = NULL;
+bool received = false;
+
+NBD_FOREACH_REPLY_CHUNK(s, iter, handle, s->info.structured_reply,
+NULL, , )
+{
+int ret;
+NBDStructuredReplyChunk *chunk = 
+
+assert(nbd_reply_is_structured());
+
+switch (chunk->type) {
+case NBD_REPLY_TYPE_BLOCK_STATUS:
+if (received) {
+s->quit = true;
+error_setg(_err, "Several BLOCK_STATUS chunks in reply");


Not necessarily an error later on when we request more than one 
namespace, but fine for the initial implementation where we really do 
expect exactly one status.



+nbd_iter_error(, true, -EINVAL, _err);
+}
+received = true;
+
+ret = nbd_parse_blockstatus_payload(s, ,
+payload, length, extent,
+_err);
+if (ret < 0) {
+s->quit = true;
+nbd_iter_error(, true, ret, _err);
+}
+   

[Qemu-block] [PATCH v2 5/8] nbd: BLOCK_STATUS for standard get_block_status function: client part

[Vladimir Sementsov-Ogievskiy]

Minimal realization: only one extent in server answer is supported.
Flag NBD_CMD_FLAG_REQ_ONE is used to force this behavior.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---

v2: - drop iotests changes, as server is fixed in 03
- rebase to byte-based block status
- use payload_advance32
- check extent->length for zero and for alignment (not sure about
  zero, but, we do not send block status with zero-length, so
  reply should not be zero-length too)
- handle max_block in nbd_client_co_block_status
- handle zero-length request in nbd_client_co_block_status
- do not use magic numbers in nbd_negotiate_simple_meta_context

? Hm, don't remember, what we decided about DATA/HOLE flags mapping..

 block/nbd-client.h  |   6 +++
 include/block/nbd.h |   3 ++
 block/nbd-client.c  | 144 
 block/nbd.c |   3 ++
 nbd/client.c| 117 ++
 5 files changed, 273 insertions(+)

diff --git a/block/nbd-client.h b/block/nbd-client.h
index 612c4c21a0..0ece76e5af 100644
--- a/block/nbd-client.h
+++ b/block/nbd-client.h
@@ -61,4 +61,10 @@ void nbd_client_detach_aio_context(BlockDriverState *bs);
 void nbd_client_attach_aio_context(BlockDriverState *bs,
AioContext *new_context);
 
+int coroutine_fn nbd_client_co_block_status(BlockDriverState *bs,
+bool want_zero,
+int64_t offset, int64_t bytes,
+int64_t *pnum, int64_t *map,
+BlockDriverState **file);
+
 #endif /* NBD_CLIENT_H */
diff --git a/include/block/nbd.h b/include/block/nbd.h
index 9f2be18186..3b34233dd4 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -262,6 +262,7 @@ struct NBDExportInfo {
 /* In-out fields, set by client before nbd_receive_negotiate() and
  * updated by server results during nbd_receive_negotiate() */
 bool structured_reply;
+bool base_allocation; /* base:allocation context for NBD_CMD_BLOCK_STATUS 
*/
 
 /* Set by server results during nbd_receive_negotiate() */
 uint64_t size;
@@ -269,6 +270,8 @@ struct NBDExportInfo {
 uint32_t min_block;
 uint32_t opt_block;
 uint32_t max_block;
+
+uint32_t meta_base_allocation_id;
 };
 typedef struct NBDExportInfo NBDExportInfo;
 
diff --git a/block/nbd-client.c b/block/nbd-client.c
index 0d9f73a137..b7683707b0 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -228,6 +228,47 @@ static int 
nbd_parse_offset_hole_payload(NBDStructuredReplyChunk *chunk,
 return 0;
 }
 
+/* nbd_parse_blockstatus_payload
+ * support only one extent in reply and only for
+ * base:allocation context
+ */
+static int nbd_parse_blockstatus_payload(NBDClientSession *client,
+ NBDStructuredReplyChunk *chunk,
+ uint8_t *payload, uint64_t 
orig_length,
+ NBDExtent *extent, Error **errp)
+{
+uint32_t context_id;
+
+if (chunk->length != sizeof(context_id) + sizeof(extent)) {
+error_setg(errp, "Protocol error: invalid payload for "
+ "NBD_REPLY_TYPE_BLOCK_STATUS");
+return -EINVAL;
+}
+
+context_id = payload_advance32();
+if (client->info.meta_base_allocation_id != context_id) {
+error_setg(errp, "Protocol error: unexpected context id: %d for "
+ "NBD_REPLY_TYPE_BLOCK_STATUS, when negotiated context 
"
+ "id is %d", context_id,
+ client->info.meta_base_allocation_id);
+return -EINVAL;
+}
+
+extent->length = payload_advance32();
+extent->flags = payload_advance32();
+
+if (extent->length == 0 ||
+extent->length % client->info.min_block != 0 ||
+extent->length > orig_length)
+{
+/* TODO: clarify in NBD spec the second requirement about min_block */
+error_setg(errp, "Protocol error: server sent chunk of invalid 
length");
+return -EINVAL;
+}
+
+return 0;
+}
+
 /* nbd_parse_error_payload
  * on success @errp contains message describing nbd error reply
  */
@@ -642,6 +683,61 @@ static int nbd_co_receive_cmdread_reply(NBDClientSession 
*s, uint64_t handle,
 return iter.ret;
 }
 
+static int nbd_co_receive_blockstatus_reply(NBDClientSession *s,
+uint64_t handle, uint64_t length,
+NBDExtent *extent, Error **errp)
+{
+NBDReplyChunkIter iter;
+NBDReply reply;
+void *payload = NULL;
+Error *local_err = NULL;
+bool received = false;
+
+NBD_FOREACH_REPLY_CHUNK(s, iter, handle, s->info.structured_reply,
+NULL, , )
+{
+int ret;
+