[PATCH v2 2/5] dump.erofs: add "-s" option to dump superblock information

[Gao Xiang]

Some nits...

On Tue, Sep 14, 2021 at 03:44:21PM +0800, Guo Xuenan wrote:
> From: Wang Qi <mpiglet at outlook.com>
> 
> Signed-off-by: Guo Xuenan <guoxuenan at huawei.com>
> Signed-off-by: Wang Qi <mpiglet at outlook.com>
> ---
>  dump/Makefile.am |  3 +-
>  dump/main.c      | 94 +++++++++++++++++++++++++++++++++++++++++++-----
>  2 files changed, 88 insertions(+), 9 deletions(-)
> 
> diff --git a/dump/Makefile.am b/dump/Makefile.am
> index 8e18c0f..1ba58da 100644
> --- a/dump/Makefile.am
> +++ b/dump/Makefile.am
> @@ -3,7 +3,8 @@
>  
>  AUTOMAKE_OPTIONS = foreign
>  bin_PROGRAMS     = dump.erofs
> +AM_CPPFLAGS = ${libuuid_CFLAGS}
>  dump_erofs_SOURCES = main.c
>  dump_erofs_CFLAGS = -Wall -Werror -I$(top_srcdir)/include
> -dump_erofs_LDADD = $(top_builddir)/lib/liberofs.la ${liblz4_LIBS}
> +dump_erofs_LDADD = ${libuuid_LIBS} $(top_builddir)/lib/liberofs.la ${liblz4_LIBS}

+dump_erofs_LDADD = $(top_builddir)/lib/liberofs.la ${libuuid_LIBS}

>  
> diff --git a/dump/main.c b/dump/main.c
> index 8f299ca..33521bf 100644
> --- a/dump/main.c
> +++ b/dump/main.c
> @@ -6,25 +6,57 @@
>   *            Guo Xuenan <guoxuenan at huawei.com>
>   */
>  
> +#define _GNU_SOURCE
>  #include <stdlib.h>
>  #include <getopt.h>
>  #include <sys/sysmacros.h>
>  #include <time.h>
>  #include <lz4.h>
> -

Should we get rid of this blank line in the patch 1 instead?

>  #include "erofs/print.h"
>  #include "erofs/io.h"
>  
> +#ifdef HAVE_LIBUUID
> +#include <uuid.h>
> +#endif
> +
> +struct dumpcfg {
> +	bool print_superblock;
> +	bool print_version;
> +};
> +static struct dumpcfg dumpcfg;
> +
>  static struct option long_options[] = {
>  	{"help", no_argument, 0, 1},
>  	{0, 0, 0, 0},
>  };
>  
> +#define EROFS_FEATURE_COMPAT	0
> +#define EROFS_FEATURE_INCOMPAT	1

How about getting rid of these?

> +
> +struct feature {
> +	int compat;

	bool compat;

> +	unsigned int mask;
> +	const char *name;
> +};
> +
> +static struct feature feature_lists[] = {
> +	{	EROFS_FEATURE_COMPAT, EROFS_FEATURE_COMPAT_SB_CHKSUM,
> +		"superblock-checksum"	},

sb_csum is enough... (refer to "metadata_csum" shown in dumpe2fs)

> +
> +	{	EROFS_FEATURE_INCOMPAT, EROFS_FEATURE_INCOMPAT_LZ4_0PADDING,
> +		"lz4-0padding"	},

How about showing "0padding" instead? Since I'll use this in the LZMA
patchset as well... (so not lz4-specific...)

> +	{	EROFS_FEATURE_INCOMPAT, EROFS_FEATURE_INCOMPAT_BIG_PCLUSTER,
> +		"big-pcluster"	},

bigpcluster

> +
> +	{	0, 0, 0	},
> +};
> +
>  static void usage(void)
>  {
>  	fputs("usage: [options] erofs-image\n\n"
>  		"Dump erofs layout from erofs-image, and [options] are:\n"
>  		"--help  display this help and exit.\n"
> +		"-s          print information about superblock\n"
>  		"-V      print the version number of dump.erofs and exit.\n",
>  		stderr);
>  }
> @@ -37,9 +69,12 @@ static int dumpfs_parse_options_cfg(int argc, char **argv)
>  {
>  	int opt;
>  
> -	while ((opt = getopt_long(argc, argv, "V",
> +	while ((opt = getopt_long(argc, argv, "sV",
>  					long_options, NULL)) != -1) {
>  		switch (opt) {
> +		case 's':
> +			dumpcfg.print_superblock = true;
> +			break;
>  		case 'V':
>  			dumpfs_print_version();
>  			exit(0);
> @@ -65,21 +100,64 @@ static int dumpfs_parse_options_cfg(int argc, char **argv)
>  	return 0;
>  }
>  
> +static void dumpfs_print_superblock(void)
> +{
> +	time_t time = sbi.build_time;
> +	unsigned int features[] = {sbi.feature_compat, sbi.feature_incompat};
> +	char uuid_str[37] = "not available";
> +	int i = 0;
> +	int j = 0;
> +
> +	fprintf(stdout, "Filesystem magic number:			0x%04X\n", EROFS_SUPER_MAGIC_V1);
> +	fprintf(stdout, "Filesystem blocks:				%lu\n", sbi.blocks);
> +	fprintf(stdout, "Filesystem inode metadata start block:		%u\n", sbi.meta_blkaddr);
> +	fprintf(stdout, "Filesystem shared xattr metadata start block:	%u\n", sbi.xattr_blkaddr);
> +	fprintf(stdout, "Filesystem root nid:				%ld\n", sbi.root_nid);
> +	fprintf(stdout, "Filesystem valid inode count:			%lu\n", sbi.inos);

"Filesystem inode count" is enough since such "valid" expression makes
people think about what does the invalid inode mean.

Thanks,
Gao Xiang

> +	fprintf(stdout, "Filesystem created:				%s", ctime(&time));
> +	fprintf(stdout, "Filesystem features:				");
> +	for (; i < ARRAY_SIZE(features); i++) {
> +		for (; j < ARRAY_SIZE(feature_lists); j++) {
> +			if (i == feature_lists[j].compat
> +				&& (features[i] & feature_lists[j].mask))
> +				fprintf(stdout, "%s ", feature_lists[j].name);
> +		}
> +	}
> +	fprintf(stdout, "\n");
> +#ifdef HAVE_LIBUUID
> +	uuid_unparse_lower(sbi.uuid, uuid_str);
> +#endif
> +	fprintf(stdout, "Filesystem UUID:				%s\n", uuid_str);
> +}
> +
>  int main(int argc, char **argv)
>  {
>  	int err = 0;
>  
>  	erofs_init_configure();
>  	err = dumpfs_parse_options_cfg(argc, argv);
> -
> -	if (cfg.c_img_path)
> -		free(cfg.c_img_path);
> -
>  	if (err) {
>  		if (err == -EINVAL)
>  			usage();
> -		return -1;
> +		goto out;
>  	}
>  
> -	return 0;
> +	err = dev_open_ro(cfg.c_img_path);
> +	if (err) {
> +		erofs_err("open image file failed");
> +		goto out;
> +	}
> +
> +	err = erofs_read_superblock();
> +	if (err) {
> +		erofs_err("read superblock failed");
> +		goto out;
> +	}
> +
> +	if (dumpcfg.print_superblock)
> +		dumpfs_print_superblock();
> +out:
> +	if (cfg.c_img_path)
> +		free(cfg.c_img_path);
> +	return err;
>  }
> -- 
> 2.25.4