[3/3] NBD proto: add "Command flags" section
diff mbox

Message ID 1459161798-32120-4-git-send-email-den@openvz.org
State New
Headers show

Commit Message

Denis V. Lunev March 28, 2016, 10:43 a.m. UTC
From: Pavel Borzenkov <pborzenkov@virtuozzo.com>

Add separate "Command flags" section to make it clear which flags are
currently defined by the protocol.

Signed-off-by: Pavel Borzenkov <pborzenkov@virtuozzo.com>
Reviewed-by: Roman Kagan <rkagan@virtuozzo.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Wouter Verhelst <w@uter.be>
CC: Eric Blake <eblake@redhat.com>
CC: Alex Bligh <alex@alex.org.uk>
---
 doc/proto.md | 10 ++++++++++
 1 file changed, 10 insertions(+)

Comments

Eric Blake March 28, 2016, 1:45 p.m. UTC | #1
On 03/28/2016 04:43 AM, Denis V. Lunev wrote:
> From: Pavel Borzenkov <pborzenkov@virtuozzo.com>
> 
> Add separate "Command flags" section to make it clear which flags are
> currently defined by the protocol.
> 
> Signed-off-by: Pavel Borzenkov <pborzenkov@virtuozzo.com>
> Reviewed-by: Roman Kagan <rkagan@virtuozzo.com>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Wouter Verhelst <w@uter.be>
> CC: Eric Blake <eblake@redhat.com>
> CC: Alex Bligh <alex@alex.org.uk>
> ---
>  doc/proto.md | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/doc/proto.md b/doc/proto.md
> index 036d6d9..662f741 100644
> --- a/doc/proto.md
> +++ b/doc/proto.md
> @@ -485,6 +485,16 @@ The following request types exist:
>      Currently one such message is known: `NBD_CMD_CACHE`, with type set to
>      5, implemented by xnbd.
>  
> +#### Command flags
> +
> +This field of 16 bits is sent by the client with every request and provides
> +additional information to the server to execute the command. Refer to
> +aforementioned "Request types" section for information about the flags

Maybe:

s/aforementioned "Request types" section/the "Request types" section above/

> +supported by particular commands.
> +
> +- bit 0, `NBD_CMD_FLAG_FUA`; should be set to 1 if the client requires
> +  "Force Unit Access" mode of operation

Trailing dot?  Should you also mention which command(s) it is valid
with? (NBD_CMD_WRITE for now, until other extension commands are added)
 It might also be worth mentioning that the flag should not be sent
unless export flags included NBD_FLAG_SEND_FUA.

> +
>  #### Error values
>  
>  The error values are used for the error field in the reply message.
>
Wouter Verhelst March 29, 2016, 7:34 a.m. UTC | #2
On Mon, Mar 28, 2016 at 07:45:27AM -0600, Eric Blake wrote:
> On 03/28/2016 04:43 AM, Denis V. Lunev wrote:
> > From: Pavel Borzenkov <pborzenkov@virtuozzo.com>
> > 
> > Add separate "Command flags" section to make it clear which flags are
> > currently defined by the protocol.
> > 
> > Signed-off-by: Pavel Borzenkov <pborzenkov@virtuozzo.com>
> > Reviewed-by: Roman Kagan <rkagan@virtuozzo.com>
> > Signed-off-by: Denis V. Lunev <den@openvz.org>
> > CC: Wouter Verhelst <w@uter.be>
> > CC: Eric Blake <eblake@redhat.com>
> > CC: Alex Bligh <alex@alex.org.uk>
> > ---
> >  doc/proto.md | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/doc/proto.md b/doc/proto.md
> > index 036d6d9..662f741 100644
> > --- a/doc/proto.md
> > +++ b/doc/proto.md
> > @@ -485,6 +485,16 @@ The following request types exist:
> >      Currently one such message is known: `NBD_CMD_CACHE`, with type set to
> >      5, implemented by xnbd.
> >  
> > +#### Command flags
> > +
> > +This field of 16 bits is sent by the client with every request and provides
> > +additional information to the server to execute the command. Refer to
> > +aforementioned "Request types" section for information about the flags
> 
> Maybe:
> 
> s/aforementioned "Request types" section/the "Request types" section above/
> 
> > +supported by particular commands.
> > +
> > +- bit 0, `NBD_CMD_FLAG_FUA`; should be set to 1 if the client requires
> > +  "Force Unit Access" mode of operation
> 
> Trailing dot?  Should you also mention which command(s) it is valid
> with? (NBD_CMD_WRITE for now, until other extension commands are added)
>  It might also be worth mentioning that the flag should not be sent
> unless export flags included NBD_FLAG_SEND_FUA.
> 
> > +
> >  #### Error values
> >  
> >  The error values are used for the error field in the reply message.

Yes, I agree that these are all (typographical, but still) improvements.
If you can update with that, I'll happily apply that.

Regards,
Eric Blake March 29, 2016, 4:01 p.m. UTC | #3
On 03/28/2016 04:43 AM, Denis V. Lunev wrote:
> From: Pavel Borzenkov <pborzenkov@virtuozzo.com>
> 
> Add separate "Command flags" section to make it clear which flags are
> currently defined by the protocol.
> 
> Signed-off-by: Pavel Borzenkov <pborzenkov@virtuozzo.com>
> Reviewed-by: Roman Kagan <rkagan@virtuozzo.com>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Wouter Verhelst <w@uter.be>
> CC: Eric Blake <eblake@redhat.com>
> CC: Alex Bligh <alex@alex.org.uk>
> ---
>  doc/proto.md | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/doc/proto.md b/doc/proto.md
> index 036d6d9..662f741 100644
> --- a/doc/proto.md
> +++ b/doc/proto.md
> @@ -485,6 +485,16 @@ The following request types exist:
>      Currently one such message is known: `NBD_CMD_CACHE`, with type set to
>      5, implemented by xnbd.
>  
> +#### Command flags
> +

I think that this new content would belong better as a subsection under
'#### Flag Fields', alongside the mention of all other flags.  I'm going
to propose a v2 of this patch with that alternate position, for comparison.
Eric Blake March 29, 2016, 4:03 p.m. UTC | #4
On 03/29/2016 10:01 AM, Eric Blake wrote:
> On 03/28/2016 04:43 AM, Denis V. Lunev wrote:
>> From: Pavel Borzenkov <pborzenkov@virtuozzo.com>
>>
>> Add separate "Command flags" section to make it clear which flags are
>> currently defined by the protocol.
>>
>> Signed-off-by: Pavel Borzenkov <pborzenkov@virtuozzo.com>
>> Reviewed-by: Roman Kagan <rkagan@virtuozzo.com>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> CC: Wouter Verhelst <w@uter.be>
>> CC: Eric Blake <eblake@redhat.com>
>> CC: Alex Bligh <alex@alex.org.uk>
>> ---
>>  doc/proto.md | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
>>
>> diff --git a/doc/proto.md b/doc/proto.md
>> index 036d6d9..662f741 100644
>> --- a/doc/proto.md
>> +++ b/doc/proto.md
>> @@ -485,6 +485,16 @@ The following request types exist:
>>      Currently one such message is known: `NBD_CMD_CACHE`, with type set to
>>      5, implemented by xnbd.
>>  
>> +#### Command flags
>> +
> 
> I think that this new content would belong better as a subsection under
> '#### Flag Fields', alongside the mention of all other flags.  I'm going
> to propose a v2 of this patch with that alternate position, for comparison.

Hmm, maybe not.  I just looked again, and '#### Flag fields' is a
subsection of '### Handshake phase', while you are correct that command
flags belong to '### Transmission phase'.

Patch
diff mbox

diff --git a/doc/proto.md b/doc/proto.md
index 036d6d9..662f741 100644
--- a/doc/proto.md
+++ b/doc/proto.md
@@ -485,6 +485,16 @@  The following request types exist:
     Currently one such message is known: `NBD_CMD_CACHE`, with type set to
     5, implemented by xnbd.
 
+#### Command flags
+
+This field of 16 bits is sent by the client with every request and provides
+additional information to the server to execute the command. Refer to
+aforementioned "Request types" section for information about the flags
+supported by particular commands.
+
+- bit 0, `NBD_CMD_FLAG_FUA`; should be set to 1 if the client requires
+  "Force Unit Access" mode of operation
+
 #### Error values
 
 The error values are used for the error field in the reply message.