On Thu, Nov 18, 2021 at 10:54:16AM -0800, Omar Sandoval wrote:

On Thu, Nov 18, 2021 at 03:23:59PM +0100, David Sterba wrote:

quoted

On Wed, Nov 17, 2021 at 12:19:22PM -0800, Omar Sandoval wrote:

quoted

From: Omar Sandoval <redacted>

Commit e77fbf990316 ("btrfs: send: prepare for v2 protocol") added
_BTRFS_SEND_C_MAX_V* macros equal to the maximum command number for the
version plus 1, but as written this creates gaps in the number space.
The maximum command number is currently 22, and __BTRFS_SEND_C_MAX_V1 is
accordingly 23. But then __BTRFS_SEND_C_MAX_V2 is 24, suggesting that v2
has a command numbered 23, and __BTRFS_SEND_C_MAX is 25, suggesting that
23 and 24 are valid commands.

The MAX definitions have the __ prefix so they're private and not meant
to be used as proper commands, so nothing should suggest there are any
commands with numbers 23 to 25 in the example.

quoted

Instead, let's explicitly set BTRFS_SEND_C_MAX_V* to the maximum command
number. This requires repeating the command name, but it has a clearer
meaning and avoids gaps. It also doesn't require updating
__BTRFS_SEND_C_MAX for every new version.

It's probably a matter of taste, I'd intentionally avoid the pattern
above, ie. repeating the previous command to define max.

quoted

--- a/fs/btrfs/send.c
+++ b/fs/btrfs/send.c

@@ -316,8 +316,8 @@ __maybe_unused
 static bool proto_cmd_ok(const struct send_ctx *sctx, int cmd)
 {
 	switch (sctx->proto) {
-	case 1:	 return cmd < __BTRFS_SEND_C_MAX_V1;
-	case 2:	 return cmd < __BTRFS_SEND_C_MAX_V2;
+	case 1:	 return cmd <= BTRFS_SEND_C_MAX_V1;
+	case 2:	 return cmd <= BTRFS_SEND_C_MAX_V2;

This seems to be the only practical difference, < or <= .

There is another practical difference, which is more significant in my
opinion: the linear style creates "gaps" in the valid commands. Consider
this, with explicit values added for clarity:

enum btrfs_send_cmd {
        BTRFS_SEND_C_UNSPEC = 0,

        /* Version 1 */
        BTRFS_SEND_C_SUBVOL = 1,
        BTRFS_SEND_C_SNAPSHOT = 2,

        BTRFS_SEND_C_MKFILE = 3,
        BTRFS_SEND_C_MKDIR = 4,
        BTRFS_SEND_C_MKNOD = 5,
        BTRFS_SEND_C_MKFIFO = 6,
        BTRFS_SEND_C_MKSOCK = 7,
        BTRFS_SEND_C_SYMLINK = 8,

        BTRFS_SEND_C_RENAME = 9,
        BTRFS_SEND_C_LINK = 10,
        BTRFS_SEND_C_UNLINK = 11,
        BTRFS_SEND_C_RMDIR = 12,

        BTRFS_SEND_C_SET_XATTR = 13,
        BTRFS_SEND_C_REMOVE_XATTR = 14,

        BTRFS_SEND_C_WRITE = 15,
        BTRFS_SEND_C_CLONE = 16,

        BTRFS_SEND_C_TRUNCATE = 17,
        BTRFS_SEND_C_CHMOD = 18,
        BTRFS_SEND_C_CHOWN = 19,
        BTRFS_SEND_C_UTIMES = 20,

        BTRFS_SEND_C_END = 21,
        BTRFS_SEND_C_UPDATE_EXTENT = 22,
        __BTRFS_SEND_C_MAX_V1 = 23,

        /* Version 2 */
        BTRFS_SEND_C_FALLOCATE = 24,
        BTRFS_SEND_C_SETFLAGS = 25,
        BTRFS_SEND_C_ENCODED_WRITE = 26,
        __BTRFS_SEND_C_MAX_V2 = 27,

        /* End */
        __BTRFS_SEND_C_MAX = 28,
};
#define BTRFS_SEND_C_MAX (__BTRFS_SEND_C_MAX - 1) /* 27 */

Notice that BTRFS_SEND_C_UPDATE_EXTENT is 22 and the next valid command
is BTRFS_SEND_C_FALLOCATE, which is 24. So 23 does not correspond to an
actual command; it's a "gap". This is somewhat cosmetic, but it's an
ugly wart in the protocol.

Also consider something indexing on the command number, like the
cmd_send_size thing I got rid of in the previous patch:

	u64 cmd_send_size[BTRFS_SEND_C_MAX + 1]

Indices 23 and 27 are wasted. It's only 16 bytes in this case, which
doesn't matter practically, but it's unpleasant.

Maybe you were aware of this and fine with it, in which case we can drop
this change. But I think the name repetition is less ugly than the gaps.

Ping. Please let me know how you'd like me to proceed on this issue and
my other replies. Thanks!