| Message ID | 20230310201525.2615385-3-eblake@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | nbd: s/handle/cookie/ | expand |
On Fri, Mar 10, 2023 at 10:16 PM Eric Blake <eblake@redhat.com> 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@redhat.com> > --- > 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. > + }; > }; > #endif /* _UAPILINUX_NBD_H */ > -- > 2.39.2 > Nir
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@redhat.com> 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@redhat.com> > > --- > > 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; and previous code was using memcpy() onto char[8] which may behave differently depending on machine endianness).
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@redhat.com> 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@redhat.com> > > > --- > > > 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
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; + }; }; #endif /* _UAPILINUX_NBD_H */
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@redhat.com> --- include/uapi/linux/nbd.h | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)