Re: [Qemu-block] [PATCH 03/18] nbd: Minimal structured read for server

[Paolo Bonzini]

On 07/02/2017 00:01, Eric Blake wrote:
> On 02/03/2017 09:47 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Minimal implementation of structured read: one data chunk + finishing
>> none chunk. No segmentation.
>> Minimal structured error implementation: no text message.
>> Support DF flag, but just ignore it, as there is no segmentation any
>> way.
> 
> Might be worth adding that this is still an experimental extension to
> the NBD spec, and therefore that this implementation serves as proof of
> concept and may still need tweaking if anything major turns up before
> promoting it to stable.  It might also be worth a link to:
> 
> https://github.com/NetworkBlockDevice/nbd/blob/extension-structured-reply/doc/proto.md

Wouter's slides from FOSDEM said the state is "discussion complete, not
yet implemented".

Paolo

>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
>> ---
>>  include/block/nbd.h |  31 +++++++++++++
>>  nbd/nbd-internal.h  |   2 +
>>  nbd/server.c        | 125 
>> ++++++++++++++++++++++++++++++++++++++++++++++++++--
>>  3 files changed, 154 insertions(+), 4 deletions(-)
>>
>> diff --git a/include/block/nbd.h b/include/block/nbd.h
>> index 3c65cf8d87..58b864f145 100644
>> --- a/include/block/nbd.h
>> +++ b/include/block/nbd.h
>> @@ -70,6 +70,25 @@ struct NBDSimpleReply {
>>  };
>>  typedef struct NBDSimpleReply NBDSimpleReply;
>>  
>> +typedef struct NBDStructuredReplyChunk {
>> +    uint32_t magic;
>> +    uint16_t flags;
>> +    uint16_t type;
>> +    uint64_t handle;
>> +    uint32_t length;
>> +} QEMU_PACKED NBDStructuredReplyChunk;
>> +
>> +typedef struct NBDStructuredRead {
>> +    NBDStructuredReplyChunk h;
>> +    uint64_t offset;
>> +} QEMU_PACKED NBDStructuredRead;
>> +
>> +typedef struct NBDStructuredError {
>> +    NBDStructuredReplyChunk h;
>> +    uint32_t error;
>> +    uint16_t message_length;
>> +} QEMU_PACKED NBDStructuredError;
> 
> Definitely a subset of all types added in the NBD protocol extension,
> but reasonable for a minimal implementation.  Might be worth adding
> comments to the types...
> 
>>  
>> +/* Structured reply flags */
>> +#define NBD_REPLY_FLAG_DONE 1
>> +
>> +/* Structured reply types */
>> +#define NBD_REPLY_TYPE_NONE 0
>> +#define NBD_REPLY_TYPE_OFFSET_DATA 1
>> +#define NBD_REPLY_TYPE_OFFSET_HOLE 2
>> +#define NBD_REPLY_TYPE_ERROR ((1 << 15) + 1)
>> +#define NBD_REPLY_TYPE_ERROR_OFFSET ((1 << 15) + 2)
> 
> ...that correspond to these constants that will be used in the [h.]type
> field.
> 
> Also, it's a bit odd that you are defining constants that aren't
> implemented here; I don't know if it is any cleaner to save the
> definition for the unimplemented types until you actually implement them
> (NBD_REPLY_TYPE_OFFSET_HOLE, NBD_REPLY_TYPE_ERROR_OFFSET).
> 
>> +++ b/nbd/nbd-internal.h
>> @@ -60,6 +60,7 @@
>>  #define NBD_REPLY_SIZE          (4 + 4 + 8)
>>  #define NBD_REQUEST_MAGIC       0x25609513
>>  #define NBD_SIMPLE_REPLY_MAGIC  0x67446698
>> +#define NBD_STRUCTURED_REPLY_MAGIC 0x668e33ef
>>  #define NBD_OPTS_MAGIC          0x49484156454F5054LL
>>  #define NBD_CLIENT_MAGIC        0x0000420281861253LL
>>  #define NBD_REP_MAGIC           0x0003e889045565a9LL
> 
> I would not be bothered if you wanted to reindent the other lines by 3
> spaces so that all the macro definitions start on the same column.  But
> I also won't require it.
> 
>> @@ -81,6 +82,7 @@
>>  #define NBD_OPT_LIST            (3)
>>  #define NBD_OPT_PEEK_EXPORT     (4)
>>  #define NBD_OPT_STARTTLS        (5)
>> +#define NBD_OPT_STRUCTURED_REPLY (8)
> 
> Similar comments about consistency in the definition column.
> 
>> +++ b/nbd/server.c
>> @@ -100,6 +100,8 @@ struct NBDClient {
>>      QTAILQ_ENTRY(NBDClient) next;
>>      int nb_requests;
>>      bool closing;
>> +
>> +    bool structured_reply;
>>  };
>>  
>>  /* That's all folks */
>> @@ -573,6 +575,16 @@ static int nbd_negotiate_options(NBDClient *client)
>>                      return ret;
>>                  }
>>                  break;
>> +
>> +            case NBD_OPT_STRUCTURED_REPLY:
>> +                client->structured_reply = true;
>> +                ret = nbd_negotiate_send_rep(client->ioc, NBD_REP_ACK,
>> +                                             clientflags);
>> +                if (ret < 0) {
>> +                    return ret;
>> +                }
>> +                break;
>> +
> 
> As written, you allow the client to negotiate this more than once.  On
> the one hand, we are idempotent, so it doesn't hurt if they do so; on
> the other hand, it is a waste of bandwidth, and a client could abuse it
> by sending an infinite stream of NBD_OPT_STRUCTURED_REPLY requests and
> never moving into transmission phase, which is a mild form of
> Denial-of-Service (they're hogging a socket from accomplishing useful
> work for some other client).  It would be acceptable if we wanted to
> disconnect any client that sends this option more than once, although
> the NBD spec does not require us to do so.  Up to you if you think
> that's worth adding.
> 
>>  
>> +static void set_be_chunk(NBDStructuredReplyChunk *chunk, uint16_t flags,
>> +                         uint16_t type, uint64_t handle, uint32_t length)
> 
> I'm not sure I like the name of this helper. I know what you are doing:
> go from native-endian local variables into network-byte-order storage in
> preparation for transmission, done at the last possible moment.  But I
> also don't know if I have a good suggestion for a better name off hand.
> 
>> +{
>> +    stl_be_p(&chunk->magic, NBD_STRUCTURED_REPLY_MAGIC);
>> +    stw_be_p(&chunk->flags, flags);
>> +    stw_be_p(&chunk->type, type);
>> +    stq_be_p(&chunk->handle, handle);
>> +    stl_be_p(&chunk->length, length);
>> +}
>> +
>> +static int nbd_co_send_iov(NBDClient *client, struct iovec *iov, unsigned 
>> niov)
> 
> Probably should add the coroutine_fn annotation to this function and its
> friends (yeah, the NBD code doesn't consistently use it yet, but it should).
> 
>> @@ -1147,7 +1239,8 @@ static ssize_t nbd_co_receive_request(NBDRequestData 
>> *req,
>>          rc = request->type == NBD_CMD_WRITE ? -ENOSPC : -EINVAL;
>>          goto out;
>>      }
>> -    if (request->flags & ~(NBD_CMD_FLAG_FUA | NBD_CMD_FLAG_NO_HOLE)) {
>> +    if (request->flags & ~(NBD_CMD_FLAG_FUA | NBD_CMD_FLAG_NO_HOLE |
>> +                           NBD_CMD_FLAG_DF)) {
>>          LOG("unsupported flags (got 0x%x)", request->flags);
>>          rc = -EINVAL;
>>          goto out;
> 
> Missing a check that NBD_CMD_FLAG_DF is only set for NBD_CMD_READ (it is
> not valid on any other command, at least in the current version of the
> extension specification).
> 
>> @@ -1444,6 +1559,8 @@ void nbd_client_new(NBDExport *exp,
>>      client->can_read = true;
>>      client->close = close_fn;
>>  
>> +    client->structured_reply = false;
> 
> Dead assignment, since we used 'client = g_malloc0()' above.
> 
> Overall looks like it matches the spec.
>

signature.asc
Description: OpenPGP digital signature