Re: [Qemu-devel] [PATCH v2 3/8] nbd: BLOCK_STATUS for standard get_block_status function: server part

[Eric Blake]

On 03/13/2018 08:47 AM, Eric Blake wrote:

On 03/12/2018 10:21 AM, Vladimir Sementsov-Ogievskiy wrote:

Minimal realization: only one extent in server answer is supported.

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




+/* nbd_negotiate_meta_queries
+ * Handle NBD_OPT_LIST_META_CONTEXT and NBD_OPT_SET_META_CONTEXT
+ *
+ * @meta may be NULL, if caller isn't interested in selected contexts 
(for

+ * NBD_OPT_LIST_META_CONTEXT)
+ *
+ * Return -errno on I/O error, 0 if option was completely handled by
+ * sending a reply about inconsistent lengths, or 1 on success. */


Comment is wrong - this function never returns 1 (nor should it, as 
nbd_negotiate_options() expects a return of 1 only from NBD_OPT_ABORT).



+static int nbd_negotiate_meta_queries(NBDClient *client,
+  NBDExportMetaContexts *meta, 
Error **errp)

+{
+    int ret;
+    NBDExport *exp;
+    NBDExportMetaContexts local_meta;
+    uint32_t nb_queries;
+    int i;
+
+    assert(client->structured_reply);


Perhaps worth a comment that this is safe because we already filtered it 
out at the caller.



+
+    if (!meta) {
+    meta = _meta;
+    }


Or, we could check here, and even base our decision on whether to change 
'meta' due to client->opt...



@@ -856,6 +1064,22 @@ static int nbd_negotiate_options(NBDClient 
*client, uint16_t myflags,

  }
  break;
+    case NBD_OPT_LIST_META_CONTEXT:
+    case NBD_OPT_SET_META_CONTEXT:
+    if (!client->structured_reply) {
+    ret = nbd_opt_invalid(
+    client, errp,
+    "request option '%s' when structured reply "
+    "is not negotiated", 
nbd_opt_lookup(option));

+    } else if (option == NBD_OPT_LIST_META_CONTEXT) {
+    ret = nbd_negotiate_meta_queries(client, NULL, 
errp);

+    } else {
+    ret = nbd_negotiate_meta_queries(client,
+ 
>export_meta,

+ errp);
+    }


Then here, we just do a single ret = nbd_negotiate_meta_queries().

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH v2 3/8] nbd: BLOCK_STATUS for standard get_block_status function: server part

[Eric Blake]

On 03/12/2018 10:21 AM, Vladimir Sementsov-Ogievskiy wrote:

Minimal realization: only one extent in server answer is supported.

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

v2: - constants and type defs were splitted out by Eric, except for
 NBD_META_ID_BASE_ALLOCATION


The constant for NBD_META_ID_BASE_ALLOCATION was intentionally not split 
out; it is the only constant that is relevant only to the server side ;) 
 In fact,...



 - add nbd_opt_skip, to skip meta query remainder, if we are already sure,
 that the query selects nothing
 - check meta export name in OPT_EXPORT_NAME and OPT_GO
 - always set context_id = 0 for NBD_OPT_LIST_META_CONTEXT
 - negotiation rewritten to avoid wasting time and memory on reading long,
 obviously invalid meta queries
 - fixed ERR_INVALID->ERR_UNKNOWN if export not found in 
nbd_negotiate_meta_queries
 - check client->export_meta.valid in "case NBD_CMD_BLOCK_STATUS"


  include/block/nbd.h |   2 +
  nbd/server.c| 310 
  2 files changed, 312 insertions(+)

diff --git a/include/block/nbd.h b/include/block/nbd.h
index 2285637e67..9f2be18186 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -188,6 +188,8 @@ typedef struct NBDExtent {
  #define NBD_CMD_FLAG_REQ_ONE(1 << 3) /* only one extent in BLOCK_STATUS
* reply chunk */
  
+#define NBD_META_ID_BASE_ALLOCATION 0

+


...I will be squashing in a change to move it out of the .h and into the .c.


  /* Supported request types */
  enum {
  NBD_CMD_READ = 0,
diff --git a/nbd/server.c b/nbd/server.c
index 085e14afbf..16d7388085 100644
--- a/nbd/server.c



@@ -371,6 +396,12 @@ static int nbd_negotiate_handle_list(NBDClient *client, 
Error **errp)
  return nbd_negotiate_send_rep(client, NBD_REP_ACK, errp);
  }
  
+static void nbd_check_meta_export_name(NBDClient *client)

+{
+client->export_meta.valid = client->export_meta.valid &&
+strcmp(client->exp->name, client->export_meta.export_name) == 0;


The indentation makes this harder for me to parse (at first glance, I 
thought you had (a) && (b), and were either missing a side effect or 
return statement).  It's a lot more obvious what you are doing with:


client->export_meta.valid &= !strcmp(client->exp->name,
 client->export_meta.export_name);



+/* nbd_meta_base_query
+ *
+ * Handle query to 'base' namespace. For now, only base:allocation context is
+ * available in it.
+ *
+ * Return -errno on I/O error, 0 if option was completely handled by
+ * sending a reply about inconsistent lengths, or 1 on success. */
+static int nbd_meta_base_query(NBDClient *client, NBDExportMetaContexts *meta,
+   uint32_t len, Error **errp)


The comments don't describe what 'len' represents, I had to go read the 
call-site before I could understand this function.  If I understand 
correctly, this function is called at the point that we have parsed 
"base:" out of the longer overall name given to LIST or SET, and len is 
the remaining length of the overall name that still needs parsing.



+{
+int ret;
+char query[sizeof("allocation") - 1];


Why discard the trailing NUL from the size?  It doesn't hurt to leave it 
in, unless...



+size_t alen = strlen("allocation");


...Better than strlen() would be sizeof(query), as long as the trailing 
NUL is not changing the size of the array.



+
+if (len == 0) {
+if (client->opt == NBD_OPT_LIST_META_CONTEXT) {
+meta->base_allocation = true;
+}
+return 1;
+}


Okay, so here, the user requested "base:"; on list we return all 
contexts that we know (base:allocation); on set we fall through.



+
+if (len != alen) {
+return nbd_opt_skip(client, len, errp);
+}


Here, the user requested "base:garbage", where the garbage (including 
empty string on set) is a different length than "base:allocation".  It 
may be a valid string for a future NBD version, but for us, we know 
right away it is is not something we recognize, so we gracefully skip it.


Checking myself: if nbd_opt_skip returned -1, we have detected 
communication problems with the client; it does not matter if there is 
any unparsed data remaining in the current option.  It can only return 0 
if nbd_opt_invalid has already finished parsing the entire option (we 
are ready to parse the next NBD_OPT command, no further queries in the 
current option matter).  It can only return 1 if we finished parsing the 
current query, and are positioned ready to parse the next query. [1]



+
+ret = nbd_opt_read(client, query, len, errp);
+if (ret <= 0) {
+return ret;
+}
+
+if (strncmp(query, "allocation", alen) == 0) {


Here, you HAD to use strncmp because you didn't leave room for the 
trailing NUL in the array above.  Tradeoffs.  So I guess your