[PATCH v2] Refactor out some I/O logic into separate function

[Gao Xiang]

Hi Kelvin,

On Tue, Nov 23, 2021 at 04:36:40PM -0800, Kelvin Zhang wrote:
> Many of the global variables are for I/O purposes. To make the codebase
> more library friendly, decouple I/O and parsing into separate functions.
> 
> Signed-off-by: Kelvin Zhang <zhangkelvin at google.com>

I think I understand the general main point of this. I'm little afraid
if erofs_parse_inode_from_buffer and erofs_parse_superblock are enough
for your use cases...

If that isn't enough, how about adding more seekable I/O source (lib/io.c)
so that it's not only limited to I/O functions but also from memory?

> ---
>  include/erofs/defs.h  | 19 +++++++++++++
>  include/erofs/parse.h | 23 +++++++++++++++
>  include/erofs_fs.h    | 12 ++++++++
>  lib/io.c              |  1 +
>  lib/namei.c           | 38 ++++++++++++++++---------
>  lib/super.c           | 66 +++++++++++++++++++++++++------------------
>  6 files changed, 119 insertions(+), 40 deletions(-)
>  create mode 100644 include/erofs/parse.h
> 
> diff --git a/include/erofs/defs.h b/include/erofs/defs.h
> index 6398cbb..16376dc 100644
> --- a/include/erofs/defs.h
> +++ b/include/erofs/defs.h
> @@ -8,6 +8,11 @@
>  #ifndef __EROFS_DEFS_H
>  #define __EROFS_DEFS_H
>  
> +#ifdef __cplusplus
> +extern "C"
> +{
> +#endif
> +
>  #include <stddef.h>
>  #include <stdint.h>
>  #include <assert.h>
> @@ -82,12 +87,18 @@ typedef int64_t         s64;
>  #endif
>  #endif
>  
> +#ifdef __cplusplus
> +#define BUILD_BUG_ON(condition) static_assert(!condition)
> +#else
> +
>  #ifndef __OPTIMIZE__
>  #define BUILD_BUG_ON(condition) ((void)sizeof(char[1 - 2 * !!(condition)]))
>  #else
>  #define BUILD_BUG_ON(condition) assert(!(condition))
>  #endif
>  
> +#endif
> +
>  #define DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d))
>  
>  #define __round_mask(x, y)      ((__typeof__(x))((y)-1))
> @@ -110,6 +121,8 @@ typedef int64_t         s64;
>  }							\
>  )
>  
> +// Defining min/max macros in C++ will cause conflicts with std::min/max
> +#ifndef __cplusplus
>  #define min(x, y) ({				\
>  	typeof(x) _min1 = (x);			\
>  	typeof(y) _min2 = (y);			\
> @@ -121,6 +134,7 @@ typedef int64_t         s64;
>  	typeof(y) _max2 = (y);			\
>  	(void) (&_max1 == &_max2);		\
>  	_max1 > _max2 ? _max1 : _max2; })
> +#endif
>  
>  /*
>   * ..and if you can't take the strict types, you can specify one yourself.
> @@ -308,4 +322,9 @@ unsigned long __roundup_pow_of_two(unsigned long n)
>  #define stat64		stat
>  #define lstat64		lstat
>  #endif
> +
> +#ifdef __cplusplus
> +}
> +#endif
> +
>  #endif
> diff --git a/include/erofs/parse.h b/include/erofs/parse.h
> new file mode 100644
> index 0000000..65948a1
> --- /dev/null
> +++ b/include/erofs/parse.h
> @@ -0,0 +1,23 @@
> +
> +#ifndef __EROFS_PARSE_H
> +#define __EROFS_PARSE_H
> +
> +#ifdef __cplusplus
> +extern "C"
> +{
> +#endif
> +
> +#include "erofs_fs.h"
> +#include "internal.h"
> +
> +int erofs_parse_inode_from_buffer(const char buf[EROFS_MAX_INODE_SIZE],
> +                                  struct erofs_inode *vi);
> +
> +int erofs_parse_superblock(const char data[EROFS_BLKSIZ],
> +                           struct erofs_sb_info *sbi);

How about moving them into include/erofs/internal.h?


> +
> +#ifdef __cplusplus
> +}
> +#endif
> +
> +#endif
> diff --git a/include/erofs_fs.h b/include/erofs_fs.h
> index 9a91877..3f1f645 100644
> --- a/include/erofs_fs.h
> +++ b/include/erofs_fs.h
> @@ -9,6 +9,12 @@
>  #ifndef __EROFS_FS_H
>  #define __EROFS_FS_H
>  
> +#ifdef __cplusplus
> +#define inline constexpr
> +extern "C"
> +{
> +#endif
> +
>  #define EROFS_SUPER_MAGIC_V1    0xE0F5E1E2
>  #define EROFS_SUPER_OFFSET      1024
>  
> @@ -189,6 +195,8 @@ struct erofs_inode_extended {
>  	__u8   i_reserved2[16];
>  };
>  
> +#define EROFS_MAX_INODE_SIZE sizeof(struct erofs_inode_extended)
> +

Could we move it into include/erofs/internal.h as well?
Since erofs_fs.h is the on-disk definition only and keeps in sync
with the kernel header.

Thanks,
Gao Xiang