[Qemu-devel] [PATCH 2/2] Correct definition of NBD_CMD_FLAG_FUA

[Qemu-devel] [PATCH 2/2] Correct definition of NBD_CMD_FLAG_FUA

[Alex Bligh]

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(-)

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 */
-- 
1.9.1

Re: [Qemu-devel] [PATCH 2/2] Correct definition of NBD_CMD_FLAG_FUA

[Alex Bligh]

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?

-- 
Alex Bligh

Re: [Qemu-devel] [PATCH 2/2] Correct definition of NBD_CMD_FLAG_FUA

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1243 bytes --]

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.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 2/2] Correct definition of NBD_CMD_FLAG_FUA

[Alex Bligh]

[-- Attachment #1: Type: text/plain, Size: 573 bytes --]


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





[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 842 bytes --]

Re: [Qemu-devel] [PATCH 2/2] Correct definition of NBD_CMD_FLAG_FUA

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1097 bytes --]

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

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 2/2] Correct definition of NBD_CMD_FLAG_FUA

[Alex Bligh]

[-- Attachment #1: Type: text/plain, Size: 664 bytes --]


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





[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 842 bytes --]

Re: [Qemu-devel] [Nbd] [PATCH 2/2] Correct definition of NBD_CMD_FLAG_FUA

[Wouter Verhelst]

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 ;-)

-- 
< ron> I mean, the main *practical* problem with C++, is there's like a dozen
       people in the world who think they really understand all of its rules,
       and pretty much all of them are just lying to themselves too.
 -- #debian-devel, OFTC, 2016-02-12

Re: [Qemu-devel] [Nbd] [PATCH 2/2] Correct definition of NBD_CMD_FLAG_FUA

[Alex Bligh]

On 1 Apr 2016, at 08:59, Wouter Verhelst <w@uter.be> wrote:

> 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 ;-)

Sorry - I think I must have done 'git grep' in the wrong directory
or something. Ignore this patch and I'll redo it.

I think probably the easiest fix is (beware, manually generated
untested patch) as follows, which at least shows the reason for the
discrepancy.

diff --git a/nbd.h b/nbd.h
index f2a32dd..53b6ca1 100644
--- a/nbd.h
+++ b/nbd.h
@@ -38,7 +38,9 @@ enum {
};

#define NBD_CMD_MASK_COMMAND 0x0000ffff
-#define NBD_CMD_FLAG_FUA (1<<16)
+#define NBD_CMD_SHIFT (16)
+#define NBD_CMD_FLAG_FUA ((1 << 0) << NBD_CMD_SHIFT)

/* values for flags field */
#define NBD_FLAG_HAS_FLAGS	(1 << 0)	/* Flags are there */

-- 
Alex Bligh

Re: [Qemu-devel] [Nbd] [PATCH 2/2] Correct definition of NBD_CMD_FLAG_FUA

[Wouter Verhelst]

On Fri, Apr 01, 2016 at 10:32:50AM +0100, Alex Bligh wrote:
> #define NBD_CMD_MASK_COMMAND 0x0000ffff
> -#define NBD_CMD_FLAG_FUA (1<<16)
> +#define NBD_CMD_SHIFT (16)
> +#define NBD_CMD_FLAG_FUA ((1 << 0) << NBD_CMD_SHIFT)

That works too, I suppose.

However, like I said, I need to clean this up anyway. I thought your
"will you take a patch" was going to do that, but if not, I'll get
around to doing it myself at some point...

-- 
< ron> I mean, the main *practical* problem with C++, is there's like a dozen
       people in the world who think they really understand all of its rules,
       and pretty much all of them are just lying to themselves too.
 -- #debian-devel, OFTC, 2016-02-12

Re: [Qemu-devel] [Nbd] [PATCH 2/2] Correct definition of NBD_CMD_FLAG_FUA

[Alex Bligh]

On 1 Apr 2016, at 10:43, Wouter Verhelst <w@uter.be> wrote:

> On Fri, Apr 01, 2016 at 10:32:50AM +0100, Alex Bligh wrote:
>> #define NBD_CMD_MASK_COMMAND 0x0000ffff
>> -#define NBD_CMD_FLAG_FUA (1<<16)
>> +#define NBD_CMD_SHIFT (16)
>> +#define NBD_CMD_FLAG_FUA ((1 << 0) << NBD_CMD_SHIFT)
> 
> That works too, I suppose.
> 
> However, like I said, I need to clean this up anyway. I thought your
> "will you take a patch" was going to do that, but if not, I'll get
> around to doing it myself at some point...

How about you take that one for now and one of us will get do
doing it properly in due course. The proper solution is obviously
to use two 16 bit fields.

-- 
Alex Bligh