* [PATCH] erofs-utils: add missing errors and normalize errors to lower-case
@ 2022-01-28 13:22 Igor Ostapenko
2022-01-29 4:52 ` Gao Xiang
2022-01-29 5:47 ` Igor Eisberg
0 siblings, 2 replies; 10+ messages in thread
From: Igor Ostapenko @ 2022-01-28 13:22 UTC (permalink / raw)
To: linux-erofs; +Cc: Igor Eisberg
From: Igor Eisberg <igoreisberg@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@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)
#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");
else
- erofs_err("Failed to extract filesystem");
+ erofs_err("failed to extract filesystem");
err = -EFSCORRUPTED;
} else if (!err) {
if (!fsckcfg.extract_path)
- erofs_info("No error found");
+ erofs_info("no errors found");
else
- erofs_info("Extract data successfully");
+ erofs_info("extracted filesystem successfully");
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] erofs-utils: add missing errors and normalize errors to lower-case
2022-01-28 13:22 [PATCH] erofs-utils: add missing errors and normalize errors to lower-case Igor Ostapenko
@ 2022-01-29 4:52 ` Gao Xiang
2022-01-29 5:47 ` Igor Eisberg
1 sibling, 0 replies; 10+ messages in thread
From: Gao Xiang @ 2022-01-29 4:52 UTC (permalink / raw)
To: Igor Ostapenko; +Cc: linux-erofs
Hi Igor,
On Fri, Jan 28, 2022 at 03:22:18PM +0200, Igor Ostapenko wrote:
> From: Igor Eisberg <igoreisberg@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@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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] erofs-utils: add missing errors and normalize errors to lower-case
2022-01-28 13:22 [PATCH] erofs-utils: add missing errors and normalize errors to lower-case Igor Ostapenko
2022-01-29 4:52 ` Gao Xiang
@ 2022-01-29 5:47 ` Igor Eisberg
2022-01-29 5:56 ` Gao Xiang
1 sibling, 1 reply; 10+ messages in thread
From: Igor Eisberg @ 2022-01-29 5:47 UTC (permalink / raw)
To: linux-erofs
[-- Attachment #1: Type: text/plain, Size: 11511 bytes --]
Point regarding conclusive messages taken, will revert the relevant lines.
As for the time variable, all I did was to match it to the format as in the
case of HAVE_UTIMENSAT, just above it.
Although the variable isn't used further, inlining it is unreadable.
You also changed this:
> ret = z_erofs_decompress(&(struct z_erofs_decompress_req) {
>
> .in = raw,
>
> .out = buffer,
>
> .decodedskip = 0,
>
> .inputsize = map.m_plen,
>
> .decodedlength = map.m_llen,
>
> .alg = map.m_algorithmformat,
>
> .partial_decoding = 0
>
> });
>
>
to this:
> struct z_erofs_decompress_req rq = {
>
> .in = raw,
>
> .out = buffer,
>
> .decodedskip = 0,
>
> .inputsize = map.m_plen,
>
> .decodedlength = map.m_llen,
>
> .alg = map.m_algorithmformat,
>
> .partial_decoding = 0
>
> };
>
>
>> ret = z_erofs_decompress(&rq);
>
>
Although that variable could have remained avoided, like in lib/data.c,
where it's still avoided:
> ret = z_erofs_decompress(&(struct z_erofs_decompress_req) {
>
> .in = raw,
>
> .out = buffer + end - offset,
>
> .decodedskip = skip,
>
> .inputsize = map.m_plen,
>
> .decodedlength = length,
>
> .alg = map.m_algorithmformat,
>
> .partial_decoding = partial
>
> });
>
>
It's just that inconsistency in code style is an eyesore. ;)
On Fri, 28 Jan 2022 at 15:22, Igor Ostapenko <igoreisberg@gmail.com> wrote:
> From: Igor Eisberg <igoreisberg@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@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)
> #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");
> else
> - erofs_err("Failed to extract filesystem");
> + erofs_err("failed to extract filesystem");
> err = -EFSCORRUPTED;
> } else if (!err) {
> if (!fsckcfg.extract_path)
> - erofs_info("No error found");
> + erofs_info("no errors found");
> else
> - erofs_info("Extract data successfully");
> + erofs_info("extracted filesystem successfully");
>
> 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
>
>
[-- Attachment #2: Type: text/html, Size: 20041 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] erofs-utils: add missing errors and normalize errors to lower-case
2022-01-29 5:47 ` Igor Eisberg
@ 2022-01-29 5:56 ` Gao Xiang
2022-01-29 6:13 ` Gao Xiang
0 siblings, 1 reply; 10+ messages in thread
From: Gao Xiang @ 2022-01-29 5:56 UTC (permalink / raw)
To: Igor Eisberg; +Cc: linux-erofs
On Sat, Jan 29, 2022 at 07:47:31AM +0200, Igor Eisberg wrote:
> Point regarding conclusive messages taken, will revert the relevant lines.
Thank.
> As for the time variable, all I did was to match it to the format as in the
> case of HAVE_UTIMENSAT, just above it.
> Although the variable isn't used further, inlining it is unreadable.
>
Please don't. Otherwise, complier will complain
"mixed declarations and code"
and I don't want to initialize such structures at the beginning of any
block.
Thanks,
Gao Xiang
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] erofs-utils: add missing errors and normalize errors to lower-case
2022-01-29 5:56 ` Gao Xiang
@ 2022-01-29 6:13 ` Gao Xiang
2022-01-29 6:48 ` Igor Eisberg
0 siblings, 1 reply; 10+ messages in thread
From: Gao Xiang @ 2022-01-29 6:13 UTC (permalink / raw)
To: Igor Eisberg; +Cc: linux-erofs
On Sat, Jan 29, 2022 at 01:56:30PM +0800, Gao Xiang wrote:
> On Sat, Jan 29, 2022 at 07:47:31AM +0200, Igor Eisberg wrote:
> > Point regarding conclusive messages taken, will revert the relevant lines.
>
> Thank.
>
> > As for the time variable, all I did was to match it to the format as in the
> > case of HAVE_UTIMENSAT, just above it.
> > Although the variable isn't used further, inlining it is unreadable.
> >
>
> Please don't. Otherwise, complier will complain
> "mixed declarations and code"
>
> and I don't want to initialize such structures at the beginning of any
> block.
Add some word. Actually, I'd like to add:
if (!fsckcfg.extract_path)
return;
at the beginning of erofsfsck_set_attributes() instead of
using "if (!ret && fsckcfg.extract_path)" in the caller.
So the HAVE_UTIMENSAT case needs to be resolved as well.
Btw, I have no idea why it could become unreadable to you, first,
it would avoid "mixed declarations and code" unless defining them at the
beginning of blocks, and I don't like memset(.., 0, ) and set
independently since memset generally is a library function (unless
compliers do some built-in tricks) rather than a language feature.
Thanks,
Gao Xiang
>
> Thanks,
> Gao Xiang
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] erofs-utils: add missing errors and normalize errors to lower-case
2022-01-29 6:13 ` Gao Xiang
@ 2022-01-29 6:48 ` Igor Eisberg
2022-01-29 8:04 ` Gao Xiang
0 siblings, 1 reply; 10+ messages in thread
From: Igor Eisberg @ 2022-01-29 6:48 UTC (permalink / raw)
To: Gao Xiang; +Cc: linux-erofs
[-- Attachment #1: Type: text/plain, Size: 2503 bytes --]
On Sat, 29 Jan 2022, 08:13 Gao Xiang, <hsiangkao@linux.alibaba.com> wrote:
> On Sat, Jan 29, 2022 at 01:56:30PM +0800, Gao Xiang wrote:
> > On Sat, Jan 29, 2022 at 07:47:31AM +0200, Igor Eisberg wrote:
> > > Point regarding conclusive messages taken, will revert the relevant
> lines.
> >
> > Thank.
> >
> > > As for the time variable, all I did was to match it to the format as
> in the
> > > case of HAVE_UTIMENSAT, just above it.
> > > Although the variable isn't used further, inlining it is unreadable.
> > >
> >
> > Please don't. Otherwise, complier will complain
> > "mixed declarations and code"
>
Why would it? Declaration of const struct timespec times is inside
#ifdef...#else, while the declaration for const struct utimebuf time is
inside #else...#endif.
The compiler won't complain because after the preprocessor is done, the
compiler only gets one of them, never both. There is no "mixed declarations
and code" thanks to the preprocessor...
>
> > and I don't want to initialize such structures at the beginning of any
> > block.
>
What exactly is "such structures"? I pointed you to an example where you
did just that with struct z_erofs_decompress_req rq variable, I'm just
following your code style, and this one stood out as unusual for your code
style. Please count how many anonymous struct initializations you have
across the whole erofs-utils project. I count only this one, and another in
lib/data.c. Everything else is initialized as named variables.
> Add some word.
I didn't understand that...
Actually, I'd like to add:
> if (!fsckcfg.extract_path)
> return;
>
> at the beginning of erofsfsck_set_attributes() instead of
> using "if (!ret && fsckcfg.extract_path)" in the caller.
>
OK, but then we will have mixed declarations and code, because then we
won't be at the beginning of the block...
> So the HAVE_UTIMENSAT case needs to be resolved as well.
>
> Btw, I have no idea why it could become unreadable to you, first,
> it would avoid "mixed declarations and code" unless defining them at the
> beginning of blocks,
Again, there is nothing mixed here, because preprocessor.
and I don't like memset(.., 0, ) and set
> independently since memset generally is a library function (unless
> compliers do some built-in tricks) rather than a language feature.
>
Forgive me but I don't remember seeing any mention of memset in the whole
fsck.c file, so I have no idea why you're telling me this.
> Thanks,
> Gao Xiang
> >
> > Thanks,
> > Gao Xiang
>
[-- Attachment #2: Type: text/html, Size: 4725 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] erofs-utils: add missing errors and normalize errors to lower-case
2022-01-29 6:48 ` Igor Eisberg
@ 2022-01-29 8:04 ` Gao Xiang
2022-01-29 18:22 ` Igor Ostapenko
2022-01-29 19:45 ` Igor Ostapenko
0 siblings, 2 replies; 10+ messages in thread
From: Gao Xiang @ 2022-01-29 8:04 UTC (permalink / raw)
To: Igor Eisberg; +Cc: linux-erofs
On Sat, Jan 29, 2022 at 08:48:01AM +0200, Igor Eisberg wrote:
> On Sat, 29 Jan 2022, 08:13 Gao Xiang, <hsiangkao@linux.alibaba.com> wrote:
..
>
> >
> > > and I don't want to initialize such structures at the beginning of any
> > > block.
> >
>
> What exactly is "such structures"? I pointed you to an example where you
> did just that with struct z_erofs_decompress_req rq variable, I'm just
> following your code style, and this one stood out as unusual for your code
> style. Please count how many anonymous struct initializations you have
> across the whole erofs-utils project. I count only this one, and another in
> lib/data.c. Everything else is initialized as named variables.
Sorry, I didn't express clear. I mean here:
if (compressed) {
struct z_erofs_decompress_req rq = {
.in = raw,
.out = buffer,
.decodedskip = 0,
.inputsize = map.m_plen,
.decodedlength = map.m_llen,
.alg = map.m_algorithmformat,
.partial_decoding = 0
};
ret = z_erofs_decompress(&rq);
if (ret < 0) {
erofs_err("failed to decompress data of m_pa %" PRIu64 ", m_plen %" PRIu64 " @ nid %llu: %s",
mdev.m_pa, map.m_plen,
inode->nid | 0ULL, strerror(-ret));
goto out;
}
}
rq is at the beginning of the block, and we use the initialized
structure immediately, so I wrote as this.
>
>
> > Add some word.
>
>
> I didn't understand that...
>
> Actually, I'd like to add:
> > if (!fsckcfg.extract_path)
> > return;
> >
> > at the beginning of erofsfsck_set_attributes() instead of
> > using "if (!ret && fsckcfg.extract_path)" in the caller.
> >
>
> OK, but then we will have mixed declarations and code, because then we
> won't be at the beginning of the block...
Ok, sorry about confusion, I've made a commit instead..
Take a look at:
https://git.kernel.org/pub/scm/linux/kernel/git/xiang/erofs-utils.git/commit/?h=experimental&id=69476e3ffa4985e767d33cb24d020f3669bc2c77
Since I'd like to avoid too many "if (!ret && fsckcfg.extract_path)"
in the callers if erofsfsck_set_attributes() uses in other place
in the future.
Thanks,
Gao Xiang
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] erofs-utils: add missing errors and normalize errors to lower-case
2022-01-29 8:04 ` Gao Xiang
@ 2022-01-29 18:22 ` Igor Ostapenko
2022-01-29 19:45 ` Igor Ostapenko
1 sibling, 0 replies; 10+ messages in thread
From: Igor Ostapenko @ 2022-01-29 18:22 UTC (permalink / raw)
To: linux-erofs; +Cc: Igor Eisberg
From: Igor Eisberg <igoreisberg@gmail.com>
* Added useful error messages.
* Most errors start with lower-case, let's make all non-summarizing
error messages lower-case for better consistency.
* Sorted default values in fsck's main function to match the struct.
Signed-off-by: Igor Ostapenko <igoreisberg@gmail.com>
---
dump/main.c | 4 +++-
fsck/main.c | 44 ++++++++++++++++++++++++++------------------
mkfs/main.c | 30 ++++++++++++++++--------------
3 files changed, 45 insertions(+), 33 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 5b75ee3..3be5d66 100644
--- a/fsck/main.c
+++ b/fsck/main.c
@@ -122,8 +122,10 @@ static int erofsfsck_parse_options_cfg(int argc, char **argv)
if (optarg) {
size_t len = strlen(optarg);
- if (len == 0)
+ if (len == 0) {
+ erofs_err("empty value given for --extract=X");
return -EINVAL;
+ }
/* remove trailing slashes except root */
while (len > 1 && optarg[len - 1] == '/')
@@ -134,10 +136,9 @@ static int erofsfsck_parse_options_cfg(int argc, char **argv)
return -ENOMEM;
strncpy(fsckcfg.extract_path, optarg, len);
fsckcfg.extract_path[len] = '\0';
- /* if path is root, start writing from position 0 */
- if (len == 1 && fsckcfg.extract_path[0] == '/')
- len = 0;
- fsckcfg.extract_pos = len;
+ /* update position only if path is not root */
+ if (len > 1 || fsckcfg.extract_path[0] != '/')
+ fsckcfg.extract_pos = len;
}
break;
case 3:
@@ -201,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)
@@ -513,7 +516,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;
}
@@ -529,8 +532,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;
}
@@ -552,18 +558,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;
}
@@ -728,15 +736,15 @@ int main(int argc, char **argv)
erofs_init_configure();
- fsckcfg.superuser = geteuid() == 0;
+ fsckcfg.physical_blocks = 0;
+ fsckcfg.logical_blocks = 0;
+ fsckcfg.extract_path = NULL;
+ fsckcfg.extract_pos = 0;
fsckcfg.umask = umask(0);
+ fsckcfg.superuser = geteuid() == 0;
fsckcfg.corrupted = false;
- fsckcfg.logical_blocks = 0;
- fsckcfg.physical_blocks = 0;
fsckcfg.print_comp_ratio = false;
fsckcfg.check_decomp = false;
- fsckcfg.extract_path = NULL;
- fsckcfg.extract_pos = 0;
fsckcfg.force = false;
fsckcfg.overwrite = false;
fsckcfg.preserve_owner = fsckcfg.superuser;
@@ -775,9 +783,9 @@ int main(int argc, char **argv)
err = -EFSCORRUPTED;
} else if (!err) {
if (!fsckcfg.extract_path)
- erofs_info("No error found");
+ erofs_info("No errors found");
else
- erofs_info("Extract data successfully");
+ erofs_info("Extracted filesystem successfully");
if (fsckcfg.print_comp_ratio) {
double comp_ratio =
diff --git a/mkfs/main.c b/mkfs/main.c
index c755da1..5f241a1 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;
}
--
2.30.2
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH] erofs-utils: add missing errors and normalize errors to lower-case
2022-01-29 8:04 ` Gao Xiang
2022-01-29 18:22 ` Igor Ostapenko
@ 2022-01-29 19:45 ` Igor Ostapenko
2022-01-30 0:26 ` Gao Xiang
1 sibling, 1 reply; 10+ messages in thread
From: Igor Ostapenko @ 2022-01-29 19:45 UTC (permalink / raw)
To: linux-erofs; +Cc: Igor Eisberg
From: Igor Eisberg <igoreisberg@gmail.com>
Had second thoughts about this change I made, because it's making
an assumption about the default value of extract_pos (being 0).
Signed-off-by: Igor Ostapenko <igoreisberg@gmail.com>
---
fsck/main.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/fsck/main.c b/fsck/main.c
index 3be5d66..e669b44 100644
--- a/fsck/main.c
+++ b/fsck/main.c
@@ -136,9 +136,10 @@ static int erofsfsck_parse_options_cfg(int argc, char **argv)
return -ENOMEM;
strncpy(fsckcfg.extract_path, optarg, len);
fsckcfg.extract_path[len] = '\0';
- /* update position only if path is not root */
- if (len > 1 || fsckcfg.extract_path[0] != '/')
- fsckcfg.extract_pos = len;
+ /* if path is root, start writing from position 0 */
+ if (len == 1 && fsckcfg.extract_path[0] == '/')
+ len = 0;
+ fsckcfg.extract_pos = len;
}
break;
case 3:
--
2.30.2
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] erofs-utils: add missing errors and normalize errors to lower-case
2022-01-29 19:45 ` Igor Ostapenko
@ 2022-01-30 0:26 ` Gao Xiang
0 siblings, 0 replies; 10+ messages in thread
From: Gao Xiang @ 2022-01-30 0:26 UTC (permalink / raw)
To: Igor Ostapenko; +Cc: linux-erofs
On Sat, Jan 29, 2022 at 09:45:32PM +0200, Igor Ostapenko wrote:
> From: Igor Eisberg <igoreisberg@gmail.com>
>
> Had second thoughts about this change I made, because it's making
> an assumption about the default value of extract_pos (being 0).
>
> Signed-off-by: Igor Ostapenko <igoreisberg@gmail.com>
Thanks for these. I've folded this patch into the previous one
and apply to experimental branch. You can always resend the whole
patch based on -dev or -experimental branch before it's applied
to -dev branch (which I will not rebase).
Also, a minor suggestion, it'd be better to do one thing (or some
related stuffs) in one patch. So here, even extract_pos
modification is needed, IMO, it would be better with a new patch.
Thanks,
Gao Xiang
> ---
> fsck/main.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/fsck/main.c b/fsck/main.c
> index 3be5d66..e669b44 100644
> --- a/fsck/main.c
> +++ b/fsck/main.c
> @@ -136,9 +136,10 @@ static int erofsfsck_parse_options_cfg(int argc, char **argv)
> return -ENOMEM;
> strncpy(fsckcfg.extract_path, optarg, len);
> fsckcfg.extract_path[len] = '\0';
> - /* update position only if path is not root */
> - if (len > 1 || fsckcfg.extract_path[0] != '/')
> - fsckcfg.extract_pos = len;
> + /* if path is root, start writing from position 0 */
> + if (len == 1 && fsckcfg.extract_path[0] == '/')
> + len = 0;
> + fsckcfg.extract_pos = len;
> }
> break;
> case 3:
> --
> 2.30.2
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2022-01-30 0:26 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-28 13:22 [PATCH] erofs-utils: add missing errors and normalize errors to lower-case Igor Ostapenko
2022-01-29 4:52 ` Gao Xiang
2022-01-29 5:47 ` Igor Eisberg
2022-01-29 5:56 ` Gao Xiang
2022-01-29 6:13 ` Gao Xiang
2022-01-29 6:48 ` Igor Eisberg
2022-01-29 8:04 ` Gao Xiang
2022-01-29 18:22 ` Igor Ostapenko
2022-01-29 19:45 ` Igor Ostapenko
2022-01-30 0:26 ` Gao Xiang
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.