[PATCH] erofs-utils: add missing errors and normalize errors to lower-case
Gao Xiang
hsiangkao at linux.alibaba.com
Sat Jan 29 15:52:07 AEDT 2022
Hi Igor,
On Fri, Jan 28, 2022 at 03:22:18PM +0200, Igor Ostapenko wrote:
> From: Igor Eisberg <igoreisberg at gmail.com>
>
> * Added useful error messages.
> * Most errors start with lower-case, let's make all of them lower-case
> for better consistency.
>
> Signed-off-by: Igor Ostapenko <igoreisberg at gmail.com>
> ---
> dump/main.c | 4 +++-
> fsck/main.c | 35 +++++++++++++++++++++++------------
> mkfs/main.c | 32 +++++++++++++++++---------------
> 3 files changed, 43 insertions(+), 28 deletions(-)
>
> diff --git a/dump/main.c b/dump/main.c
> index 0616113..664780b 100644
> --- a/dump/main.c
> +++ b/dump/main.c
> @@ -162,8 +162,10 @@ static int erofsdump_parse_options_cfg(int argc, char **argv)
> }
> }
>
> - if (optind >= argc)
> + if (optind >= argc) {
> + erofs_err("missing argument: IMAGE");
> return -EINVAL;
> + }
>
> cfg.c_img_path = strdup(argv[optind++]);
> if (!cfg.c_img_path)
> diff --git a/fsck/main.c b/fsck/main.c
> index 6f17d0e..5667f2a 100644
> --- a/fsck/main.c
> +++ b/fsck/main.c
> @@ -202,8 +202,10 @@ static int erofsfsck_parse_options_cfg(int argc, char **argv)
> }
> }
>
> - if (optind >= argc)
> + if (optind >= argc) {
> + erofs_err("missing argument: IMAGE");
> return -EINVAL;
> + }
>
> cfg.c_img_path = strdup(argv[optind++]);
> if (!cfg.c_img_path)
> @@ -230,8 +232,12 @@ static void erofsfsck_set_attributes(struct erofs_inode *inode, char *path)
>
> if (utimensat(AT_FDCWD, path, times, AT_SYMLINK_NOFOLLOW) < 0)
> #else
> - if (utime(path, &((struct utimbuf){.actime = inode->i_ctime,
> - .modtime = inode->i_ctime})) < 0)
> + const struct utimbuf time = {
> + .actime = inode->i_ctime,
> + .modtime = inode->i_ctime,
> + };
> +
> + if (utime(path, &time) < 0)
Can we drop this change since personally I don't want to introduce
unnecessary variables if possible?
> #endif
> erofs_warn("failed to set times: %s", path);
>
> @@ -512,7 +518,7 @@ static inline int erofs_extract_dir(struct erofs_inode *inode)
> struct stat st;
>
> if (errno != EEXIST) {
> - erofs_err("failed to create directory %s: %s",
> + erofs_err("failed to create directory: %s (%s)",
> fsckcfg.extract_path, strerror(errno));
> return -errno;
> }
> @@ -528,8 +534,11 @@ static inline int erofs_extract_dir(struct erofs_inode *inode)
> * Try to change permissions of existing directory so
> * that we can write to it
> */
> - if (chmod(fsckcfg.extract_path, 0700) < 0)
> + if (chmod(fsckcfg.extract_path, 0700) < 0) {
> + erofs_err("failed to set permissions: %s (%s)",
> + fsckcfg.extract_path, strerror(errno));
> return -errno;
> + }
> }
> return 0;
> }
> @@ -551,18 +560,20 @@ again:
> erofs_warn("try to forcely remove directory %s",
> fsckcfg.extract_path);
> if (rmdir(fsckcfg.extract_path) < 0) {
> - erofs_err("failed to remove: %s",
> - fsckcfg.extract_path);
> + erofs_err("failed to remove: %s (%s)",
> + fsckcfg.extract_path, strerror(errno));
> return -EISDIR;
> }
> } else if (errno == EACCES &&
> chmod(fsckcfg.extract_path, 0700) < 0) {
> + erofs_err("failed to set permissions: %s (%s)",
> + fsckcfg.extract_path, strerror(errno));
> return -errno;
> }
> tryagain = false;
> goto again;
> }
> - erofs_err("failed to open %s: %s", fsckcfg.extract_path,
> + erofs_err("failed to open: %s (%s)", fsckcfg.extract_path,
> strerror(errno));
> return -errno;
> }
> @@ -768,15 +779,15 @@ int main(int argc, char **argv)
> err = erofsfsck_check_inode(sbi.root_nid, sbi.root_nid);
> if (fsckcfg.corrupted) {
> if (!fsckcfg.extract_path)
> - erofs_err("Found some filesystem corruption");
> + erofs_err("found some filesystem corruption");
IMO, how about leaving these conclusive messages starting with
upper cases?
> else
> - erofs_err("Failed to extract filesystem");
> + erofs_err("failed to extract filesystem");
ditto.
> err = -EFSCORRUPTED;
> } else if (!err) {
> if (!fsckcfg.extract_path)
> - erofs_info("No error found");
> + erofs_info("no errors found");
ditto.
> else
> - erofs_info("Extract data successfully");
> + erofs_info("extracted filesystem successfully");
ditto.
Thanks,
Gao Xiang
>
> if (fsckcfg.print_comp_ratio) {
> double comp_ratio =
> diff --git a/mkfs/main.c b/mkfs/main.c
> index c755da1..dd698ff 100644
> --- a/mkfs/main.c
> +++ b/mkfs/main.c
> @@ -381,9 +381,6 @@ static int mkfs_parse_options_cfg(int argc, char *argv[])
> }
> }
>
> - if (optind >= argc)
> - return -EINVAL;
> -
> if (cfg.c_blobdev_path && cfg.c_chunkbits < LOG_BLOCK_SIZE) {
> erofs_err("--blobdev must be used together with --chunksize");
> return -EINVAL;
> @@ -396,24 +393,29 @@ static int mkfs_parse_options_cfg(int argc, char *argv[])
> return -EINVAL;
> }
>
> + if (optind >= argc) {
> + erofs_err("missing argument: FILE");
> + return -EINVAL;
> + }
> +
> cfg.c_img_path = strdup(argv[optind++]);
> if (!cfg.c_img_path)
> return -ENOMEM;
>
> if (optind >= argc) {
> - erofs_err("Source directory is missing");
> + erofs_err("missing argument: DIRECTORY");
> return -EINVAL;
> }
>
> cfg.c_src_path = realpath(argv[optind++], NULL);
> if (!cfg.c_src_path) {
> - erofs_err("Failed to parse source directory: %s",
> + erofs_err("failed to parse source directory: %s",
> erofs_strerror(-errno));
> return -ENOENT;
> }
>
> if (optind < argc) {
> - erofs_err("Unexpected argument: %s\n", argv[optind]);
> + erofs_err("unexpected argument: %s\n", argv[optind]);
> return -EINVAL;
> }
> if (quiet)
> @@ -456,7 +458,7 @@ int erofs_mkfs_update_super_block(struct erofs_buffer_head *bh,
>
> buf = calloc(sb_blksize, 1);
> if (!buf) {
> - erofs_err("Failed to allocate memory for sb: %s",
> + erofs_err("failed to allocate memory for sb: %s",
> erofs_strerror(-errno));
> return -ENOMEM;
> }
> @@ -538,7 +540,7 @@ int parse_source_date_epoch(void)
>
> epoch = strtoull(source_date_epoch, &endptr, 10);
> if (epoch == -1ULL || *endptr != '\0') {
> - erofs_err("Environment variable $SOURCE_DATE_EPOCH %s is invalid",
> + erofs_err("environment variable $SOURCE_DATE_EPOCH %s is invalid",
> source_date_epoch);
> return -EINVAL;
> }
> @@ -641,34 +643,34 @@ int main(int argc, char **argv)
> sb_bh = erofs_buffer_init();
> if (IS_ERR(sb_bh)) {
> err = PTR_ERR(sb_bh);
> - erofs_err("Failed to initialize buffers: %s",
> + erofs_err("failed to initialize buffers: %s",
> erofs_strerror(err));
> goto exit;
> }
> err = erofs_bh_balloon(sb_bh, EROFS_SUPER_END);
> if (err < 0) {
> - erofs_err("Failed to balloon erofs_super_block: %s",
> + erofs_err("failed to balloon erofs_super_block: %s",
> erofs_strerror(err));
> goto exit;
> }
>
> err = erofs_load_compress_hints();
> if (err) {
> - erofs_err("Failed to load compress hints %s",
> + erofs_err("failed to load compress hints %s",
> cfg.c_compress_hints_file);
> goto exit;
> }
>
> err = z_erofs_compress_init(sb_bh);
> if (err) {
> - erofs_err("Failed to initialize compressor: %s",
> + erofs_err("failed to initialize compressor: %s",
> erofs_strerror(err));
> goto exit;
> }
>
> err = erofs_generate_devtable();
> if (err) {
> - erofs_err("Failed to generate device table: %s",
> + erofs_err("failed to generate device table: %s",
> erofs_strerror(err));
> goto exit;
> }
> @@ -681,7 +683,7 @@ int main(int argc, char **argv)
>
> err = erofs_build_shared_xattrs_from_path(cfg.c_src_path);
> if (err) {
> - erofs_err("Failed to build shared xattrs: %s",
> + erofs_err("failed to build shared xattrs: %s",
> erofs_strerror(err));
> goto exit;
> }
> @@ -727,7 +729,7 @@ exit:
> erofs_exit_configure();
>
> if (err) {
> - erofs_err("\tCould not format the device : %s\n",
> + erofs_err("\tcould not format the device : %s\n",
> erofs_strerror(err));
> return 1;
> }
> --
> 2.30.2
More information about the Linux-erofs
mailing list