Re: [Libguestfs] [PATCH] docs: Prefer 'cookie' over 'handle'

[Laszlo Ersek]

On 3/3/23 23:15, Eric Blake wrote:
> In libnbd, we quickly learned that distinguishing between 'handle'
> (verb for acting on an object) and 'handle' (noun describing which
> object to act on) could get confusing; we solved it by renaming the
> latter to 'cookie'.  Copy that approach into the NBD spec, and make it
> obvious that a cookie is opaque data from the point of view of the
> server.  Makes no difference to implementations (other than older code
> still using 'handle' may be slightly harder to tie back to the spec).
> 
> Suggested-by: Richard W.M. Jones 
> Signed-off-by: Eric Blake 
> ---
>  doc/proto.md | 18 +-
>  1 file changed, 9 insertions(+), 9 deletions(-)

ugh, the subject prefix does not name the project this patch is for, and
seeing it through the libguestfs mailing list confused me...

Is this for
?

Anyway, from a superficial reading, this seems certainly sane. I've
known "cookie" from similar (but independent) contexts, and I agree it's
superior to "handle" (noun).

> 
> diff --git a/doc/proto.md b/doc/proto.md
> index 3a96703..abb23e8 100644
> --- a/doc/proto.md
> +++ b/doc/proto.md
> @@ -301,11 +301,11 @@ may be handled by the server asynchronously), and 
> structured reply
>  chunks from one request may be interleaved with reply messages from
>  other requests; however, there may be constraints that prevent
>  arbitrary reordering of structured reply chunks within a given reply.
> -Clients SHOULD use a handle that is distinct from all other currently
> -pending transactions, but MAY reuse handles that are no longer in
> -flight; handles need not be consecutive.  In each reply message
> +Clients SHOULD use a cookie that is distinct from all other currently
> +pending transactions, but MAY reuse cookies that are no longer in
> +flight; cookies need not be consecutive.  In each reply message
>  (whether simple or structured), the server MUST use the same value for
> -handle as was sent by the client in the corresponding request.  In
> +cookie as was sent by the client in the corresponding request.  In
>  this way, the client can correlate which request is receiving a
>  response.
> 
> @@ -349,7 +349,7 @@ The request message, sent by the client, looks as follows:
>  C: 32 bits, 0x25609513, magic (`NBD_REQUEST_MAGIC`)  
>  C: 16 bits, command flags  
>  C: 16 bits, type  
> -C: 64 bits, handle  
> +C: 64 bits, cookie  
>  C: 64 bits, offset (unsigned)  
>  C: 32 bits, length (unsigned)  
>  C: (*length* bytes of data if the request is of type `NBD_CMD_WRITE`)  
> @@ -366,7 +366,7 @@ follows:
>  S: 32 bits, 0x67446698, magic (`NBD_SIMPLE_REPLY_MAGIC`; used to be
> `NBD_REPLY_MAGIC`)  
>  S: 32 bits, error (MAY be zero)  
> -S: 64 bits, handle  
> +S: 64 bits, cookie  
>  S: (*length* bytes of data if the request is of type `NBD_CMD_READ` and
>  *error* is zero)  
> 
> @@ -381,7 +381,7 @@ server must initiate a hard disconnect).  Second, there 
> is no way to
>  efficiently skip over portions of a sparse file that are known to
>  contain all zeroes.  Finally, it is not possible to reliably decode
>  the server traffic without also having context of what pending read
> -requests were sent by the client, to see which *handle* values will
> +requests were sent by the client, to see which *cookie* values will
>  have accompanying payload on success.  Therefore structured replies
>  are also permitted if negotiated.
> 
> @@ -398,7 +398,7 @@ sending errors via a structured reply, as the error can 
> then be
>  accompanied by a string payload to present to a human user.
> 
>  A structured reply MAY occupy multiple structured chunk messages
> -(all with the same value for "handle"), and the
> +(all with the same value for "cookie"), and the
>  `NBD_REPLY_FLAG_DONE` reply flag is used to identify the final
>  chunk.  Unless further documented by individual requests below,
>  the chunks MAY be sent in any order, except that the chunk with
> @@ -418,7 +418,7 @@ A structured reply chunk message looks as follows:
>  S: 32 bits, 0x668e33ef, magic (`NBD_STRUCTURED_REPLY_MAGIC`)  
>  S: 16 bits, flags  
>  S: 16 bits, type  
> -S: 64 bits, handle  
> +S: 64 bits, cookie  
>  S: 32 bits, length of payload (unsigned)  
>  S: *length* bytes of payload data (if *length* is nonzero)  
> 

Acked-by: Laszlo Ersek 

Laszlo

___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [nbdkit PATCH 0/9] common: catch up with libnbd

[Laszlo Ersek]

On 3/3/23 20:18, Eric Blake wrote:
> On Fri, Mar 03, 2023 at 07:00:16PM +, Richard W.M. Jones wrote:
>>
>> This series looks, great, thanks!
>>
>> ACK
> 
> ACK from me as well; I didn't spot any problems in my read through.
> 

Thank you both for reviewing this; commit range 648a7909b69e..e36cfb6fa4f8.

Laszlo
___
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

[Eric Blake]

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.


-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org
___
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

[Eric Blake]

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:
> [...]
> > @@ -1370,9 +1475,10 @@ of the newstyle negotiation.
> >  Return a list of `NBD_REP_META_CONTEXT` replies, one per context,
> >  followed by an `NBD_REP_ACK` or an error.
> > 
> > -This option SHOULD NOT be requested unless structured replies have
> > -been negotiated first. If a client attempts to do so, a server
> > -MAY send `NBD_REP_ERR_INVALID`.
> > +This option SHOULD NOT be requested unless structured replies or
> > +extended headers have been negotiated first. If a client attempts
> > +to do so, a server MAY send `NBD_REP_ERR_INVALID` or
> > +`NBD_REP_ERR_EXT_HEADER_REQD`.
> 
> Is it the intent that NBD_REP_ERR_EXT_HEADER_REQD means structured
> replies are not supported by this server? I think that could be
> clarified here.

My intent here was that a newer server that ONLY wants to talk to
clients that understand extended headers can use
NBD_REP_ERR_EXT_HEADER_REQD as its error message to clients that have
not requested extended headers yet.  Older clients may not know what
that error code means, but our spec already has reasonable constraints
that the client should at least recognize it as an error code.

That way, a server only has to implement extended headers, rather than
maintaining the structured header code in parallel.

> 
> (this occurs twice)
> 
> [...]
> > +* `NBD_OPT_EXTENDED_HEADERS` (11)
> > +
> > +The client wishes to use extended headers during the transmission
> > +phase.  The client MUST NOT send any additional data with the
> > +option, and the server SHOULD reject a request that includes data
> > +with `NBD_REP_ERR_INVALID`.
> > +
> > +When successful, this option takes precedence over structured
> > +replies.  A client MAY request structured replies first, although
> > +a server SHOULD support this option even if structured replies are
> > +not negotiated.
> > +
> > +It is envisioned that future extensions will add other new
> > +requests that support a data payload in the request or reply.  A
> > +server that supports such extensions SHOULD NOT advertise those
> > +extensions until the client has negotiated extended headers; and a
> > +client MUST NOT make use of those extensions without first
> > +enabling support for reply payloads.
> > +
> > +The server replies with the following, or with an error permitted
> > +elsewhere in this document:
> > +
> > +- `NBD_REP_ACK`: Extended headers have been negotiated; the client
> > +  MUST use the 32-byte extended request header, with proper use of
> > +  `NBD_CMD_FLAG_PAYLOAD_LEN` for all commands sending a payload;
> > +  and the server MUST use the 32-byte extended reply header.
> > +- For backwards compatibility, clients SHOULD be prepared to also
> > +  handle `NBD_REP_ERR_UNSUP`; in this case, only the compact
> > +  transmission headers will be used.
> > +
> > +Note that a response of `NBD_REP_ERR_BLOCK_SIZE_REQD` does not
> > +make sense in response to this command, but a server MAY fail with
> > +that error for a later `NBD_OPT_GO` without a client request for
> > +`NBD_INFO_BLOCK_SIZE`, since the use of extended headers provides
> > +more incentive for a client to promise to obey block size
> > +constraints.
> > +
> > +If the client requests `NBD_OPT_STARTTLS` after this option, it
> > +MUST renegotiate extended headers.
> > +
> 
> Does it make sense here to also forbid use of NBD_OPT_EXPORT_NAME? I
> think the sooner we get rid of that, the better ;-)

I hadn't thought of that, but it does indeed sound desirable.  I can
touch up the spec to state that NBD_OPT_EXPORT_NAME MUST NOT be used
by a client that requested extended headers.

> 
> [...]
> > @@ -1746,13 +1914,15 @@ unrecognized flags.
> > 
> >   Structured reply types
> > 
> > -These values are used in the "type" field of a structured reply.
> > -Some chunk types can additionally be categorized by role, such as
> > -*error chunks* or *content chunks*.  Each type determines how to
> > -interpret the "length" bytes of payload.  If the client receives
> > -an unknown or unexpected type, other than an *error chunk*, it
> > -MUST initiate a hard disconnect.  A server MUST NOT send a chunk
> > -larger than any advertised maximum block payload size.
> > +These values are used in the "type" field of a structured reply.  Some
> > +chunk types can additionally be categorized by role, such as *error
> > +chunks*, *content chunks*, or *status chunks*.  Each type determines
> > +how to interpret the "length" bytes of payload.  If the client
> > +receives an unknown or unexpected type, other than an *error chunk*,
> > +it MAY initiate a hard disconnect on the grounds that the client is
> > +uncertain whether the server handled the request as desired.  A server
> > +MUST NOT send a chu

Re: [Libguestfs] [PATCH v2 2/6] spec: Tweak description of maximum block size

[Eric Blake]

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
payload is, the longer that takes - the client is starving the server
from serving other clients by making it chew through data).  But
sending larger read requests MAY be tolerated by a server that sends a
graceful error message ("you req

Re: [Libguestfs] [PATCH v2 2/6] spec: Tweak description of maximum block size

[Eric Blake]

On Fri, Dec 16, 2022 at 11:22:49PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> On 11/15/22 01:46, 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
> 
> I'm not sure about term "block payload size".. What's "block payload"? May be 
> "reply payload size" or something like this?
> 
> Or should we simply say about simple-reply / structured-reply-chunk total 
> size limit?

Yes, I could live with 'reply payload size limit'; a simple-reply
always has one payload, while a structured-reply can be divided across
multiple chunks where each chunk payload fits that limit.

> 
> > +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
> >   `NBD_INFO_BLOCK_SIZE` (via an error reply of
> >   `NBD_REP_ERR_BLOCK_SIZE_REQD`); but servers SHOULD NOT refuse clients
> >   that do not request sizing information when the server supports
> > @@ -818,17 +818,40 @@ the preferred block size for that export.  The server 
> > MAY advertise an
> >   export size that is not an integer multiple of the preferred block
> >   size.
> > 
> > -The maximum block size represents the maximum length that the server
> 
> The term "maximum block size" is not defined anymore (and removed from 
> NBD_INFO_BLOCK_SIZE)...
> 
> > -is willing to handle in one request.  If advertised, it MAY be
> > -something other than a power of 2, but MUST be either an integer
> > -multiple of the minimum block size or the value 0x for no
> > -inherent limit, MUST be at least as large as the smaller of the
> > +The maximum block payload size represents the maximum payload length
> > +that the server is willing to handle in one request.  If advertised,
> >

Re: [Libguestfs] [PATCH v2 1/6] spec: Recommend cap on NBD_REPLY_TYPE_BLOCK_STATUS length

[Eric Blake]

On Fri, Dec 16, 2022 at 10:32:01PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> On 11/15/22 01:46, Eric Blake wrote:
> > The spec was silent on how many extents a server could reply with.
> > However, both qemu and nbdkit (the two server implementations known to
> > have implemented the NBD_CMD_BLOCK_STATUS extension) implement a hard
> > cap, and will truncate the amount of extents in a reply to avoid
> > sending a client a reply so large that the client would treat it as a
> > denial of service attack.  Clients currently have no way during
> > negotiation to request such a limit of the server, so it is easier to
> > just document this as a restriction on viable server implementations
> > than to add yet another round of handshaking.  Also, mentioning
> > amplification effects is worthwhile.
> > 
> > When qemu first implemented NBD_CMD_BLOCK_STATUS for the
> > base:allocation context (qemu commit e7b1948d51, Mar 2018), it behaved
> > as if NBD_CMD_FLAG_REQ_ONE were always passed by the client, and never
> > responded with more than one extent.  Later, when adding its
> > qemu:dirty-bitmap:XYZ context extension (qemu commit 3d068aff16, Jun
> > 2018), it added a cap to 128k extents (1M+4 bytes), and that cap was
> > applied to base:allocation once qemu started sending multiple extents
> > for that context as well (qemu commit fb7afc797e, Jul 2018).  Qemu
> > extents are never smaller than 512 bytes (other than an exception at
> > the end of a file whose size is not aligned to 512), but even so, a
> > request for just under 4G of block status could produce 8M extents,
> > resulting in a reply of 64M if it were not capped smaller.
> > 
> > When nbdkit first implemented NBD_CMD_BLOCK_STATUS (nbdkit 4ca66f70a5,
> > Mar 2019), it did not impose any restriction on the number of extents
> > in the reply chunk.  But because it allows extents as small as one
> > byte, it is easy to write a server that can amplify a client's request
> > of status over 1M of the image into a reply over 8M in size, and it
> > was very easy to demonstrate that a hard cap was needed to avoid
> > crashing clients or otherwise killing the connection (a bad server
> > impacting the client negatively).  So nbdkit enforced a bound of 1M
> > extents (8M+4 bytes, nbdkit commit 6e0dc839ea, Jun 2019).  [Unrelated
> > to this patch, but worth noting for history: nbdkit's situation also
> > has to deal with the fact that it is designed for plugin server
> > implementations; and not capping the number of extents in a reply also
> > posed a problem to nbdkit as the server, where a plugin could exhaust
> > memory and kill the server, unrelated to any size constraints enforced
> > by a client.]
> > 
> > Since the limit chosen by these two implementations is different, and
> > since nbdkit has versions that were not limited, add this as a SHOULD
> > NOT instead of MUST NOT constraint on servers implementing block
> > status.  It does not matter that qemu picked a smaller limit that it
> > truncates to, since we have already documented that the server may
> > truncate for other reasons (such as it being inefficient to collect
> > that many extents in the first place).  But documenting the limit now
> > becomes even more important in the face of a future addition of 64-bit
> > requests, where a client's request is no longer bounded to 4G and
> > could thereby produce even more than 8M extents for the corner case
> > when every 512 bytes is a new extent, if it were not for this
> > recommendation.
> 
> 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.

> 
> 
> Reviewed-by: Vladimir Sementsov-Ogievskiy 
> 
> -- 
> Best regards,
> Vladimir
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

[Libguestfs] [PATCH] docs: Prefer 'cookie' over 'handle'

[Eric Blake]

In libnbd, we quickly learned that distinguishing between 'handle'
(verb for acting on an object) and 'handle' (noun describing which
object to act on) could get confusing; we solved it by renaming the
latter to 'cookie'.  Copy that approach into the NBD spec, and make it
obvious that a cookie is opaque data from the point of view of the
server.  Makes no difference to implementations (other than older code
still using 'handle' may be slightly harder to tie back to the spec).

Suggested-by: Richard W.M. Jones 
Signed-off-by: Eric Blake 
---
 doc/proto.md | 18 +-
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/doc/proto.md b/doc/proto.md
index 3a96703..abb23e8 100644
--- a/doc/proto.md
+++ b/doc/proto.md
@@ -301,11 +301,11 @@ may be handled by the server asynchronously), and 
structured reply
 chunks from one request may be interleaved with reply messages from
 other requests; however, there may be constraints that prevent
 arbitrary reordering of structured reply chunks within a given reply.
-Clients SHOULD use a handle that is distinct from all other currently
-pending transactions, but MAY reuse handles that are no longer in
-flight; handles need not be consecutive.  In each reply message
+Clients SHOULD use a cookie that is distinct from all other currently
+pending transactions, but MAY reuse cookies that are no longer in
+flight; cookies need not be consecutive.  In each reply message
 (whether simple or structured), the server MUST use the same value for
-handle as was sent by the client in the corresponding request.  In
+cookie as was sent by the client in the corresponding request.  In
 this way, the client can correlate which request is receiving a
 response.

@@ -349,7 +349,7 @@ The request message, sent by the client, looks as follows:
 C: 32 bits, 0x25609513, magic (`NBD_REQUEST_MAGIC`)  
 C: 16 bits, command flags  
 C: 16 bits, type  
-C: 64 bits, handle  
+C: 64 bits, cookie  
 C: 64 bits, offset (unsigned)  
 C: 32 bits, length (unsigned)  
 C: (*length* bytes of data if the request is of type `NBD_CMD_WRITE`)  
@@ -366,7 +366,7 @@ follows:
 S: 32 bits, 0x67446698, magic (`NBD_SIMPLE_REPLY_MAGIC`; used to be
`NBD_REPLY_MAGIC`)  
 S: 32 bits, error (MAY be zero)  
-S: 64 bits, handle  
+S: 64 bits, cookie  
 S: (*length* bytes of data if the request is of type `NBD_CMD_READ` and
 *error* is zero)  

@@ -381,7 +381,7 @@ server must initiate a hard disconnect).  Second, there is 
no way to
 efficiently skip over portions of a sparse file that are known to
 contain all zeroes.  Finally, it is not possible to reliably decode
 the server traffic without also having context of what pending read
-requests were sent by the client, to see which *handle* values will
+requests were sent by the client, to see which *cookie* values will
 have accompanying payload on success.  Therefore structured replies
 are also permitted if negotiated.

@@ -398,7 +398,7 @@ sending errors via a structured reply, as the error can 
then be
 accompanied by a string payload to present to a human user.

 A structured reply MAY occupy multiple structured chunk messages
-(all with the same value for "handle"), and the
+(all with the same value for "cookie"), and the
 `NBD_REPLY_FLAG_DONE` reply flag is used to identify the final
 chunk.  Unless further documented by individual requests below,
 the chunks MAY be sent in any order, except that the chunk with
@@ -418,7 +418,7 @@ A structured reply chunk message looks as follows:
 S: 32 bits, 0x668e33ef, magic (`NBD_STRUCTURED_REPLY_MAGIC`)  
 S: 16 bits, flags  
 S: 16 bits, type  
-S: 64 bits, handle  
+S: 64 bits, cookie  
 S: 32 bits, length of payload (unsigned)  
 S: *length* bytes of payload data (if *length* is nonzero)  

-- 
2.39.2

___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [nbdkit PATCH 0/9] common: catch up with libnbd

[Eric Blake]

On Fri, Mar 03, 2023 at 07:00:16PM +, Richard W.M. Jones wrote:
> 
> This series looks, great, thanks!
> 
> ACK

ACK from me as well; I didn't spot any problems in my read through.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [nbdkit PATCH 0/9] common: catch up with libnbd

[Richard W.M. Jones]

This series looks, great, thanks!

ACK

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [V2V PATCH 0/5] Bring support for virtio-scsi back to Windows

[Richard W.M. Jones]

On Thu, Mar 02, 2023 at 11:31:14PM +0200, Andrey Drobyshev wrote:
> On 3/2/23 22:59, Richard W.M. Jones wrote:
> > On Thu, Mar 02, 2023 at 08:52:33PM +0200, Andrey Drobyshev wrote:
> >> On 3/2/23 20:36, Richard W.M. Jones wrote:
> >>> I can probably do an outline of a patch if you need it, but
> >>> not today.
> >>
> >> Yes, sure.  But speaking of the ways to influence the behaviour of a
> >> command line tool: there're also env variables, which are being used by
> >> v2v as well (for instance, for setting the tools ISO location).  So what
> >> if we introduce an env variable here instead?  I presume it would be
> >> much easier to implement.  Smth like:
> > 
> > The environment variable is a bit of a mistake too.  It'd be better to
> > specify that through the command line; and in fact that's what I went
> > with for the new virt-customize --inject-virtio-win option:
> > 
> >--inject-virtio-win METHOD
> >Inject virtio-win drivers into a Windows guest.  These drivers 
> > add
> >virtio accelerated drivers suitable when running on top of a
> >hypervisor that supports virtio (such as qemu/KVM).  The 
> > operation
> >also adjusts the Windows Registry so that the drivers are 
> > installed
> >when the guest boots.
> > 
> >The parameter can be one of:
> > 
> >ISO The path to the ISO image containing the virtio-win drivers
> >(eg. /usr/share/virtio-win/virtio-win.iso).
> >[etc]
> > 
> >>> diff --git a/mlcustomize/inject_virtio_win.ml 
> >>> b/mlcustomize/inject_virtio_win.ml
> >>> index 62f7710..562f055 100644
> >>> --- a/mlcustomize/inject_virtio_win.ml
> >>> +++ b/mlcustomize/inject_virtio_win.ml
> >>> @@ -177,6 +177,11 @@ let rec inject_virtio_win_drivers ({ g } as t) reg =
> >>>  (* Can we install the block driver? *)
> >>>  let block : block_type =
> >>>let filenames = ["virtio_blk"; "vrtioblk"; "viostor"] in
> >>> +  let viostor_drv_env = Sys.getenv_opt "VIOSTOR_DRIVER" in
> >>> +  let filenames =
> >>> +match viostor_drv_env with
> >>> +| None -> filenames
> >>> +| Some str -> str :: filenames in
> >>>let viostor_driver = try (
> >>>  Some (
> >>>List.find (
> >>
> >> As you can see here the default doesn't change if the variable isn't
> >> provided at all (or it is, but is set to a non-existing driver).
> >> What do you think of such a solution?
> > 
> > I prefer the command line option even though it's a little bit more
> > work.  Offer above still stands if you can't work out the complexities
> > of OCaml command line parsing.
> 
> It's not about the command line parsing complexities, but rather about
> the proper way to pass that option from the top level of the call stack
> (convert ()) down to the inject_virtio_win_drivers ().  That'd require
> adding one more extra argument to all the calls along this call stack,
> which might look a bit clumsy.  Isn't there a more elegant way?
> 
> So having a sketch in that regard would be indeed much appreciated.

OK I'll come up with something, but may not be til Monday.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
nbdkit - Flexible, fast NBD server with plugins
https://gitlab.com/nbdkit/nbdkit
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs