Re: [PATCH v14 2/7] btrfs: send: explicitly number commands and attributes

Sweet Tea Dorminy
2022-03-24T17:52:54Z

On 3/17/22 13:25, 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
s/macros/enums/?
> 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.
>
> Instead, let's explicitly number all of the commands, attributes, and
> sentinel MAX constants.

If you were sending out another version, the last sentence explains what the change is doing but not why you like it as a solution to the problem.

Nit: I think it would be slightly more elegant to set the MAX values to the appropriate symbolic value instead of numerical, to emphasize the relationship a tiny bit more. Perhaps e.g.

enum btrfs_send_cmds {
...
BTRFS_SEND_C_UPDATE_EXTENT,
__BTRFS_SEND_CMDS_V1_MAX = BTRFS_SEND_C_UPDATE_EXTENT,
BTRFS_SEND_C_FALLOCATE,
...
BTRFS_SEND_C_ENCODED_WRITE,
__BTRFS_SEND_CMDS_V2_MAX = BTRFS_SEND_C_ENCODED_WRITE,
BTRFS_SEND_C_CMDS_MAX = __BTRFS_SEND_CMDS_V2_MAX,
};

(either with or without explicitly setting the numerical values of the individual commands). Or perhaps #define the MAX values instead, still in terms of the symbolic constant?

I have a mild preference for not explicitly setting things to numerical values, so it's harder to duplicate a value by accident later on; but explicit setting does help if one day some cmd needs to be dropped from the middle of the list without renumbering everything, so shrug.

Nit: If you do stick with explicit numerical values everywhere, is there a chance you could line up the = vertically, to make it easier to scan down the list of numbers and verify they are in order and without holes in the sequence? I know vertical alignment is usually unneeded, but in this case I think it does add a bunch to the readability of the table.