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.
|