| Message ID | 1459161798-32120-2-git-send-email-den@openvz.org |
|---|---|
| State | New |
| Headers | show |
On 03/28/2016 04:43 AM, Denis V. Lunev wrote: > From: Pavel Borzenkov <pborzenkov@virtuozzo.com> > > There is a loophole in the protocol that allows a client to send TRIM > request even if support for it wasn't negotiated with the server. State > explicitly that the client MUST NOT send such command without prior > successful negotiation. > > 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 | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/doc/proto.md b/doc/proto.md > index 6d1cb34..d54ed19 100644 > --- a/doc/proto.md > +++ b/doc/proto.md > @@ -471,6 +471,9 @@ The following request types exist: > about the contents of the export affected by this command, until > overwriting it again with `NBD_CMD_WRITE`. > > + A client MUST NOT send a trim request unless `NBD_FLAG_SEND_TRIM` > + was set in the export flags field. > + Do we also want to mention that the server SHOULD fail with EINVAL if the client sends it anyway, and similarly if NBD_CMD_FLUSH was sent without the appropriate export flag (but that the client should not rely on that particular failure)? But as this is a strict improvement, Reviewed-by: Eric Blake <eblake@redhat.com>
On Mon, Mar 28, 2016 at 07:00:17AM -0600, Eric Blake wrote: > On 03/28/2016 04:43 AM, Denis V. Lunev wrote: > > From: Pavel Borzenkov <pborzenkov@virtuozzo.com> > > > > There is a loophole in the protocol that allows a client to send TRIM > > request even if support for it wasn't negotiated with the server. State > > explicitly that the client MUST NOT send such command without prior > > successful negotiation. > > > > 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 | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/doc/proto.md b/doc/proto.md > > index 6d1cb34..d54ed19 100644 > > --- a/doc/proto.md > > +++ b/doc/proto.md > > @@ -471,6 +471,9 @@ The following request types exist: > > about the contents of the export affected by this command, until > > overwriting it again with `NBD_CMD_WRITE`. > > > > + A client MUST NOT send a trim request unless `NBD_FLAG_SEND_TRIM` > > + was set in the export flags field. > > + > > Do we also want to mention that the server SHOULD fail with EINVAL if > the client sends it anyway, and similarly if NBD_CMD_FLUSH was sent > without the appropriate export flag (but that the client should not rely > on that particular failure)? I think the protocol should mention that the server MAY fail with EINVAL, rather than SHOULD. Rationale: the robusness principle -- if you didn't negotiate it, you may end up with a server who doesn't know about the feature; but if it just so happens that the server does know about it even though you didn't negotiate it, there is little harm in it following up on the request. > But as this is a strict improvement, > Reviewed-by: Eric Blake <eblake@redhat.com>
On 03/29/2016 01:22 AM, Wouter Verhelst wrote: >>> +++ b/doc/proto.md >>> @@ -471,6 +471,9 @@ The following request types exist: >>> about the contents of the export affected by this command, until >>> overwriting it again with `NBD_CMD_WRITE`. >>> >>> + A client MUST NOT send a trim request unless `NBD_FLAG_SEND_TRIM` >>> + was set in the export flags field. >>> + >> >> Do we also want to mention that the server SHOULD fail with EINVAL if >> the client sends it anyway, and similarly if NBD_CMD_FLUSH was sent >> without the appropriate export flag (but that the client should not rely >> on that particular failure)? > > I think the protocol should mention that the server MAY fail with > EINVAL, rather than SHOULD. Rationale: the robusness principle -- if you > didn't negotiate it, you may end up with a server who doesn't know about > the feature; but if it just so happens that the server does know about it even > though you didn't negotiate it, there is little harm in it following up on the > request. Good point; furthermore, a server is compliant if it accepts and ignores NBD_CMD_TRIM (as that command is only a hint); which is different from NBD_CMD_FLUSH (which must ensure a barrier).
diff --git a/doc/proto.md b/doc/proto.md index 6d1cb34..d54ed19 100644 --- a/doc/proto.md +++ b/doc/proto.md @@ -471,6 +471,9 @@ The following request types exist: about the contents of the export affected by this command, until overwriting it again with `NBD_CMD_WRITE`. + A client MUST NOT send a trim request unless `NBD_FLAG_SEND_TRIM` + was set in the export flags field. + * Other requests Some third-party implementations may require additional protocol