Re: [Nbd] [PATCH 1/3] NBD proto: forbid TRIM command without negotiation
- To: Eric Blake <eblake@...696...>
- Cc: nbd-general@lists.sourceforge.net, "Denis V. Lunev" <den@...2317...>, qemu-devel@...530..., Pavel Borzenkov <pborzenkov@...2319...>
- Subject: Re: [Nbd] [PATCH 1/3] NBD proto: forbid TRIM command without negotiation
- From: Wouter Verhelst <w@...112...>
- Date: Tue, 29 Mar 2016 09:22:23 +0200
- Message-id: <20160329072223.GB22386@...3...>
- In-reply-to: <56F92AE1.2040709@...696...>
- References: <1459161798-32120-1-git-send-email-den@...2317...> <1459161798-32120-2-git-send-email-den@...2317...> <56F92AE1.2040709@...696...>
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@...2319...>
> >
> > 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@...2319...>
> > Reviewed-by: Roman Kagan <rkagan@...2319...>
> > Signed-off-by: Denis V. Lunev <den@...2317...>
> > CC: Wouter Verhelst <w@...112...>
> > CC: Eric Blake <eblake@...696...>
> > CC: Alex Bligh <alex@...872...>
> > ---
> > 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@...696...>
--
< ron> I mean, the main *practical* problem with C++, is there's like a dozen
people in the world who think they really understand all of its rules,
and pretty much all of them are just lying to themselves too.
-- #debian-devel, OFTC, 2016-02-12
Reply to: