Re: [PATCH v12 12/17] btrfs: send: fix maximum command numbering

From: Omar Sandoval <osandov@osandov.com>
To: dsterba@suse.cz, linux-btrfs@vger.kernel.org, kernel-team@fb.com
Subject: Re: [PATCH v12 12/17] btrfs: send: fix maximum command numbering
Date: Thu, 18 Nov 2021 10:54:16 -0800	[thread overview]
Message-ID: <YZahWPMfY5CLXTa6@relinquished.localdomain> (raw)
In-Reply-To: <20211118142359.GE28560@twin.jikos.cz>

On Thu, Nov 18, 2021 at 03:23:59PM +0100, David Sterba wrote:
> On Wed, Nov 17, 2021 at 12:19:22PM -0800, Omar Sandoval wrote:
> > From: Omar Sandoval <osandov@fb.com>
> > 
> > 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.
> 
> > 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.
> 
> > --- 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.