lkml.org 
[lkml]   [2023]   [Jan]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 2/6] composefs: Add on-disk layout
On Mon, Nov 28, 2022 at 12:16:23PM +0100, Alexander Larsson wrote:
> This commit adds the on-disk layout header file of composefs.
>
> Signed-off-by: Alexander Larsson <alexl@redhat.com>
> Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

Add Co-Developed-By: Giuseppe ... ?

Full disclosure: I'm not a file system developer but I'll attempt to
help with the review of this series.

> ---
> fs/composefs/cfs.h | 242 +++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 242 insertions(+)
> create mode 100644 fs/composefs/cfs.h
>
> diff --git a/fs/composefs/cfs.h b/fs/composefs/cfs.h
> new file mode 100644
> index 000000000000..8f001fd28d6b
> --- /dev/null
> +++ b/fs/composefs/cfs.h
> @@ -0,0 +1,242 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * composefs
> + *
> + * Copyright (C) 2021 Giuseppe Scrivano
> + * Copyright (C) 2022 Alexander Larsson
> + *
> + * This file is released under the GPL.
> + */
> +
> +#ifndef _CFS_H
> +#define _CFS_H
> +
> +#include <asm/byteorder.h>
> +#include <crypto/sha2.h>
> +#include <linux/fs.h>
> +#include <linux/stat.h>
> +#include <linux/types.h>
> +
> +#define CFS_VERSION 1
> +
> +#define CFS_MAGIC 0xc078629aU
> +
> +#define CFS_MAX_DIR_CHUNK_SIZE 4096
> +#define CFS_MAX_XATTRS_SIZE 4096
> +
> +static inline u16 cfs_u16_to_file(u16 val)
> +{
> + return cpu_to_le16(val);
> +}
> +
> +static inline u32 cfs_u32_to_file(u32 val)
> +{
> + return cpu_to_le32(val);
> +}
> +
> +static inline u64 cfs_u64_to_file(u64 val)
> +{
> + return cpu_to_le64(val);
> +}
> +
> +static inline u16 cfs_u16_from_file(u16 val)
> +{
> + return le16_to_cpu(val);
> +}
> +
> +static inline u32 cfs_u32_from_file(u32 val)
> +{
> + return le32_to_cpu(val);
> +}
> +
> +static inline u64 cfs_u64_from_file(u64 val)
> +{
> + return le64_to_cpu(val);
> +}

I don't see where the cfs_xxx_{to,from}_file() approach is used in other
filesystems. Instead, move the cpu() functions directly into the code.

> +static inline int cfs_xdigit_value(char c)
> +{
> + if (c >= '0' && c <= '9')
> + return c - '0';
> + if (c >= 'A' && c <= 'F')
> + return c - 'A' + 10;
> + if (c >= 'a' && c <= 'f')
> + return c - 'a' + 10;
> + return -1;
> +}

There's some utilities in lib/hexdump.c that you can use. hex_to_bin()
will convert a single character and hex2bin() will convert a string for
you.

> +static inline int cfs_digest_from_payload(const char *payload,
> + size_t payload_len,
> + u8 digest_out[SHA256_DIGEST_SIZE])
> +{
> + const char *p, *end;
> + u8 last_digit = 0;
> + int digit = 0;
> + size_t n_nibbles = 0;

Put in reverse Christmas tree order.

> +
> + end = payload + payload_len;
> + for (p = payload; p != end; p++) {
> + /* Skip subdir structure */
> + if (*p == '/')
> + continue;
> +
> + /* Break at (and ignore) extension */
> + if (*p == '.')
> + break;

A comment would be helpful in this area that shows what the payload is
expected to be.

> +
> + if (n_nibbles == SHA256_DIGEST_SIZE * 2)
> + return -1; /* Too long */

return -EINVAL; ?

> +
> + digit = cfs_xdigit_value(*p);
> + if (digit == -1)
> + return -1; /* Not hex digit */

-EINVAL here as well

> +
> + n_nibbles++;
> + if ((n_nibbles % 2) == 0) {
> + digest_out[n_nibbles / 2 - 1] =
> + (last_digit << 4) | digit;
> + }
> + last_digit = digit;
> + }
> +
> + if (n_nibbles != SHA256_DIGEST_SIZE * 2)
> + return -1; /* Too short */

-EINVAL here as well

> +
> + return 0;
> +}
> +
> +struct cfs_vdata_s {
> + u64 off;
> + u32 len;
> +} __packed;
> +
> +struct cfs_header_s {
> + u8 version;
> + u8 unused1;
> + u16 unused2;
> +
> + u32 magic;

Should the magic number appear first?

> + u64 data_offset;
> + u64 root_inode;
> +
> + u64 unused3[2];
> +} __packed;
> +
> +enum cfs_inode_flags {
> + CFS_INODE_FLAGS_NONE = 0,
> + CFS_INODE_FLAGS_PAYLOAD = 1 << 0,
> + CFS_INODE_FLAGS_MODE = 1 << 1,
> + CFS_INODE_FLAGS_NLINK = 1 << 2,
> + CFS_INODE_FLAGS_UIDGID = 1 << 3,
> + CFS_INODE_FLAGS_RDEV = 1 << 4,
> + CFS_INODE_FLAGS_TIMES = 1 << 5,
> + CFS_INODE_FLAGS_TIMES_NSEC = 1 << 6,
> + CFS_INODE_FLAGS_LOW_SIZE = 1 << 7, /* Low 32bit of st_size */
> + CFS_INODE_FLAGS_HIGH_SIZE = 1 << 8, /* High 32bit of st_size */
> + CFS_INODE_FLAGS_XATTRS = 1 << 9,
> + CFS_INODE_FLAGS_DIGEST = 1
> + << 10, /* fs-verity sha256 digest of content */

Include << 10 on line above

Brian

\
 
 \ /
  Last update: 2023-03-26 23:28    [W:0.182 / U:0.456 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site