[Qemu-devel] [PATCH 0/2] NBD fixes before softfreeze

[Qemu-devel] [PATCH 0/2] NBD fixes before softfreeze

[Eric Blake]

Paolo's inclusion of my NBD series in his miscellaneous tree
ended up with a couple of bugs that were pointed out in the
thread after the pull request was first created; these are the
fixes.  They are small enough that I don't mind using maintainer
status to bundle them up as a PULL request for NBD sometime on
Monday (in order to make soft freeze), whether or not they've
been reviewed by then; but of course, review is appreciated.

https://lists.gnu.org/archive/html/qemu-devel/2017-07/msg01983.html
https://lists.gnu.org/archive/html/qemu-devel/2017-07/msg01981.html

Eric Blake (2):
  nbd: Trace client command being sent
  nbd: Fix server reply to NBD_OPT_EXPORT_NAME of older clients

 nbd/client.c     | 3 ++-
 nbd/server.c     | 4 ++--
 nbd/trace-events | 2 +-
 3 files changed, 5 insertions(+), 4 deletions(-)

-- 
2.9.4

[Qemu-devel] [PATCH 1/2] nbd: Trace client command being sent

[Eric Blake]

Make the client trace slightly more legible by including the name
of the command being sent.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 nbd/client.c     | 3 ++-
 nbd/trace-events | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/nbd/client.c b/nbd/client.c
index c3ee9f3..4c12fff 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -902,7 +902,8 @@ ssize_t nbd_send_request(QIOChannel *ioc, NBDRequest *request)
     uint8_t buf[NBD_REQUEST_SIZE];

     trace_nbd_send_request(request->from, request->len, request->handle,
-                           request->flags, request->type);
+                           request->flags, request->type,
+                           nbd_cmd_lookup(request->type));

     stl_be_p(buf, NBD_REQUEST_MAGIC);
     stw_be_p(buf + 4, request->flags);
diff --git a/nbd/trace-events b/nbd/trace-events
index f5024d8..d7c7746 100644
--- a/nbd/trace-events
+++ b/nbd/trace-events
@@ -28,7 +28,7 @@ nbd_client_loop(void) "Doing NBD loop"
 nbd_client_loop_ret(int ret, const char *error) "NBD loop returned %d: %s"
 nbd_client_clear_queue(void) "Clearing NBD queue"
 nbd_client_clear_socket(void) "Clearing NBD socket"
-nbd_send_request(uint64_t from, uint32_t len, uint64_t handle, uint16_t flags, uint16_t type) "Sending request to server: { .from = %" PRIu64", .len = %" PRIu32 ", .handle = %" PRIu64 ", .flags = %" PRIx16 ", .type = %" PRIu16 " }"
+nbd_send_request(uint64_t from, uint32_t len, uint64_t handle, uint16_t flags, uint16_t type, const char *name) "Sending request to server: { .from = %" PRIu64", .len = %" PRIu32 ", .handle = %" PRIu64 ", .flags = %" PRIx16 ", .type = %" PRIu16 " (%s) }"
 nbd_receive_reply(uint32_t magic, int32_t error, uint64_t handle) "Got reply: { magic = 0x%" PRIx32 ", .error = % " PRId32 ", handle = %" PRIu64" }"

 # nbd/server.c
-- 
2.9.4

Re: [Qemu-devel] [Qemu-block] [PATCH 1/2] nbd: Trace client command being sent

[John Snow]

On 07/14/2017 02:32 PM, Eric Blake wrote:
> Make the client trace slightly more legible by including the name
> of the command being sent.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>

Reviewed-by: John Snow <jsnow@redhat.com>

[Qemu-devel] [PATCH 2/2] nbd: Fix server reply to NBD_OPT_EXPORT_NAME of older clients

[Eric Blake]

A typo in commit 23e099c set the size of buf[] used in response
to NBD_OPT_EXPORT_NAME according to the length needed for old-style
negotiation (4 bytes of flag information) instead of the intended
2 bytes used in new style.  If the client doesn't enable
NBD_FLAG_C_NO_ZEROES, then the server sends two bytes too many,
and is then out of sync in response to the client's next command
(the bug is masked when modern qemu is the client, since we enable
the no zeroes flag).

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 nbd/server.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index 49ed574..bcb241c 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -283,7 +283,7 @@ static int nbd_negotiate_handle_export_name(NBDClient *client, uint32_t length,
                                             Error **errp)
 {
     char name[NBD_MAX_NAME_SIZE + 1];
-    char buf[8 + 4 + 124] = "";
+    char buf[8 + 2 + 124] = "";
     size_t len;
     int ret;

@@ -800,7 +800,7 @@ static int nbd_negotiate_options(NBDClient *client, uint16_t myflags,
  */
 static coroutine_fn int nbd_negotiate(NBDClient *client, Error **errp)
 {
-    char buf[8 + 8 + 8 + 128];
+    char buf[8 + 8 + 8 + 2 + 2 + 124];
     int ret;
     const uint16_t myflags = (NBD_FLAG_HAS_FLAGS | NBD_FLAG_SEND_TRIM |
                               NBD_FLAG_SEND_FLUSH | NBD_FLAG_SEND_FUA |
-- 
2.9.4

Re: [Qemu-devel] [Qemu-block] [PATCH 2/2] nbd: Fix server reply to NBD_OPT_EXPORT_NAME of older clients

[John Snow]

On 07/14/2017 02:32 PM, Eric Blake wrote:
> A typo in commit 23e099c set the size of buf[] used in response
> to NBD_OPT_EXPORT_NAME according to the length needed for old-style
> negotiation (4 bytes of flag information) instead of the intended
> 2 bytes used in new style.  If the client doesn't enable
> NBD_FLAG_C_NO_ZEROES, then the server sends two bytes too many,
> and is then out of sync in response to the client's next command
> (the bug is masked when modern qemu is the client, since we enable
> the no zeroes flag).
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  nbd/server.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/nbd/server.c b/nbd/server.c
> index 49ed574..bcb241c 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -283,7 +283,7 @@ static int nbd_negotiate_handle_export_name(NBDClient *client, uint32_t length,
>                                              Error **errp)
>  {
>      char name[NBD_MAX_NAME_SIZE + 1];
> -    char buf[8 + 4 + 124] = "";
> +    char buf[8 + 2 + 124] = "";

Is it worth naming these values with some kind of constant to help
describe what the components are? Or at least documenting nearby what
these pieces are? It's clearly written to help illustrate the length of
segments of the reply, but I'm not sure what the segments are.

Also, do we have a policy on how much stuff we're allowed to put on the
stack? I know we've been moving some larger allocations off, but I don't
know what the rule of thumb is.

>      size_t len;
>      int ret;
> 
> @@ -800,7 +800,7 @@ static int nbd_negotiate_options(NBDClient *client, uint16_t myflags,
>   */
>  static coroutine_fn int nbd_negotiate(NBDClient *client, Error **errp)
>  {
> -    char buf[8 + 8 + 8 + 128];
> +    char buf[8 + 8 + 8 + 2 + 2 + 124];

I assume this is a semantic fix instead of a logical one, but I don't
know exactly what or why.

>      int ret;
>      const uint16_t myflags = (NBD_FLAG_HAS_FLAGS | NBD_FLAG_SEND_TRIM |
>                                NBD_FLAG_SEND_FLUSH | NBD_FLAG_SEND_FUA |
> 

Sincerely, someone who has never read our NBD code before.

Re: [Qemu-devel] [Qemu-block] [PATCH 2/2] nbd: Fix server reply to NBD_OPT_EXPORT_NAME of older clients

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 4569 bytes --]

On 07/17/2017 01:27 PM, John Snow wrote:
> 
> 
> On 07/14/2017 02:32 PM, Eric Blake wrote:
>> A typo in commit 23e099c set the size of buf[] used in response
>> to NBD_OPT_EXPORT_NAME according to the length needed for old-style
>> negotiation (4 bytes of flag information) instead of the intended
>> 2 bytes used in new style.  If the client doesn't enable
>> NBD_FLAG_C_NO_ZEROES, then the server sends two bytes too many,
>> and is then out of sync in response to the client's next command
>> (the bug is masked when modern qemu is the client, since we enable
>> the no zeroes flag).
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> ---
>>  nbd/server.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/nbd/server.c b/nbd/server.c
>> index 49ed574..bcb241c 100644
>> --- a/nbd/server.c
>> +++ b/nbd/server.c
>> @@ -283,7 +283,7 @@ static int nbd_negotiate_handle_export_name(NBDClient *client, uint32_t length,
>>                                              Error **errp)
>>  {
>>      char name[NBD_MAX_NAME_SIZE + 1];
>> -    char buf[8 + 4 + 124] = "";
>> +    char buf[8 + 2 + 124] = "";
> 
> Is it worth naming these values with some kind of constant to help
> describe what the components are? Or at least documenting nearby what
> these pieces are? It's clearly written to help illustrate the length of
> segments of the reply, but I'm not sure what the segments are.

Good question.  In nbd/nbd_internal.h, we have some:

/* Size of all NBD_OPT_*, without payload */
#define NBD_REQUEST_SIZE        (4 + 2 + 2 + 8 + 8 + 4)
/* Size of all NBD_REP_* sent in answer to most NBD_OPT_*, without
payload */
#define NBD_REPLY_SIZE          (4 + 4 + 8)

But this is in response to NBD_OPT_EXPORT_NEXT, which is different from
all other NBD_OPT_* (hence the "most" in the comment above).  I don't
know if buf[sizeof(uint64_t) + sizeof(uint16_t) + sizeof(int[124])] is
any cleaner to read, but at least having another defined constant in
nbd_internal.h alongside the existing constants can't hurt.

Either way, the length is broken out to match the code below, which is
populating it via:

    stq_be_p(buf, client->exp->size);
    stw_be_p(buf + 8, client->exp->nbdflags | myflags);
    len = no_zeroes ? 10 : sizeof(buf);

(that is, 8 bytes size, 2 bytes flags, then 124 bytes of NUL)

> 
> Also, do we have a policy on how much stuff we're allowed to put on the
> stack? I know we've been moving some larger allocations off, but I don't
> know what the rule of thumb is.

Rule of thumb is that anything larger than 4k makes it too easy to
accidentally miss the guard page, and should be reworked to use other
means.  Stuff under 1k, like this, is no problem.

> 
>>      size_t len;
>>      int ret;
>>
>> @@ -800,7 +800,7 @@ static int nbd_negotiate_options(NBDClient *client, uint16_t myflags,
>>   */
>>  static coroutine_fn int nbd_negotiate(NBDClient *client, Error **errp)
>>  {
>> -    char buf[8 + 8 + 8 + 128];
>> +    char buf[8 + 8 + 8 + 2 + 2 + 124];
> 
> I assume this is a semantic fix instead of a logical one, but I don't
> know exactly what or why.

Yes, semantic fix.  The code below populates this as:

    memset(buf, 0, sizeof(buf));
    memcpy(buf, "NBDMAGIC", 8);
...
        stq_be_p(buf + 8, NBD_CLIENT_MAGIC);
        stq_be_p(buf + 16, client->exp->size);
        stw_be_p(buf + 26, client->exp->nbdflags | myflags);

(that is, 8 bytes general magic, 8 bytes old-style magic, 8 bytes size,
2 bytes left 0, 2 bytes of flags [hmm, the code could have used
stl_be_p(buf + 24, flags...) with no semantic change], then 124 bytes of
NUL)

If the spec helps,
https://github.com/NetworkBlockDevice/nbd/blob/master/doc/proto.md,
search for Oldstyle negotiation, then search for the two instances of
the string "size of the export in bytes" (second hit is the newstyle
counterpart, covering the first part of this patch).

> 
>>      int ret;
>>      const uint16_t myflags = (NBD_FLAG_HAS_FLAGS | NBD_FLAG_SEND_TRIM |
>>                                NBD_FLAG_SEND_FLUSH | NBD_FLAG_SEND_FUA |
>>
> 
> Sincerely, someone who has never read our NBD code before.
> 

Your pre-freeze reviews are much appreciated, even if it is in areas
where you are less familiar.  Thanks!

Sounds like I'll post a v2 with some magic numbers hoisted to the header
for consistency.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]