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

[Omar Sandoval <osandov@osandov.com>]

On Thu, Dec 09, 2021 at 10:08:02AM -0800, Omar Sandoval wrote:
> 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:
> > > 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.
> 
> Ping. Please let me know how you'd like me to proceed on this issue and
> my other replies. Thanks!

New year, new ping. Thanks!