[Nbd,2/2] Correct definition of NBD_CMD_FLAG_FUA

Commit Message

Alex Bligh April 1, 2016, 9:32 a.m. UTC

On 1 Apr 2016, at 08:59, Wouter Verhelst <[e-mail]> 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.

Comments

Wouter Verhelst April 1, 2016, 9:43 a.m. UTC | #1

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

Alex Bligh April 1, 2016, 10:11 a.m. UTC | #2

On 1 Apr 2016, at 10:43, Wouter Verhelst <[e-mail]> 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.

Patch

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