Re: [Libguestfs] [PATCH] docs: Prefer 'cookie' over 'handle'
On Sat, Mar 04, 2023 at 10:03:46PM +0200, Nir Soffer wrote:
> On Sat, Mar 4, 2023 at 12:15 AM Eric Blake wrote:
> > Makes no difference to implementations (other than older code
> > still using 'handle' may be slightly harder to tie back to the spec).
>
> To avoid confusion with older code that carefully used "handle" to match
> the spec, maybe add a note that "cookie" was named "handle" before?
Yes, this. I'm happy with renaming it cookie (it makes sense), but there
is a *lot* of stuff out there that calls it "handle" (including a
wireshark plugin) and it would be confusing if that link isn't available
anywhere.
--
w@uter.{be,co.za}
wouter@{grep.be,fosdem.org,debian.org}
I will have a Tin-Actinium-Potassium mixture, thanks.
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [PATCH v2 5/6] spec: Introduce NBD_FLAG_BLOCK_STATUS_PAYLOAD
On Fri, Mar 03, 2023 at 04:40:38PM -0600, Eric Blake wrote:
> On Wed, Feb 22, 2023 at 12:05:44PM +0200, Wouter Verhelst wrote:
> > On Mon, Nov 14, 2022 at 04:46:54PM -0600, Eric Blake wrote:
> > > Simple reply message
> > >
> > > @@ -1232,6 +1235,19 @@ The field has the following format:
> > >will be faster than a regular write). Clients MUST NOT set the
> > >`NBD_CMD_FLAG_FAST_ZERO` request flag unless this transmission flag
> > >is set.
> > > +- bit 12, `NBD_FLAG_BLOCK_STATUS_PAYLOAD`: Indicates that the server
> > > + understands the use of the `NBD_CMD_FLAG_PAYLOAD_LEN` flag to
> > > + `NBD_CMD_BLOCK_STATUS` to allow the client to request that the
> > > + server filters its response to a specific subset of negotiated
> > > + metacontext ids passed in via a client payload, rather than the
> > > + default of replying to all metacontext ids. Servers MUST NOT
> > > + advertise this bit unless the client successfully negotiates
> > > + extended headers via `NBD_OPT_EXTENDED_HEADERS`, and SHOULD NOT
> > > + advertise this bit in response to `NBD_OPT_EXPORT_NAME` or
> > > + `NBD_OPT_GO` if the client does not negotiate metacontexts with
> > > + `NBD_OPT_SET_META_CONTEXT`; clients SHOULD NOT set the
> > > + `NBD_CMD_FLAG_PAYLOAD_LEN` flag for `NBD_CMD_BLOCK_STATUS` unless
> > > + this transmission flag is set.
> >
> > Given that we are introducing this at the same time as the extended
> > headers (and given that we're running out of available flags in this
> > 16-bit field), isn't it better to make the ability to understand
> > PAYLOAD_LEN be implied by extended headers? Or is there a use case for
> > implementing extended headers but not a payload length on
> > CMD_BLOCK_STATUS that I'm missing?
>
> In my proof of implementation, this was a distinct feature addition on
> top of 64-bit headers.
>
> It is easy to write a server that does not want to deal with a client
> payload on a block status request (for example, a server that only
> knows about one metacontext, and therefore never needs to deal with a
> client wanting a subset of negotiated metacontexts). Forcing a server
> to have to support a client-side filtering request on block status
> commands, merely because the server now supports 64-bit lengths,
> seemed like a stretch too far, which is why I decided to use a feature
> bit for this one.
Okay, yes, that makes sense. Thanks.
--
w@uter.{be,co.za}
wouter@{grep.be,fosdem.org,debian.org}
I will have a Tin-Actinium-Potassium mixture, thanks.
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [PATCH v2 3/6] spec: Add NBD_OPT_EXTENDED_HEADERS
On Fri, Mar 03, 2023 at 04:36:41PM -0600, Eric Blake wrote:
> On Wed, Feb 22, 2023 at 11:49:18AM +0200, Wouter Verhelst wrote:
> > On Mon, Nov 14, 2022 at 04:46:52PM -0600, Eric Blake wrote:
[...]
> > > + Note that even when extended headers are in use, the client MUST be
> > > + prepared for the server to use either the compact or extended chunk
> > > + type, regardless of whether the client's hinted effect length was
> > > + more or less than 32 bits; but the server MUST use exactly one of
> > > + the two chunk types per negotiated metacontext ID.
> >
> > Is this last paragraph really a good idea? I would think it makes more
> > sense to require the new format if we're already required to support it
> > on both sides anyway.
>
> My proof of implementation was easier to code when I didn't have to
> resize the block status reply sizing in the same patch as adding the
> 64-bit headers. But if you think requiring the 64-bit reply type
> always (and forbidding the 32-bit reply) when extended headers are in
> force, that's also possible.
Intuitively, this sounds off. It would seem to me that it's easier to do
if you don't have to have a conditional on each received data packet.
But maybe I'm missing something -- I haven't done an implementation yet,
anyway.
--
w@uter.{be,co.za}
wouter@{grep.be,fosdem.org,debian.org}
I will have a Tin-Actinium-Potassium mixture, thanks.
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [PATCH v2 2/6] spec: Tweak description of maximum block size
On Fri, Mar 03, 2023 at 04:26:53PM -0600, Eric Blake wrote: > On Tue, Feb 21, 2023 at 05:21:37PM +0200, Wouter Verhelst wrote: > > Hi Eric, > > > > Busy days, busy times. Sorry about the insane delays here. > > No problem; I've been tackling other things in the meantime too, so > this extension has taken far longer than I planned for more reasons > than just slow review time. > > > > > On Mon, Nov 14, 2022 at 04:46:51PM -0600, Eric Blake wrote: > > > Commit 9f30fedb improved the spec to allow non-payload requests that > > > exceed any advertised maximum block size. Take this one step further > > > by permitting the server to use NBD_EOVERFLOW as a hint to the client > > > when a request is oversize (while permitting NBD_EINVAL for > > > back-compat), and by rewording the text to explicitly call out that > > > what is being advertised is the maximum payload length, not maximum > > > block size. This becomes more important when we add 64-bit > > > extensions, where it becomes possible to extend `NBD_CMD_BLOCK_STATUS` > > > to have both an effect length (how much of the image does the client > > > want status on - may be larger than 32 bits) and an optional payload > > > length (a way to filter the response to a subset of negotiated > > > metadata contexts). In the shorter term, it means that a server may > > > (but not must) accept a read request larger than the maximum block > > > size if it can use structured replies to keep each chunk of the > > > response under the maximum payload limits. > > > --- > > > doc/proto.md | 127 +-- > > > 1 file changed, 73 insertions(+), 54 deletions(-) > > > > > > diff --git a/doc/proto.md b/doc/proto.md > > > index 8f08583..53c334a 100644 > > > --- a/doc/proto.md > > > +++ b/doc/proto.md > > > @@ -745,8 +745,8 @@ text unless the client insists on TLS. > > > > > > During transmission phase, several operations are constrained by the > > > export size sent by the final `NBD_OPT_EXPORT_NAME` or `NBD_OPT_GO`, > > > -as well as by three block size constraints defined here (minimum, > > > -preferred, and maximum). > > > +as well as by three block size constraints defined here (minimum > > > +block, preferred block, and maximum payload). > > > > > > If a client can honour server block size constraints (as set out below > > > and under `NBD_INFO_BLOCK_SIZE`), it SHOULD announce this during the > > > @@ -772,15 +772,15 @@ learn the server's constraints without committing > > > to them. > > > > > > If block size constraints have not been advertised or agreed on > > > externally, then a server SHOULD support a default minimum block size > > > -of 1, a preferred block size of 2^12 (4,096), and a maximum block size > > > -that is effectively unlimited (0x, or the export size if that > > > -is smaller), while a client desiring maximum interoperability SHOULD > > > -constrain its requests to a minimum block size of 2^9 (512), and limit > > > -`NBD_CMD_READ` and `NBD_CMD_WRITE` commands to a maximum block size of > > > -2^25 (33,554,432). A server that wants to enforce block sizes other > > > -than the defaults specified here MAY refuse to go into transmission > > > -phase with a client that uses `NBD_OPT_EXPORT_NAME` (via a hard > > > -disconnect) or which uses `NBD_OPT_GO` without requesting > > > +of 1, a preferred block size of 2^12 (4,096), and a maximum block > > > +payload size that is at least 2^25 (33,554,432) (even if the export > > > +size is smaller); while a client desiring maximum interoperability > > > +SHOULD constrain its requests to a minimum block size of 2^9 (512), > > > +and limit `NBD_CMD_READ` and `NBD_CMD_WRITE` commands to a maximum > > > +block size of 2^25 (33,554,432). A server that wants to enforce block > > > +sizes other than the defaults specified here MAY refuse to go into > > > +transmission phase with a client that uses `NBD_OPT_EXPORT_NAME` (via > > > +a hard disconnect) or which uses `NBD_OPT_GO` without requesting > > > > This does more than what the commit message says: it also limits payload > > size from 0x to 2^25. We already have a "A server that desires > > maximum interoperability" clause that mentions the 2^25, so I'm not > > entirely sure why we need to restrict that for the default cause. > > > > Remember, apart from specifying how something should be implemented, the > > spec also documents current and historic behavior. I am probably > > convinced that it makes more sense to steer people towards limiting to > > 2^25, but it should be done in such a way that servers which accept a > > 0x block size are not suddenly noncompliant. I don't think this > > does that. > > I'll have to play more with the wording here. A client shouldn't send > larger write requests to a server without knowing the server will > accept it (because traditional servers disconnect instead - the > alternative is to read the client's entire payload, and the larger the >
Re: [Libguestfs] [PATCH v2 1/6] spec: Recommend cap on NBD_REPLY_TYPE_BLOCK_STATUS length
On Fri, Mar 03, 2023 at 04:17:40PM -0600, Eric Blake wrote:
> On Fri, Dec 16, 2022 at 10:32:01PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > s-o-b line missed.
>
> I'm not sure if the NBD project has a strict policy on including one,
> but I don't mind adding it.
I've never required it, mostly because it's something that I myself
always forget, too, so, *shrug*.
(if there were a way in git to make it add that automatically, that
would help; I've looked but haven't found it)
--
w@uter.{be,co.za}
wouter@{grep.be,fosdem.org,debian.org}
I will have a Tin-Actinium-Potassium mixture, thanks.
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
