Re: [Qemu-block] [PATCH 3/9] nbd: BLOCK_STATUS for standard get_block_status function: server part

[Vladimir Sementsov-Ogievskiy]

16.02.2018 16:21, Eric Blake wrote:

On 02/15/2018 07:51 AM, Vladimir Sementsov-Ogievskiy wrote:

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

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





[...]


+    meta = _meta;
+    }
+
+    memset(meta, 0, sizeof(*meta));
+
+    ret = nbd_read_size_string(client, meta->export_name,
+   NBD_MAX_NAME_SIZE, errp);


Revisiting a question I raised in my first half review - you saved the 
name as part of the struct because we have to later compare that the 
final OPT_SET export name matches the request during OPT_GO (if they 
don't match, then we have no contexts to serve after all).  So a 
'const char *' won't work, but maybe the struct could use a 'char *' 
pointing to malloc'd storage rather than char[MAX_NAME] that reserves 
array space that is mostly unused for the typical name that is much 
shorter than the maximum name length.


Do these 256 bytes worth creating nbd_clear_meta function, to call it 
from client_put and on NBD_OPT_SET_META_CONTEXT, to clear previous meta? 
If yes, I'd prefer to postpone it to continuation about BLOCKSTATUS for 
dirty bitmaps, when I'll have to upgrade meta structure, to maintain 
dirty bitmaps.





+    if (ret <= 0) {
+    return ret;
+    }
+
+    exp = nbd_export_find(meta->export_name);
+    if (exp == NULL) {
+    return nbd_opt_invalid(client, errp,
+   "export '%s' not present", 
meta->export_name);





--
Best regards,
Vladimir

Re: [Qemu-block] [PATCH 3/9] nbd: BLOCK_STATUS for standard get_block_status function: server part

[Vladimir Sementsov-Ogievskiy]

16.02.2018 20:01, Eric Blake wrote:

On 02/16/2018 08:43 AM, Vladimir Sementsov-Ogievskiy wrote:

16.02.2018 16:21, Eric Blake wrote:

On 02/15/2018 07:51 AM, Vladimir Sementsov-Ogievskiy wrote:

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

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



Again, function comments are useful.


+    if (query[0] == '\0' || strcmp(query, "allocation") == 0) {
+    /* Note: empty query should select all contexts within base
+ * namespace. */
+    meta->base_allocation = true;


From the client perspective, this handling of the empty leaf-name 
works well for NBD_OPT_LIST_META_CONTEXT (I want to see what leaf 
nodes the server supports), but not so well for 
NBD_OPT_SET_META_CONTEXT (asking the server to send ALL base 
allocations, even when I don't necessarily know how to interpret 
anything other than base:allocation, is a waste). So this function 
needs a bool parameter that says whether it is being invoked from 
_LIST (empty string okay, to advertise ALL base leaf names back to 
client, which for now is just base:allocation) or from _SET (empty 
string is ignored as invalid; client has to specifically ask for 
base:allocation by name).


"empty string is ignored as invalid", hm, do we have this in spec? I 
think SET and LIST must select exactly same sets of contexts.


No, it is specifically NOT intended that SET and LIST have to produce 
the same set of contexts; although I do see your point that as 
currently written, it does not appear to require SET to ignore "base:" 
or to treat it as an error.  At any rate, the spec gives the example of:


In either case, however, for any given namespace the server MAY, 
instead of exhaustively listing every matching context available to 
select (or every context available to select where no query is 
given), send sufficient context records back to allow a client with 
knowledge of the namespace to select any context. This may be helpful 
where a client can construct algorithmic queries. For instance, a 
client might reply simply with the namespace with no leaf-name (e.g. 
'X-FooBar:') or with a range of values (e.g. 
'X-ModifiedDate:20160310-20161214'). The semantics of such a reply 
are a matter for the definition of the namespace. However each 
namespace returned MUST begin with the relevant namespace, followed 
by a colon, and then other UTF-8 characters, with the entire string 
following the restrictions for strings set out earlier in this document.


with the intent being that for some namespaces, it may be easy to 
perform an algorithmic query via _LIST to see what ranges are 
supported, but that you cannot select ALL elements in the range 
simultaneously. The empty query for _LIST exists to enumerate what is 
supported, but does not have to equate to an empty query for _SET 
selecting everything possible.  I could even see it being possible to 
have some round-trips, depending on the namespace (of course, any 
namespace other than "base:" will be tightly coordinated between both 
client and server, so they understand each other - the point was that 
the NBD spec didn't want to constrain what a client and server could 
do as long as they stay within the generic framework):


C> LIST ""
S> REPLY "base:allocation" id 0
S> REPLY "X-FooBar:" id 0
S> ACK
C> LIST "X-FooBar:"
S> REPLY "X-FooBar:A_Required", id 0
S> REPLY "X-FooBar:A_Min=100", id 0
S> REPLY "X-FooBar:A_Max=200", id 0
S> REPLY "X-FooBar:B_Default=300", id 0
S> REPLY "X-FooBar:B_Min=300", id 0
S> REPLY "X-FooBar:B_Max=400", id 0
S> ACK
C> SET "X-FooBar:A=150" "base:allocation"
S> REPLY "X-FooBar:A=150,B=300", id 1
S> REPLY "base:allocation", id 2
S> ACK

where the global query of all available contexts merely lists that 
X-FooBar: is understood, but that a specific query is needed for more 
details (to avoid the client having to parse those specifics if it 
doesn't care about X-FooBar:), and the specific query sets up the 
algorithmic description (parameter A is required, between 100 and 200; 
parameter B is optional, between 300 and 400, defaulting to 300), and 
the final SET gives the actual request (A given a value, B left to its 
default; but the reply names the entire context rather than repeating 
the shorter request).  So the spec is written to permit something like 
that for third-party namespaces, while also trying to be very specific 
about the "base:" context as that is the one that needs the most 
interoperability.


It is strange behavior of client to set "base:", but it is its 
decision. And I don't thing that it is invalid.


For LIST, querying "base:" makes total sense (for the sake of example, 
we may add "base:frob" down the road that does something new.  Being 
able to LIST whether "base:" turns into just "base:allocation" or into 
"base:allocation"+"base:frob" may be useful to a client that 
understands both formats and wants to probe if the server is new; and 
even for a client 

Re: [Qemu-block] [PATCH 3/9] nbd: BLOCK_STATUS for standard get_block_status function: server part

[Eric Blake]

On 02/16/2018 08:43 AM, Vladimir Sementsov-Ogievskiy wrote:

16.02.2018 16:21, Eric Blake wrote:

On 02/15/2018 07:51 AM, Vladimir Sementsov-Ogievskiy wrote:

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

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



Again, function comments are useful.


+    if (query[0] == '\0' || strcmp(query, "allocation") == 0) {
+    /* Note: empty query should select all contexts within base
+ * namespace. */
+    meta->base_allocation = true;


From the client perspective, this handling of the empty leaf-name 
works well for NBD_OPT_LIST_META_CONTEXT (I want to see what leaf 
nodes the server supports), but not so well for 
NBD_OPT_SET_META_CONTEXT (asking the server to send ALL base 
allocations, even when I don't necessarily know how to interpret 
anything other than base:allocation, is a waste). So this function 
needs a bool parameter that says whether it is being invoked from 
_LIST (empty string okay, to advertise ALL base leaf names back to 
client, which for now is just base:allocation) or from _SET (empty 
string is ignored as invalid; client has to specifically ask for 
base:allocation by name).


"empty string is ignored as invalid", hm, do we have this in spec? I 
think SET and LIST must select exactly same sets of contexts.


No, it is specifically NOT intended that SET and LIST have to produce 
the same set of contexts; although I do see your point that as currently 
written, it does not appear to require SET to ignore "base:" or to treat 
it as an error.  At any rate, the spec gives the example of:



In either case, however, for any given namespace the server MAY, instead of 
exhaustively listing every matching context available to select (or every 
context available to select where no query is given), send sufficient context 
records back to allow a client with knowledge of the namespace to select any 
context. This may be helpful where a client can construct algorithmic queries. 
For instance, a client might reply simply with the namespace with no leaf-name 
(e.g. 'X-FooBar:') or with a range of values (e.g. 
'X-ModifiedDate:20160310-20161214'). The semantics of such a reply are a matter 
for the definition of the namespace. However each namespace returned MUST begin 
with the relevant namespace, followed by a colon, and then other UTF-8 
characters, with the entire string following the restrictions for strings set 
out earlier in this document.


with the intent being that for some namespaces, it may be easy to 
perform an algorithmic query via _LIST to see what ranges are supported, 
but that you cannot select ALL elements in the range simultaneously. 
The empty query for _LIST exists to enumerate what is supported, but 
does not have to equate to an empty query for _SET selecting everything 
possible.  I could even see it being possible to have some round-trips, 
depending on the namespace (of course, any namespace other than "base:" 
will be tightly coordinated between both client and server, so they 
understand each other - the point was that the NBD spec didn't want to 
constrain what a client and server could do as long as they stay within 
the generic framework):


C> LIST ""
S> REPLY "base:allocation" id 0
S> REPLY "X-FooBar:" id 0
S> ACK
C> LIST "X-FooBar:"
S> REPLY "X-FooBar:A_Required", id 0
S> REPLY "X-FooBar:A_Min=100", id 0
S> REPLY "X-FooBar:A_Max=200", id 0
S> REPLY "X-FooBar:B_Default=300", id 0
S> REPLY "X-FooBar:B_Min=300", id 0
S> REPLY "X-FooBar:B_Max=400", id 0
S> ACK
C> SET "X-FooBar:A=150" "base:allocation"
S> REPLY "X-FooBar:A=150,B=300", id 1
S> REPLY "base:allocation", id 2
S> ACK

where the global query of all available contexts merely lists that 
X-FooBar: is understood, but that a specific query is needed for more 
details (to avoid the client having to parse those specifics if it 
doesn't care about X-FooBar:), and the specific query sets up the 
algorithmic description (parameter A is required, between 100 and 200; 
parameter B is optional, between 300 and 400, defaulting to 300), and 
the final SET gives the actual request (A given a value, B left to its 
default; but the reply names the entire context rather than repeating 
the shorter request).  So the spec is written to permit something like 
that for third-party namespaces, while also trying to be very specific 
about the "base:" context as that is the one that needs the most 
interoperability.


It is 
strange behavior of client to set "base:", but it is its decision. And I 
don't thing that it is invalid.


For LIST, querying "base:" makes total sense (for the sake of example, 
we may add "base:frob" down the road that does something new.  Being 
able to LIST whether "base:" turns into just "base:allocation" or into 
"base:allocation"+"base:frob" may be useful to a client that understands 
both formats and wants to probe if the server is new; and even for a 
client right now, the client can gracefully 

Re: [Qemu-block] [PATCH 3/9] nbd: BLOCK_STATUS for standard get_block_status function: server part

[Vladimir Sementsov-Ogievskiy]

16.02.2018 16:21, Eric Blake wrote:

On 02/15/2018 07:51 AM, Vladimir Sementsov-Ogievskiy wrote:

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

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


continuing where I left off,



+++ b/nbd/common.c
@@ -75,6 +75,10 @@ const char *nbd_opt_lookup(uint32_t opt)
  return "go";
  case NBD_OPT_STRUCTURED_REPLY:
  return "structured reply";
+    case NBD_OPT_LIST_META_CONTEXT:
+    return "list meta context";
+    case NBD_OPT_SET_META_CONTEXT:
+    return "set meta context";
  default:


Should the changes to the header for new macros and to common.c for 
mapping bits to names be split into a separate patch, so that someone 
could backport just the new constants and then the client-side 
implementation, rather than being forced to backport the rest of the 
server implementation at the same time?


Not a problem, I can split. I've thought about it, but decided for first 
roll send it as one patch.





+
+/* nbd_read_size_string
+ *
+ * Read string in format
+ *   uint32_t len
+ *   len bytes string (not 0-terminated)
+ *
+ * @buf should be enough to store @max_len+1
+ *
+ * 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_read_size_string(NBDClient *client, char *buf,
+    uint32_t max_len, Error **errp)


Would existing code benefit from using this helper?  If so, splitting 
it into a separate patch, plus converting initial clients to use it, 
would be worthwhile.




+
+static int nbd_negotiate_send_meta_context(NBDClient *client,
+   const char *context,
+   uint32_t context_id,
+   Error **errp)


No comment documenting this function?


+{
+    NBDOptionReplyMetaContext opt;
+    struct iovec iov[] = {
+    {.iov_base = , .iov_len = sizeof(opt)},
+    {.iov_base = (void *)context, .iov_len = strlen(context)}


Casting to void* looks suspicious, but I see that it is casting away 
const.  Okay.



+    };
+
+    set_be_option_rep(, client->opt, NBD_REP_META_CONTEXT,
+  sizeof(opt) - sizeof(opt.h) + iov[1].iov_len);
+    stl_be_p(_id, context_id);
+
+    return qio_channel_writev_all(client->ioc, iov, 2, errp) < 0 ? 
-EIO : 0;

+}
+
+static void nbd_meta_base_query(NBDExportMetaContexts *meta, const 
char *query)

+{


Again, function comments are useful.


+    if (query[0] == '\0' || strcmp(query, "allocation") == 0) {
+    /* Note: empty query should select all contexts within base
+ * namespace. */
+    meta->base_allocation = true;


From the client perspective, this handling of the empty leaf-name 
works well for NBD_OPT_LIST_META_CONTEXT (I want to see what leaf 
nodes the server supports), but not so well for 
NBD_OPT_SET_META_CONTEXT (asking the server to send ALL base 
allocations, even when I don't necessarily know how to interpret 
anything other than base:allocation, is a waste). So this function 
needs a bool parameter that says whether it is being invoked from 
_LIST (empty string okay, to advertise ALL base leaf names back to 
client, which for now is just base:allocation) or from _SET (empty 
string is ignored as invalid; client has to specifically ask for 
base:allocation by name).


"empty string is ignored as invalid", hm, do we have this in spec? I 
think SET and LIST must select exactly same sets of contexts. It is 
strange behavior of client to set "base:", but it is its decision. And I 
don't thing that it is invalid.
Formally we may answer with NBD_REP_ERR_TOO_BIG, but it will look weird, 
as client see that both base: and base:allocation returns _one_  
context, but in one case it is too big. But if we will have several 
base: contextes, server may fairly answer with NBD_REP_ERR_TOO_BIG.


So, I think for now the code is ok.

Also, I don't see NBD_REP_ERR_TOO_BIG possible reply in 
NBD_OPT_LIST_META_CONTEXT description. Should it be here?





+    }
+}
+
+/* nbd_negotiate_meta_query
+ * 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_negotiate_meta_query(NBDClient *client,
+    NBDExportMetaContexts *meta, 
Error **errp)

+{
+    int ret;
+    char *query, *colon, *namespace, *subquery;


Is it worth stack-allocating query here, so you don't have to g_free() 
it later?  NBD limits the maximum string to 4k, which is a little bit 
big for stack allocation (on an operating system with 4k pages, 
allocating more than 4k on the stack in one function risks missing the 
guard page on stack overflow), but we also have the benefit that we 
KNOW that the set of meta-context namespaces that we support have a 
much smaller maximum limit of what we 

Re: [Qemu-block] [PATCH 3/9] nbd: BLOCK_STATUS for standard get_block_status function: server part

[Eric Blake]

On 02/15/2018 07:51 AM, Vladimir Sementsov-Ogievskiy wrote:

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

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


continuing where I left off,



+++ b/nbd/common.c
@@ -75,6 +75,10 @@ const char *nbd_opt_lookup(uint32_t opt)
  return "go";
  case NBD_OPT_STRUCTURED_REPLY:
  return "structured reply";
+case NBD_OPT_LIST_META_CONTEXT:
+return "list meta context";
+case NBD_OPT_SET_META_CONTEXT:
+return "set meta context";
  default:


Should the changes to the header for new macros and to common.c for 
mapping bits to names be split into a separate patch, so that someone 
could backport just the new constants and then the client-side 
implementation, rather than being forced to backport the rest of the 
server implementation at the same time?



+
+/* nbd_read_size_string
+ *
+ * Read string in format
+ *   uint32_t len
+ *   len bytes string (not 0-terminated)
+ *
+ * @buf should be enough to store @max_len+1
+ *
+ * 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_read_size_string(NBDClient *client, char *buf,
+uint32_t max_len, Error **errp)


Would existing code benefit from using this helper?  If so, splitting it 
into a separate patch, plus converting initial clients to use it, would 
be worthwhile.




+
+static int nbd_negotiate_send_meta_context(NBDClient *client,
+   const char *context,
+   uint32_t context_id,
+   Error **errp)


No comment documenting this function?


+{
+NBDOptionReplyMetaContext opt;
+struct iovec iov[] = {
+{.iov_base = , .iov_len = sizeof(opt)},
+{.iov_base = (void *)context, .iov_len = strlen(context)}


Casting to void* looks suspicious, but I see that it is casting away 
const.  Okay.



+};
+
+set_be_option_rep(, client->opt, NBD_REP_META_CONTEXT,
+  sizeof(opt) - sizeof(opt.h) + iov[1].iov_len);
+stl_be_p(_id, context_id);
+
+return qio_channel_writev_all(client->ioc, iov, 2, errp) < 0 ? -EIO : 0;
+}
+
+static void nbd_meta_base_query(NBDExportMetaContexts *meta, const char *query)
+{


Again, function comments are useful.


+if (query[0] == '\0' || strcmp(query, "allocation") == 0) {
+/* Note: empty query should select all contexts within base
+ * namespace. */
+meta->base_allocation = true;


From the client perspective, this handling of the empty leaf-name works 
well for NBD_OPT_LIST_META_CONTEXT (I want to see what leaf nodes the 
server supports), but not so well for NBD_OPT_SET_META_CONTEXT (asking 
the server to send ALL base allocations, even when I don't necessarily 
know how to interpret anything other than base:allocation, is a waste). 
So this function needs a bool parameter that says whether it is being 
invoked from _LIST (empty string okay, to advertise ALL base leaf names 
back to client, which for now is just base:allocation) or from _SET 
(empty string is ignored as invalid; client has to specifically ask for 
base:allocation by name).



+}
+}
+
+/* nbd_negotiate_meta_query
+ * 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_negotiate_meta_query(NBDClient *client,
+NBDExportMetaContexts *meta, Error **errp)
+{
+int ret;
+char *query, *colon, *namespace, *subquery;


Is it worth stack-allocating query here, so you don't have to g_free() 
it later?  NBD limits the maximum string to 4k, which is a little bit 
big for stack allocation (on an operating system with 4k pages, 
allocating more than 4k on the stack in one function risks missing the 
guard page on stack overflow), but we also have the benefit that we KNOW 
that the set of meta-context namespaces that we support have a much 
smaller maximum limit of what we even care about.



+
+ret = nbd_alloc_read_size_string(client, , errp);
+if (ret <= 0) {
+return ret;
+}
+
+colon = strchr(query, ':');
+if (colon == NULL) {
+ret = nbd_opt_invalid(client, errp, "no colon in query");
+goto out;


Hmm, that puts a slight wrinkle into my proposal, or else maybe it is 
something I should bring up on the NBD list.  If we only read 5 
characters (because the max namespace WE support is "base:"), but a 
client asks for namespace "X-longname:", we should gracefully ignore the 
client's request; while we still want to reply with an error to a client 
that asks for "garbage" with no colon at all.  The question for the NBD 
spec, then, is whether detecting bad client requests that didn't use 
colon is mandatory for the server (meaning we MUST read the 

Re: [Qemu-block] [PATCH 3/9] nbd: BLOCK_STATUS for standard get_block_status function: server part

[Eric Blake]

On 02/16/2018 05:09 AM, Vladimir Sementsov-Ogievskiy wrote:

16.02.2018 02:02, Eric Blake wrote:

On 02/15/2018 07:51 AM, Vladimir Sementsov-Ogievskiy wrote:

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

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  include/block/nbd.h |  33 ++
  nbd/common.c    |  10 ++
  nbd/server.c    | 310 
+++-

  3 files changed, 352 insertions(+), 1 deletion(-)




@@ -200,9 +227,15 @@ enum {
  #define NBD_REPLY_TYPE_NONE  0
  #define NBD_REPLY_TYPE_OFFSET_DATA   1
  #define NBD_REPLY_TYPE_OFFSET_HOLE   2
+#define NBD_REPLY_TYPE_BLOCK_STATUS  5


Stale; see nbd.git commit 56c77720 which changed this to 3.


Very unpleasant surprise. I understand that this is still experimental 
extension, but actually we use it =5 in production about one year. Can 
we revert it to 5?


For that, you'll have to ask on the upstream NBD list.  There may be 
other things that turn out to need tweaking as I get through the rest of 
your initial implementation, vs. what works well for interoperability, 
so having a distinct number for experimental vs. actual may be 
worthwhile for other reasons as well, even if it requires some glue code 
on your side to handle an older server sending 5 instead of 3 to a newer 
client.


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

Re: [Qemu-block] [PATCH 3/9] nbd: BLOCK_STATUS for standard get_block_status function: server part

[Vladimir Sementsov-Ogievskiy]

16.02.2018 02:02, Eric Blake wrote:

On 02/15/2018 07:51 AM, Vladimir Sementsov-Ogievskiy wrote:

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

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  include/block/nbd.h |  33 ++
  nbd/common.c    |  10 ++
  nbd/server.c    | 310 
+++-

  3 files changed, 352 insertions(+), 1 deletion(-)




@@ -200,9 +227,15 @@ enum {
  #define NBD_REPLY_TYPE_NONE  0
  #define NBD_REPLY_TYPE_OFFSET_DATA   1
  #define NBD_REPLY_TYPE_OFFSET_HOLE   2
+#define NBD_REPLY_TYPE_BLOCK_STATUS  5


Stale; see nbd.git commit 56c77720 which changed this to 3.


Very unpleasant surprise. I understand that this is still experimental 
extension, but actually we use it =5 in production about one year. Can 
we revert it to 5?





+++ b/nbd/server.c
@@ -82,6 +82,15 @@ struct NBDExport {
    static QTAILQ_HEAD(, NBDExport) exports = 
QTAILQ_HEAD_INITIALIZER(exports);

  +/* NBDExportMetaContexts represents list of selected by
+ * NBD_OPT_SET_META_CONTEXT contexts to be exported. */


represents a list of contexts to be exported, as selected by 
NBD_OPT_SET_META_CONTEXT.



+typedef struct NBDExportMetaContexts {
+    char export_name[NBD_MAX_NAME_SIZE + 1];


Would this work as const char * pointing at some other storage, 
instead of having to copy into this storage?


I'll think about



+    bool valid; /* means that negotiation of the option finished 
without

+   errors */
+    bool base_allocation; /* export base:allocation context (block 
status) */

+} NBDExportMetaContexts;
+
  struct NBDClient {


@@ -636,6 +646,201 @@ static QIOChannel 
*nbd_negotiate_handle_starttls(NBDClient *client,

  return QIO_CHANNEL(tioc);
  }
  +/* nbd_alloc_read_size_string
+ *
+ * Read string in format
+ *   uint32_t len
+ *   len bytes string (not 0-terminated)
+ * String is allocated and pointer returned as @buf
+ *
+ * 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_alloc_read_size_string(NBDClient *client, char **buf,
+  Error **errp)
+{
+    int ret;
+    uint32_t len;
+
+    ret = nbd_opt_read(client, , sizeof(len), errp);
+    if (ret <= 0) {
+    return ret;
+    }
+    cpu_to_be32s();
+
+    *buf = g_try_malloc(len + 1);


I'd rather check that len is sane prior to trying to malloc. 
Otherwise, a malicious client can convince us to waste time/space 
doing a large malloc before we finally realize that we can't read that 
many bytes after all.  And in fact, if you check len in advance, you 
can probably just use g_malloc() instead of g_try_malloc() 
(g_try_malloc() makes sense on a 1M allocation, where we can still 
allocate smaller stuff in reporting the error; but since NBD limits 
strings to 4k, if we fail at allocating 4k, we are probably already so 
hosed that our attempts to report the failure will also run out of 
memory and abort, so why not just abort now).


reasonable, will do




+    if (*buf == NULL) {
+    error_setg(errp, "No memory");
+    return -ENOMEM;
+    }
+    (*buf)[len] = '\0';
+
+    ret = nbd_opt_read(client, *buf, len, errp);
+    if (ret <= 0) {
+    g_free(*buf);
+    *buf = NULL;
+    }
+
+    return ret;
+}
+
+/* nbd_read_size_string


Will resume review here...




--
Best regards,
Vladimir

Re: [Qemu-block] [PATCH 3/9] nbd: BLOCK_STATUS for standard get_block_status function: server part

[Eric Blake]

On 02/15/2018 07:51 AM, Vladimir Sementsov-Ogievskiy wrote:

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

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  include/block/nbd.h |  33 ++
  nbd/common.c|  10 ++
  nbd/server.c| 310 +++-
  3 files changed, 352 insertions(+), 1 deletion(-)




@@ -200,9 +227,15 @@ enum {
  #define NBD_REPLY_TYPE_NONE  0
  #define NBD_REPLY_TYPE_OFFSET_DATA   1
  #define NBD_REPLY_TYPE_OFFSET_HOLE   2
+#define NBD_REPLY_TYPE_BLOCK_STATUS  5


Stale; see nbd.git commit 56c77720 which changed this to 3.


+++ b/nbd/server.c
@@ -82,6 +82,15 @@ struct NBDExport {
  
  static QTAILQ_HEAD(, NBDExport) exports = QTAILQ_HEAD_INITIALIZER(exports);
  
+/* NBDExportMetaContexts represents list of selected by

+ * NBD_OPT_SET_META_CONTEXT contexts to be exported. */


represents a list of contexts to be exported, as selected by 
NBD_OPT_SET_META_CONTEXT.



+typedef struct NBDExportMetaContexts {
+char export_name[NBD_MAX_NAME_SIZE + 1];


Would this work as const char * pointing at some other storage, instead 
of having to copy into this storage?



+bool valid; /* means that negotiation of the option finished without
+   errors */
+bool base_allocation; /* export base:allocation context (block status) */
+} NBDExportMetaContexts;
+
  struct NBDClient {



@@ -636,6 +646,201 @@ static QIOChannel 
*nbd_negotiate_handle_starttls(NBDClient *client,
  return QIO_CHANNEL(tioc);
  }
  
+/* nbd_alloc_read_size_string

+ *
+ * Read string in format
+ *   uint32_t len
+ *   len bytes string (not 0-terminated)
+ * String is allocated and pointer returned as @buf
+ *
+ * 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_alloc_read_size_string(NBDClient *client, char **buf,
+  Error **errp)
+{
+int ret;
+uint32_t len;
+
+ret = nbd_opt_read(client, , sizeof(len), errp);
+if (ret <= 0) {
+return ret;
+}
+cpu_to_be32s();
+
+*buf = g_try_malloc(len + 1);


I'd rather check that len is sane prior to trying to malloc.  Otherwise, 
a malicious client can convince us to waste time/space doing a large 
malloc before we finally realize that we can't read that many bytes 
after all.  And in fact, if you check len in advance, you can probably 
just use g_malloc() instead of g_try_malloc() (g_try_malloc() makes 
sense on a 1M allocation, where we can still allocate smaller stuff in 
reporting the error; but since NBD limits strings to 4k, if we fail at 
allocating 4k, we are probably already so hosed that our attempts to 
report the failure will also run out of memory and abort, so why not 
just abort now).



+if (*buf == NULL) {
+error_setg(errp, "No memory");
+return -ENOMEM;
+}
+(*buf)[len] = '\0';
+
+ret = nbd_opt_read(client, *buf, len, errp);
+if (ret <= 0) {
+g_free(*buf);
+*buf = NULL;
+}
+
+return ret;
+}
+
+/* nbd_read_size_string


Will resume review here...

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