Re: [PATCH 2/3] uapi nbd: add cookie alias to handle

From: Ming Lei
Date: Tue Mar 14 2023 - 23:34:36 EST


On Tue, Mar 14, 2023 at 02:50:23PM -0500, Eric Blake wrote:
> On Sat, Mar 11, 2023 at 02:30:39PM +0200, Nir Soffer wrote:
> > On Fri, Mar 10, 2023 at 10:16 PM Eric Blake <eblake@xxxxxxxxxx> wrote:
> > >
> > > The uapi <linux/nbd.h> header declares a 'char handle[8]' per request;
> > > which is overloaded in English (are you referring to "handle" the
> > > verb, such as handling a signal or writing a callback handler, or
> > > "handle" the noun, the value used in a lookup table to correlate a
> > > response back to the request). Many client-side NBD implementations
> > > (both servers and clients) have instead used 'u64 cookie' or similar,
> > > as it is easier to directly assign an integer than to futz around with
> > > memcpy. In fact, upstream documentation is now encouraging this shift
> > > in terminology: https://lists.debian.org/nbd/2023/03/msg00031.html
> > >
> > > Accomplish this by use of an anonymous union to provide the alias for
> > > anyone getting the definition from the uapi; this does not break
> > > existing clients, while exposing the nicer name for those who prefer
> > > it. Note that block/nbd.c still uses the term handle (in fact, it
> > > actually combines a 32-bit cookie and a 32-bit tag into the 64-bit
> > > handle), but that internal usage is not changed the public uapi, since
> > > no compliant NBD server has any reason to inspect or alter the 64
> > > bits sent over the socket.
> > >
> > > Signed-off-by: Eric Blake <eblake@xxxxxxxxxx>
> > > ---
> > > include/uapi/linux/nbd.h | 10 ++++++++--
> > > 1 file changed, 8 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/include/uapi/linux/nbd.h b/include/uapi/linux/nbd.h
> > > index 8797387caaf7..f58f2043f62e 100644
> > > --- a/include/uapi/linux/nbd.h
> > > +++ b/include/uapi/linux/nbd.h
> > > @@ -81,7 +81,10 @@ enum {
> > > struct nbd_request {
> > > __be32 magic; /* NBD_REQUEST_MAGIC */
> > > __be32 type; /* See NBD_CMD_* */
> > > - char handle[8];
> > > + union {
> > > + char handle[8];
> > > + __be64 cookie;
> > > + };
> > > __be64 from;
> > > __be32 len;
> > > } __attribute__((packed));
> > > @@ -93,6 +96,9 @@ struct nbd_request {
> > > struct nbd_reply {
> > > __be32 magic; /* NBD_REPLY_MAGIC */
> > > __be32 error; /* 0 = ok, else error */
> > > - char handle[8]; /* handle you got from request */
> > > + union {
> > > + char handle[8]; /* handle you got from request */
> > > + __be64 cookie;
> >
> > Should we document like this?
> >
> > union {
> > __be64 cookie; /* cookie you got from request */
> > char handle[8]; /* older name */
> >
> > I think we want future code to use the new term.
>
> Sure, swapping the order to favor the preferred name first makes sense.
>
> I'm still not sure on whether cookie should be u64 or __be64 (it's
> opaque, so endianness over the wire doesn't matter;

I guess it is 'u64', given ->handle is always copied to nbd_reply from
nbd_request in nbd server side, so native endian is always applied for
building and parsing ->handle in nbd client side.

But it looks odd to mark it as u64.

> and previous code
> was using memcpy() onto char[8] which may behave differently depending
> on machine endianness).




Thanks,
Ming