| Message ID | 1459448132-52364-1-git-send-email-alex@alex.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 31 Mar 2016, at 19:15, Alex Bligh <alex@alex.org.uk> wrote: > It is doubtful whether anyone is using NBD_CMD_FLAG_FUA > at the moment in any case. Drat. I spoke too soon. Qemu uses it, but presumably from its own .h file. However, it's now nonsensical having it defined as 1<<16 in a 16 bit flags variable. Should we produce a new name for it (and future command flags) that aren't shifted left 16 places, and just maintain the current value for compatibility?
On 03/31/2016 12:21 PM, Alex Bligh wrote: > > On 31 Mar 2016, at 19:15, Alex Bligh <alex@alex.org.uk> wrote: > >> It is doubtful whether anyone is using NBD_CMD_FLAG_FUA >> at the moment in any case. > > Drat. I spoke too soon. Qemu uses it, but presumably from its > own .h file. Yes, qemu has its own nbd.h, which still has nbd_request with a single uint32_type that holds both flags and command type. It wouldn't be too hard to rework that to more closely match upstream NBD. > > However, it's now nonsensical having it defined as 1<<16 in a > 16 bit flags variable. I don't see any problem with your patch on the NBD project side of things; it's not like 'make install' is dumping a header into /usr/include for client programs to reuse (which is _why_ qemu is using its own nbd.h), because no one has really churned out an NBD-client library for embedding in larger programs. > > Should we produce a new name for it (and future command flags) > that aren't shifted left 16 places, and just maintain the > current value for compatibility? I don't see the point. Your fix looks correct.
On 31 Mar 2016, at 20:14, Eric Blake <eblake@redhat.com> wrote: >> >> >> Should we produce a new name for it (and future command flags) >> that aren't shifted left 16 places, and just maintain the >> current value for compatibility? > > I don't see the point. Your fix looks correct. OK. And the wrongness hasn't yet got into /usr/include/linux/nbd.h or include/uapi/linux/nbd.h ; these have no reference to FUA at all, even though it does have NBD_FLAG_SEND_FLUSH, which is odd as I added both flags at the same time. So I'm guessing it's safe. -- Alex Bligh
On 03/31/2016 01:25 PM, Alex Bligh wrote: > > On 31 Mar 2016, at 20:14, Eric Blake <eblake@redhat.com> wrote: > >>> >>> >>> Should we produce a new name for it (and future command flags) >>> that aren't shifted left 16 places, and just maintain the >>> current value for compatibility? >> >> I don't see the point. Your fix looks correct. > > OK. And the wrongness hasn't yet got into /usr/include/linux/nbd.h Still using the older '__be32 type;' instead of the newer '__be16 flags; __be16 type;', changing that one will be ABI compatible, but not API compatible. I don't know what people want to do there, :( > or include/uapi/linux/nbd.h ; these have no reference to FUA at all, I'm not finding that file on my system; not sure what it contains, or where it is maintained. > even though it does have NBD_FLAG_SEND_FLUSH, which is odd as I added > both flags at the same time. > > So I'm guessing it's safe. > > -- > Alex Bligh > > > >
On 31 Mar 2016, at 21:07, Eric Blake <eblake@redhat.com> wrote: >> >> or include/uapi/linux/nbd.h ; these have no reference to FUA at all, > > I'm not finding that file on my system; not sure what it contains, or > where it is maintained. files have moved around in the kernel, and the userspace api file now lives in include/uapi/linux/nbd.h e.g: https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/nbd.h include/linux/nbd.h has gone from the kernel, and previously included internal structs (now in nbd.c I think) Disclaimer: it's a while since I looked at nbd kernel side. -- Alex Bligh
On Thu, Mar 31, 2016 at 07:15:32PM +0100, Alex Bligh wrote: > NBD_CMD_FLAG_FUA is defined as 1<<0 in the documentation, but > 1<<16 in nbd.h. It is not used anywhere within the code. Yes it is: wouter@gangtai:~/code/c/nbd$ grep -rl CMD_FLAG_FUA * doc/proto.md make-integrityhuge.c nbd.h nbd-server.c nbd-trdump.c tests/run/nbd-tester-client.c wouter@gangtai:~/code/c/nbd$ I don't mind bringing the code in sync with what the documentation says, but it should not change behaviour ;-)
diff --git a/nbd.h b/nbd.h index f2a32dd..53b6ca1 100644 --- a/nbd.h +++ b/nbd.h @@ -38,7 +38,7 @@ enum { }; #define NBD_CMD_MASK_COMMAND 0x0000ffff -#define NBD_CMD_FLAG_FUA (1<<16) +#define NBD_CMD_FLAG_FUA (1 << 0) /* values for flags field */ #define NBD_FLAG_HAS_FLAGS (1 << 0) /* Flags are there */
NBD_CMD_FLAG_FUA is defined as 1<<0 in the documentation, but 1<<16 in nbd.h. It is not used anywhere within the code. 1<<16 cannot work as the flags word is only 16 bits long. It is doubtful whether anyone is using NBD_CMD_FLAG_FUA at the moment in any case. Signed-off-by: Alex Bligh <alex@alex.org.uk> --- nbd.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)