Re: [PATCH v3] btrfs: send: add support for fs-verity

[David Sterba <dsterba@suse.cz>]

On Tue, Aug 02, 2022 at 01:38:37PM -0700, Eric Biggers wrote:
> On Mon, Aug 01, 2022 at 11:54:40AM -0700, Boris Burkov wrote:
> > +#ifdef CONFIG_FS_VERITY
> > +static int send_verity(struct send_ctx *sctx, struct fs_path *path,
> > +		       struct fsverity_descriptor *desc)
> > +{
> > +	int ret;
> > +
> > +	ret = begin_cmd(sctx, BTRFS_SEND_C_ENABLE_VERITY);
> > +	if (ret < 0)
> > +		goto out;
> > +
> > +	TLV_PUT_PATH(sctx, BTRFS_SEND_A_PATH, path);
> > +	TLV_PUT_U8(sctx, BTRFS_SEND_A_VERITY_ALGORITHM, desc->hash_algorithm);
> > +	TLV_PUT_U32(sctx, BTRFS_SEND_A_VERITY_BLOCK_SIZE, 1U << desc->log_blocksize);
> > +	TLV_PUT(sctx, BTRFS_SEND_A_VERITY_SALT_DATA, desc->salt, desc->salt_size);
> > +	TLV_PUT(sctx, BTRFS_SEND_A_VERITY_SIG_DATA, desc->signature, (int)desc->sig_size);
> 
> le32_to_cpu(desc->sig_size)

Don't all the members of desc need the le/cpu helpers? The whole
structure is read from disk directly to the memory buffer, there's no
conversion to a cpu-native order structure, so this must be done when
the members are accessed.

While the first four are of type __u8 so there's no endianness
conversion needed, I'd rather do it for clarity that the structure needs
special handling.

> > +	ret = send_cmd(sctx);
> > +
> > +tlv_put_failure:
> > +out:
> > +	return ret;
> > +}
> 
> The 'out' label is unnecessary.

It's a common pattern in send callbacks to have out: label next to
tlv_put_failure.

> > +static int process_new_verity(struct send_ctx *sctx)
> 
> What does "new verity" mean in this context?  The other functions called by
> finish_inode_if_needed() have names like send_chown(), send_chmod(), etc., so
> this name seems inconsistent (although I'm not familiar with this code).

Yeah I think process_verity or process_verity_desc will be better.

> > +	ret = send_verity(sctx, p, sctx->verity_descriptor);
> > +	if (ret < 0)
> > +		goto free_path;
> > +
> > +free_path:
> > +	fs_path_free(p);
> 
> The goto above is unnecessary.
> 
> > +static int process_new_verity(struct send_ctx *sctx)
> > +{
> > +	int ret = 0;
> > +	struct send_ctx tmp;
> > +
> > +	return -EPERM;
> > +	/* avoid unused TLV_PUT_U8 build warning without CONFIG_FS_VERITY */
> > +	TLV_PUT_U8(&tmp, 0, 0);
> > +tlv_put_failure:
> > +	return -EPERM;
> > +}
> > +#endif
> 
> How about adding __maybe_unused to tlv_put_u##bits instead?

Or it could use U16 or U32 type, it's not strictly necessary to use the
same type width as the in-memory structures.