[PATCH] AOSP: erofs-utils: fix sub-directory prefix for canned fs_config

[PATCH] AOSP: erofs-utils: fix sub-directory prefix for canned fs_config

[Gao Xiang via Linux-erofs]

From: Gao Xiang <hsiangkao@aol.com>

"failed to find [%s] in canned fs_config" was observed by using
"--fs-config-file" option as reported by Yue Hu [1].

The root cause was that the mountpoint prefix to subdirectories is
also needed if "--mount-point" presents. However, such prefix cannot
be added by just using erofs_fspath().

One exception is that the root directory itself needs to be handled
specially for canned fs_config. For such case, the prefix of the root
directory has to be dropped instead.

[1] https://lkml.kernel.org/r/20201222020430.12512-1-zbestahu@gmail.com

Fixes: 8a9e8046f170 ("AOSP: erofs-utils: add fs_config support")
Reported-by: Yue Hu <huyue2@yulong.com>
Signed-off-by: Gao Xiang <hsiangkao@aol.com>
---
Hi Yue, Jianan,

I've verified cuttlefish booting with success, It'd be better to
verify this patchset on your sides. Please kingly leave "Tested-by:"
if possible.

Hi Guifu,
Could you also review this patch ? This needs to be included in
the upcoming v1.2.1 as well...

Thanks,
Gao Xiang

 lib/inode.c | 39 +++++++++++++++++++++++++--------------
 1 file changed, 25 insertions(+), 14 deletions(-)

diff --git a/lib/inode.c b/lib/inode.c
index 0c4839dc7152..f88966d26fce 100644
--- a/lib/inode.c
+++ b/lib/inode.c
@@ -696,32 +696,43 @@ int erofs_droid_inode_fsconfig(struct erofs_inode *inode,
 	/* filesystem_config does not preserve file type bits */
 	mode_t stat_file_type_mask = st->st_mode & S_IFMT;
 	unsigned int uid = 0, gid = 0, mode = 0;
-	char *fspath;
+	const char *fspath;
+	char *decorated = NULL;
 
 	inode->capabilities = 0;
+	if (!cfg.fs_config_file && !cfg.mount_point)
+		return 0;
+
+	if (!cfg.mount_point ||
+	/* have to drop the mountpoint for rootdir of canned fsconfig */
+	    (cfg.fs_config_file && IS_ROOT(inode))) {
+		fspath = erofs_fspath(path);
+	} else {
+		if (asprintf(&decorated, "%s/%s", cfg.mount_point,
+			     erofs_fspath(path)) <= 0)
+			return -ENOMEM;
+		fspath = decorated;
+	}
+
 	if (cfg.fs_config_file)
-		canned_fs_config(erofs_fspath(path),
+		canned_fs_config(fspath,
 				 S_ISDIR(st->st_mode),
 				 cfg.target_out_path,
 				 &uid, &gid, &mode, &inode->capabilities);
-	else if (cfg.mount_point) {
-		if (asprintf(&fspath, "%s/%s", cfg.mount_point,
-			     erofs_fspath(path)) <= 0)
-			return -ENOMEM;
-
+	else
 		fs_config(fspath, S_ISDIR(st->st_mode),
 			  cfg.target_out_path,
 			  &uid, &gid, &mode, &inode->capabilities);
-		free(fspath);
-	}
-	st->st_uid = uid;
-	st->st_gid = gid;
-	st->st_mode = mode | stat_file_type_mask;
 
 	erofs_dbg("/%s -> mode = 0x%x, uid = 0x%x, gid = 0x%x, "
 		  "capabilities = 0x%" PRIx64 "\n",
-		  erofs_fspath(path),
-		  mode, uid, gid, inode->capabilities);
+		  fspath, mode, uid, gid, inode->capabilities);
+
+	if (decorated)
+		free(decorated);
+	st->st_uid = uid;
+	st->st_gid = gid;
+	st->st_mode = mode | stat_file_type_mask;
 	return 0;
 }
 #else
-- 
2.24.0

Re: [PATCH] AOSP: erofs-utils: fix sub-directory prefix for canned fs_config

[Yue Hu]

On Sat, 26 Dec 2020 14:27:36 +0800
Gao Xiang <hsiangkao@aol.com> wrote:

> From: Gao Xiang <hsiangkao@aol.com>
> 
> "failed to find [%s] in canned fs_config" was observed by using
> "--fs-config-file" option as reported by Yue Hu [1].
> 
> The root cause was that the mountpoint prefix to subdirectories is
> also needed if "--mount-point" presents. However, such prefix cannot
> be added by just using erofs_fspath().
> 
> One exception is that the root directory itself needs to be handled
> specially for canned fs_config. For such case, the prefix of the root
> directory has to be dropped instead.
> 
> [1]
> https://lkml.kernel.org/r/20201222020430.12512-1-zbestahu@gmail.com
> 
> Fixes: 8a9e8046f170 ("AOSP: erofs-utils: add fs_config support")
> Reported-by: Yue Hu <huyue2@yulong.com>
> Signed-off-by: Gao Xiang <hsiangkao@aol.com>
> ---
> Hi Yue, Jianan,
> 
> I've verified cuttlefish booting with success, It'd be better to
> verify this patchset on your sides. Please kingly leave "Tested-by:"
> if possible.

Hi Xiang,

I'm testing now. I will inform once finished.

Thx.

> 
> Hi Guifu,
> Could you also review this patch ? This needs to be included in
> the upcoming v1.2.1 as well...
> 
> Thanks,
> Gao Xiang
> 
>  lib/inode.c | 39 +++++++++++++++++++++++++--------------
>  1 file changed, 25 insertions(+), 14 deletions(-)
> 
> diff --git a/lib/inode.c b/lib/inode.c
> index 0c4839dc7152..f88966d26fce 100644
> --- a/lib/inode.c
> +++ b/lib/inode.c
> @@ -696,32 +696,43 @@ int erofs_droid_inode_fsconfig(struct
> erofs_inode *inode, /* filesystem_config does not preserve file type
> bits */ mode_t stat_file_type_mask = st->st_mode & S_IFMT;
>  	unsigned int uid = 0, gid = 0, mode = 0;
> -	char *fspath;
> +	const char *fspath;
> +	char *decorated = NULL;
>  
>  	inode->capabilities = 0;
> +	if (!cfg.fs_config_file && !cfg.mount_point)
> +		return 0;
> +
> +	if (!cfg.mount_point ||
> +	/* have to drop the mountpoint for rootdir of canned
> fsconfig */
> +	    (cfg.fs_config_file && IS_ROOT(inode))) {
> +		fspath = erofs_fspath(path);
> +	} else {
> +		if (asprintf(&decorated, "%s/%s", cfg.mount_point,
> +			     erofs_fspath(path)) <= 0)
> +			return -ENOMEM;
> +		fspath = decorated;
> +	}
> +
>  	if (cfg.fs_config_file)
> -		canned_fs_config(erofs_fspath(path),
> +		canned_fs_config(fspath,
>  				 S_ISDIR(st->st_mode),
>  				 cfg.target_out_path,
>  				 &uid, &gid, &mode,
> &inode->capabilities);
> -	else if (cfg.mount_point) {
> -		if (asprintf(&fspath, "%s/%s", cfg.mount_point,
> -			     erofs_fspath(path)) <= 0)
> -			return -ENOMEM;
> -
> +	else
>  		fs_config(fspath, S_ISDIR(st->st_mode),
>  			  cfg.target_out_path,
>  			  &uid, &gid, &mode, &inode->capabilities);
> -		free(fspath);
> -	}
> -	st->st_uid = uid;
> -	st->st_gid = gid;
> -	st->st_mode = mode | stat_file_type_mask;
>  
>  	erofs_dbg("/%s -> mode = 0x%x, uid = 0x%x, gid = 0x%x, "
>  		  "capabilities = 0x%" PRIx64 "\n",
> -		  erofs_fspath(path),
> -		  mode, uid, gid, inode->capabilities);
> +		  fspath, mode, uid, gid, inode->capabilities);
> +
> +	if (decorated)
> +		free(decorated);
> +	st->st_uid = uid;
> +	st->st_gid = gid;
> +	st->st_mode = mode | stat_file_type_mask;
>  	return 0;
>  }
>  #else

Re: [PATCH] AOSP: erofs-utils: fix sub-directory prefix for canned fs_config

[Huang Jianan]

在 2020/12/26 14:27, Gao Xiang 写道:
> From: Gao Xiang <hsiangkao@aol.com>
>
> "failed to find [%s] in canned fs_config" was observed by using
> "--fs-config-file" option as reported by Yue Hu [1].
>
> The root cause was that the mountpoint prefix to subdirectories is
> also needed if "--mount-point" presents. However, such prefix cannot
> be added by just using erofs_fspath().
>
> One exception is that the root directory itself needs to be handled
> specially for canned fs_config. For such case, the prefix of the root
> directory has to be dropped instead.
>
> [1] https://lkml.kernel.org/r/20201222020430.12512-1-zbestahu@gmail.com
>
> Fixes: 8a9e8046f170 ("AOSP: erofs-utils: add fs_config support")
> Reported-by: Yue Hu <huyue2@yulong.com>
> Signed-off-by: Gao Xiang <hsiangkao@aol.com>
> ---
> Hi Yue, Jianan,
>
> I've verified cuttlefish booting with success, It'd be better to
> verify this patchset on your sides. Please kingly leave "Tested-by:"
> if possible.
>
> Hi Guifu,
> Could you also review this patch ? This needs to be included in
> the upcoming v1.2.1 as well...
>
> Thanks,
> Gao Xiang
>
>   lib/inode.c | 39 +++++++++++++++++++++++++--------------
>   1 file changed, 25 insertions(+), 14 deletions(-)
>
> diff --git a/lib/inode.c b/lib/inode.c
> index 0c4839dc7152..f88966d26fce 100644
> --- a/lib/inode.c
> +++ b/lib/inode.c
> @@ -696,32 +696,43 @@ int erofs_droid_inode_fsconfig(struct erofs_inode *inode,
>   	/* filesystem_config does not preserve file type bits */
>   	mode_t stat_file_type_mask = st->st_mode & S_IFMT;
>   	unsigned int uid = 0, gid = 0, mode = 0;
> -	char *fspath;
> +	const char *fspath;
> +	char *decorated = NULL;
>   
>   	inode->capabilities = 0;
> +	if (!cfg.fs_config_file && !cfg.mount_point)
> +		return 0;
> +
> +	if (!cfg.mount_point ||
> +	/* have to drop the mountpoint for rootdir of canned fsconfig */
> +	    (cfg.fs_config_file && IS_ROOT(inode))) {

Hi Xiang,

I have tested this patch with --fs-config-file, and the problem still 
exists.

IS_ROOT can't be used here to determine the root directory , because 
inode->i_parent is null at this time. It will be set after this function.

Thanks,

Jianan

> +		fspath = erofs_fspath(path);
> +	} else {
> +		if (asprintf(&decorated, "%s/%s", cfg.mount_point,
> +			     erofs_fspath(path)) <= 0)
> +			return -ENOMEM;
> +		fspath = decorated;
> +	}
> +
>   	if (cfg.fs_config_file)
> -		canned_fs_config(erofs_fspath(path),
> +		canned_fs_config(fspath,
>   				 S_ISDIR(st->st_mode),
>   				 cfg.target_out_path,
>   				 &uid, &gid, &mode, &inode->capabilities);
> -	else if (cfg.mount_point) {
> -		if (asprintf(&fspath, "%s/%s", cfg.mount_point,
> -			     erofs_fspath(path)) <= 0)
> -			return -ENOMEM;
> -
> +	else
>   		fs_config(fspath, S_ISDIR(st->st_mode),
>   			  cfg.target_out_path,
>   			  &uid, &gid, &mode, &inode->capabilities);
> -		free(fspath);
> -	}
> -	st->st_uid = uid;
> -	st->st_gid = gid;
> -	st->st_mode = mode | stat_file_type_mask;
>   
>   	erofs_dbg("/%s -> mode = 0x%x, uid = 0x%x, gid = 0x%x, "
>   		  "capabilities = 0x%" PRIx64 "\n",
> -		  erofs_fspath(path),
> -		  mode, uid, gid, inode->capabilities);
> +		  fspath, mode, uid, gid, inode->capabilities);
> +
> +	if (decorated)
> +		free(decorated);
> +	st->st_uid = uid;
> +	st->st_gid = gid;
> +	st->st_mode = mode | stat_file_type_mask;
>   	return 0;
>   }
>   #else

Re: [PATCH] AOSP: erofs-utils: fix sub-directory prefix for canned fs_config

[Gao Xiang]

On Mon, Dec 28, 2020 at 06:03:26PM +0800, Huang Jianan wrote:
> 
> 在 2020/12/26 14:27, Gao Xiang 写道:

...

> > +	if (!cfg.mount_point ||
> > +	/* have to drop the mountpoint for rootdir of canned fsconfig */
> > +	    (cfg.fs_config_file && IS_ROOT(inode))) {
> 
> Hi Xiang,
> 
> I have tested this patch with --fs-config-file, and the problem still
> exists.
> 
> IS_ROOT can't be used here to determine the root directory , because
> inode->i_parent is null at this time. It will be set after this function.
> 

Hi Jianan,

Thanks for your report! Sorry for the troublesome.
I need to turn back as the next version.... Very sorry about this!

Thanks,
Gao Xiang

> Thanks,
> 
> Jianan
> 
> > +		fspath = erofs_fspath(path);
> > +	} else {
> > +		if (asprintf(&decorated, "%s/%s", cfg.mount_point,
> > +			     erofs_fspath(path)) <= 0)
> > +			return -ENOMEM;
> > +		fspath = decorated;
> > +	}
> > +
> >   	if (cfg.fs_config_file)
> > -		canned_fs_config(erofs_fspath(path),
> > +		canned_fs_config(fspath,
> >   				 S_ISDIR(st->st_mode),
> >   				 cfg.target_out_path,
> >   				 &uid, &gid, &mode, &inode->capabilities);
> > -	else if (cfg.mount_point) {
> > -		if (asprintf(&fspath, "%s/%s", cfg.mount_point,
> > -			     erofs_fspath(path)) <= 0)
> > -			return -ENOMEM;
> > -
> > +	else
> >   		fs_config(fspath, S_ISDIR(st->st_mode),
> >   			  cfg.target_out_path,
> >   			  &uid, &gid, &mode, &inode->capabilities);
> > -		free(fspath);
> > -	}
> > -	st->st_uid = uid;
> > -	st->st_gid = gid;
> > -	st->st_mode = mode | stat_file_type_mask;
> >   	erofs_dbg("/%s -> mode = 0x%x, uid = 0x%x, gid = 0x%x, "
> >   		  "capabilities = 0x%" PRIx64 "\n",
> > -		  erofs_fspath(path),
> > -		  mode, uid, gid, inode->capabilities);
> > +		  fspath, mode, uid, gid, inode->capabilities);
> > +
> > +	if (decorated)
> > +		free(decorated);
> > +	st->st_uid = uid;
> > +	st->st_gid = gid;
> > +	st->st_mode = mode | stat_file_type_mask;
> >   	return 0;
> >   }
> >   #else
> 

[PATCH v2] AOSP: erofs-utils: fix sub-directory prefix for canned fs_config

[Gao Xiang]

From: Gao Xiang <hsiangkao@aol.com>

"failed to find [%s] in canned fs_config" was observed by using
"--fs-config-file" option as reported by Yue Hu [1].

The root cause was that the mountpoint prefix to subdirectories is
also needed if "--mount-point" presents. However, such prefix cannot
be added by just using erofs_fspath().

One exception is that the root directory itself needs to be handled
specially for canned fs_config. For such case, the prefix of the root
directory has to be dropped instead.

[1] https://lkml.kernel.org/r/20201222020430.12512-1-zbestahu@gmail.com

Link: https://lore.kernel.org/r/20201226062736.29920-1-hsiangkao@aol.com
Fixes: 8a9e8046f170 ("AOSP: erofs-utils: add fs_config support")
Reported-by: Yue Hu <huyue2@yulong.com>
Signed-off-by: Gao Xiang <hsiangkao@aol.com>
---
changes since v2:
 - fix IS_ROOT misuse reported by Jianan, very sorry about this since
   I know little about canned fs_config.

(please kindly test again...)

 lib/inode.c | 39 +++++++++++++++++++++++++--------------
 1 file changed, 25 insertions(+), 14 deletions(-)

diff --git a/lib/inode.c b/lib/inode.c
index 0c4839d..e6159c9 100644
--- a/lib/inode.c
+++ b/lib/inode.c
@@ -696,32 +696,43 @@ int erofs_droid_inode_fsconfig(struct erofs_inode *inode,
 	/* filesystem_config does not preserve file type bits */
 	mode_t stat_file_type_mask = st->st_mode & S_IFMT;
 	unsigned int uid = 0, gid = 0, mode = 0;
-	char *fspath;
+	const char *fspath;
+	char *decorated = NULL;
 
 	inode->capabilities = 0;
+	if (!cfg.fs_config_file && !cfg.mount_point)
+		return 0;
+
+	if (!cfg.mount_point ||
+	/* have to drop the mountpoint for rootdir of canned fsconfig */
+	    (cfg.fs_config_file && erofs_fspath(path)[0] == '\0')) {
+		fspath = erofs_fspath(path);
+	} else {
+		if (asprintf(&decorated, "%s/%s", cfg.mount_point,
+			     erofs_fspath(path)) <= 0)
+			return -ENOMEM;
+		fspath = decorated;
+	}
+
 	if (cfg.fs_config_file)
-		canned_fs_config(erofs_fspath(path),
+		canned_fs_config(fspath,
 				 S_ISDIR(st->st_mode),
 				 cfg.target_out_path,
 				 &uid, &gid, &mode, &inode->capabilities);
-	else if (cfg.mount_point) {
-		if (asprintf(&fspath, "%s/%s", cfg.mount_point,
-			     erofs_fspath(path)) <= 0)
-			return -ENOMEM;
-
+	else
 		fs_config(fspath, S_ISDIR(st->st_mode),
 			  cfg.target_out_path,
 			  &uid, &gid, &mode, &inode->capabilities);
-		free(fspath);
-	}
-	st->st_uid = uid;
-	st->st_gid = gid;
-	st->st_mode = mode | stat_file_type_mask;
 
 	erofs_dbg("/%s -> mode = 0x%x, uid = 0x%x, gid = 0x%x, "
 		  "capabilities = 0x%" PRIx64 "\n",
-		  erofs_fspath(path),
-		  mode, uid, gid, inode->capabilities);
+		  fspath, mode, uid, gid, inode->capabilities);
+
+	if (decorated)
+		free(decorated);
+	st->st_uid = uid;
+	st->st_gid = gid;
+	st->st_mode = mode | stat_file_type_mask;
 	return 0;
 }
 #else
-- 
2.27.0

Re: [PATCH v2] AOSP: erofs-utils: fix sub-directory prefix for canned fs_config

[Huang Jianan]

Hi Xiang,

This patch works well with or without --fs-config-file in build.

Tested-by: Huang Jianan <huangjianan@oppo.com>

Thanks,

Jianan

在 2020/12/28 18:51, Gao Xiang 写道:
> From: Gao Xiang <hsiangkao@aol.com>
>
> "failed to find [%s] in canned fs_config" was observed by using
> "--fs-config-file" option as reported by Yue Hu [1].
>
> The root cause was that the mountpoint prefix to subdirectories is
> also needed if "--mount-point" presents. However, such prefix cannot
> be added by just using erofs_fspath().
>
> One exception is that the root directory itself needs to be handled
> specially for canned fs_config. For such case, the prefix of the root
> directory has to be dropped instead.
>
> [1] https://lkml.kernel.org/r/20201222020430.12512-1-zbestahu@gmail.com
>
> Link: https://lore.kernel.org/r/20201226062736.29920-1-hsiangkao@aol.com
> Fixes: 8a9e8046f170 ("AOSP: erofs-utils: add fs_config support")
> Reported-by: Yue Hu <huyue2@yulong.com>
> Signed-off-by: Gao Xiang <hsiangkao@aol.com>
> ---
> changes since v2:
>   - fix IS_ROOT misuse reported by Jianan, very sorry about this since
>     I know little about canned fs_config.
>
> (please kindly test again...)
>
>   lib/inode.c | 39 +++++++++++++++++++++++++--------------
>   1 file changed, 25 insertions(+), 14 deletions(-)
>
> diff --git a/lib/inode.c b/lib/inode.c
> index 0c4839d..e6159c9 100644
> --- a/lib/inode.c
> +++ b/lib/inode.c
> @@ -696,32 +696,43 @@ int erofs_droid_inode_fsconfig(struct erofs_inode *inode,
>   	/* filesystem_config does not preserve file type bits */
>   	mode_t stat_file_type_mask = st->st_mode & S_IFMT;
>   	unsigned int uid = 0, gid = 0, mode = 0;
> -	char *fspath;
> +	const char *fspath;
> +	char *decorated = NULL;
>   
>   	inode->capabilities = 0;
> +	if (!cfg.fs_config_file && !cfg.mount_point)
> +		return 0;
> +
> +	if (!cfg.mount_point ||
> +	/* have to drop the mountpoint for rootdir of canned fsconfig */
> +	    (cfg.fs_config_file && erofs_fspath(path)[0] == '\0')) {
> +		fspath = erofs_fspath(path);
> +	} else {
> +		if (asprintf(&decorated, "%s/%s", cfg.mount_point,
> +			     erofs_fspath(path)) <= 0)
> +			return -ENOMEM;
> +		fspath = decorated;
> +	}
> +
>   	if (cfg.fs_config_file)
> -		canned_fs_config(erofs_fspath(path),
> +		canned_fs_config(fspath,
>   				 S_ISDIR(st->st_mode),
>   				 cfg.target_out_path,
>   				 &uid, &gid, &mode, &inode->capabilities);
> -	else if (cfg.mount_point) {
> -		if (asprintf(&fspath, "%s/%s", cfg.mount_point,
> -			     erofs_fspath(path)) <= 0)
> -			return -ENOMEM;
> -
> +	else
>   		fs_config(fspath, S_ISDIR(st->st_mode),
>   			  cfg.target_out_path,
>   			  &uid, &gid, &mode, &inode->capabilities);
> -		free(fspath);
> -	}
> -	st->st_uid = uid;
> -	st->st_gid = gid;
> -	st->st_mode = mode | stat_file_type_mask;
>   
>   	erofs_dbg("/%s -> mode = 0x%x, uid = 0x%x, gid = 0x%x, "
>   		  "capabilities = 0x%" PRIx64 "\n",
> -		  erofs_fspath(path),
> -		  mode, uid, gid, inode->capabilities);
> +		  fspath, mode, uid, gid, inode->capabilities);
> +
> +	if (decorated)
> +		free(decorated);
> +	st->st_uid = uid;
> +	st->st_gid = gid;
> +	st->st_mode = mode | stat_file_type_mask;
>   	return 0;
>   }
>   #else

Re: [PATCH v2] AOSP: erofs-utils: fix sub-directory prefix for canned fs_config

[Yue Hu]

On Mon, 28 Dec 2020 18:51:46 +0800
Gao Xiang <hsiangkao@redhat.com> wrote:

> From: Gao Xiang <hsiangkao@aol.com>
> 
> "failed to find [%s] in canned fs_config" was observed by using
> "--fs-config-file" option as reported by Yue Hu [1].
> 
> The root cause was that the mountpoint prefix to subdirectories is
> also needed if "--mount-point" presents. However, such prefix cannot
> be added by just using erofs_fspath().
> 
> One exception is that the root directory itself needs to be handled
> specially for canned fs_config. For such case, the prefix of the root
> directory has to be dropped instead.
> 
> [1]
> https://lkml.kernel.org/r/20201222020430.12512-1-zbestahu@gmail.com
> 
> Link:
> https://lore.kernel.org/r/20201226062736.29920-1-hsiangkao@aol.com
> Fixes: 8a9e8046f170 ("AOSP: erofs-utils: add fs_config support")
> Reported-by: Yue Hu <huyue2@yulong.com> Signed-off-by: Gao Xiang
> <hsiangkao@aol.com> ---
> changes since v2:
>  - fix IS_ROOT misuse reported by Jianan, very sorry about this since
>    I know little about canned fs_config.
> 
> (please kindly test again...)
> 
>  lib/inode.c | 39 +++++++++++++++++++++++++--------------
>  1 file changed, 25 insertions(+), 14 deletions(-)
> 
> diff --git a/lib/inode.c b/lib/inode.c
> index 0c4839d..e6159c9 100644
> --- a/lib/inode.c
> +++ b/lib/inode.c
> @@ -696,32 +696,43 @@ int erofs_droid_inode_fsconfig(struct
> erofs_inode *inode, /* filesystem_config does not preserve file type
> bits */ mode_t stat_file_type_mask = st->st_mode & S_IFMT;
>  	unsigned int uid = 0, gid = 0, mode = 0;
> -	char *fspath;
> +	const char *fspath;
> +	char *decorated = NULL;
>  
>  	inode->capabilities = 0;
> +	if (!cfg.fs_config_file && !cfg.mount_point)
> +		return 0;
> +
> +	if (!cfg.mount_point ||
> +	/* have to drop the mountpoint for rootdir of canned
> fsconfig */
> +	    (cfg.fs_config_file && erofs_fspath(path)[0] == '\0')) {
> +		fspath = erofs_fspath(path);
> +	} else {
> +		if (asprintf(&decorated, "%s/%s", cfg.mount_point,
> +			     erofs_fspath(path)) <= 0)
> +			return -ENOMEM;
> +		fspath = decorated;
> +	}
> +
>  	if (cfg.fs_config_file)

Whether we can use the first "cfg.fs_config_file" when loading canned
fs-config to reduce/simplify these duplicated calling?

> -		canned_fs_config(erofs_fspath(path),
> +		canned_fs_config(fspath,
>  				 S_ISDIR(st->st_mode),
>  				 cfg.target_out_path,
>  				 &uid, &gid, &mode,
> &inode->capabilities);
> -	else if (cfg.mount_point) {
> -		if (asprintf(&fspath, "%s/%s", cfg.mount_point,
> -			     erofs_fspath(path)) <= 0)
> -			return -ENOMEM;
> -
> +	else
>  		fs_config(fspath, S_ISDIR(st->st_mode),
>  			  cfg.target_out_path,
>  			  &uid, &gid, &mode, &inode->capabilities);
> -		free(fspath);
> -	}
> -	st->st_uid = uid;
> -	st->st_gid = gid;
> -	st->st_mode = mode | stat_file_type_mask;
>  
>  	erofs_dbg("/%s -> mode = 0x%x, uid = 0x%x, gid = 0x%x, "
>  		  "capabilities = 0x%" PRIx64 "\n",
> -		  erofs_fspath(path),
> -		  mode, uid, gid, inode->capabilities);
> +		  fspath, mode, uid, gid, inode->capabilities);
> +
> +	if (decorated)
> +		free(decorated);
> +	st->st_uid = uid;
> +	st->st_gid = gid;
> +	st->st_mode = mode | stat_file_type_mask;
>  	return 0;
>  }
>  #else

Re: [PATCH v2] AOSP: erofs-utils: fix sub-directory prefix for canned fs_config

[Gao Xiang]

Hi Yue,

On Mon, Dec 28, 2020 at 07:20:48PM +0800, Yue Hu wrote:
> On Mon, 28 Dec 2020 18:51:46 +0800
> Gao Xiang <hsiangkao@redhat.com> wrote:

...

> > @@ -696,32 +696,43 @@ int erofs_droid_inode_fsconfig(struct
> > erofs_inode *inode, /* filesystem_config does not preserve file type
> > bits */ mode_t stat_file_type_mask = st->st_mode & S_IFMT;
> >  	unsigned int uid = 0, gid = 0, mode = 0;
> > -	char *fspath;
> > +	const char *fspath;
> > +	char *decorated = NULL;
> >  
> >  	inode->capabilities = 0;
> > +	if (!cfg.fs_config_file && !cfg.mount_point)
> > +		return 0;
> > +
> > +	if (!cfg.mount_point ||
> > +	/* have to drop the mountpoint for rootdir of canned
> > fsconfig */
> > +	    (cfg.fs_config_file && erofs_fspath(path)[0] == '\0')) {
> > +		fspath = erofs_fspath(path);
> > +	} else {
> > +		if (asprintf(&decorated, "%s/%s", cfg.mount_point,
> > +			     erofs_fspath(path)) <= 0)
> > +			return -ENOMEM;
> > +		fspath = decorated;
> > +	}
> > +
> >  	if (cfg.fs_config_file)
> 
> Whether we can use the first "cfg.fs_config_file" when loading canned
> fs-config to reduce/simplify these duplicated calling?

Not sure what you mean... If you mean why not using some "fs_config_func"
as squashfs did, that is I'd like to 1) avoid such unneeded indirect
function pointers, and 2) no need to introduce such function prototype (I
don't want to maintain such function pointer type) since fs_config and
canned_fs_config implement differently (and it seems they also behave
differently so no need to mix them at all).

Thanks,
Gao Xiang

Re: [PATCH v2] AOSP: erofs-utils: fix sub-directory prefix for canned fs_config

[Yue Hu]

On Mon, 28 Dec 2020 19:32:47 +0800
Gao Xiang <hsiangkao@redhat.com> wrote:

> Hi Yue,
> 
> On Mon, Dec 28, 2020 at 07:20:48PM +0800, Yue Hu wrote:
> > On Mon, 28 Dec 2020 18:51:46 +0800
> > Gao Xiang <hsiangkao@redhat.com> wrote:  
> 
> ...
> 
> > > @@ -696,32 +696,43 @@ int erofs_droid_inode_fsconfig(struct
> > > erofs_inode *inode, /* filesystem_config does not preserve file
> > > type bits */ mode_t stat_file_type_mask = st->st_mode & S_IFMT;
> > >  	unsigned int uid = 0, gid = 0, mode = 0;
> > > -	char *fspath;
> > > +	const char *fspath;
> > > +	char *decorated = NULL;
> > >  
> > >  	inode->capabilities = 0;
> > > +	if (!cfg.fs_config_file && !cfg.mount_point)
> > > +		return 0;
> > > +
> > > +	if (!cfg.mount_point ||
> > > +	/* have to drop the mountpoint for rootdir of canned
> > > fsconfig */
> > > +	    (cfg.fs_config_file && erofs_fspath(path)[0] ==
> > > '\0')) {
> > > +		fspath = erofs_fspath(path);
> > > +	} else {
> > > +		if (asprintf(&decorated, "%s/%s",
> > > cfg.mount_point,
> > > +			     erofs_fspath(path)) <= 0)
> > > +			return -ENOMEM;
> > > +		fspath = decorated;
> > > +	}
> > > +
> > >  	if (cfg.fs_config_file)  
> > 
> > Whether we can use the first "cfg.fs_config_file" when loading
> > canned fs-config to reduce/simplify these duplicated calling?  
> 
> Not sure what you mean... If you mean why not using some

Sorry for my typo. I mean duplicated 'check' to cfg.fs_config_file.

> "fs_config_func" as squashfs did, that is I'd like to 1) avoid such
> unneeded indirect function pointers, and 2) no need to introduce such
> function prototype (I don't want to maintain such function pointer
> type) since fs_config and canned_fs_config implement differently (and
> it seems they also behave differently so no need to mix them at all).

Ok.

> 
> Thanks,
> Gao Xiang
> 

Re: [PATCH v2] AOSP: erofs-utils: fix sub-directory prefix for canned fs_config

[Yue Hu]

Hi Xiang,

Works fine to me for canned/non-canned fs_config.

Tested-by: Yue Hu <huyue2@yulong.com>

Thx.

On Mon, 28 Dec 2020 18:51:46 +0800
Gao Xiang <hsiangkao@redhat.com> wrote:

> From: Gao Xiang <hsiangkao@aol.com>
> 
> "failed to find [%s] in canned fs_config" was observed by using
> "--fs-config-file" option as reported by Yue Hu [1].
> 
> The root cause was that the mountpoint prefix to subdirectories is
> also needed if "--mount-point" presents. However, such prefix cannot
> be added by just using erofs_fspath().
> 
> One exception is that the root directory itself needs to be handled
> specially for canned fs_config. For such case, the prefix of the root
> directory has to be dropped instead.
> 
> [1]
> https://lkml.kernel.org/r/20201222020430.12512-1-zbestahu@gmail.com
> 
> Link:
> https://lore.kernel.org/r/20201226062736.29920-1-hsiangkao@aol.com
> Fixes: 8a9e8046f170 ("AOSP: erofs-utils: add fs_config support")
> Reported-by: Yue Hu <huyue2@yulong.com> Signed-off-by: Gao Xiang
> <hsiangkao@aol.com> ---
> changes since v2:
>  - fix IS_ROOT misuse reported by Jianan, very sorry about this since
>    I know little about canned fs_config.
> 
> (please kindly test again...)
> 
>  lib/inode.c | 39 +++++++++++++++++++++++++--------------
>  1 file changed, 25 insertions(+), 14 deletions(-)
> 
> diff --git a/lib/inode.c b/lib/inode.c
> index 0c4839d..e6159c9 100644
> --- a/lib/inode.c
> +++ b/lib/inode.c
> @@ -696,32 +696,43 @@ int erofs_droid_inode_fsconfig(struct
> erofs_inode *inode, /* filesystem_config does not preserve file type
> bits */ mode_t stat_file_type_mask = st->st_mode & S_IFMT;
>  	unsigned int uid = 0, gid = 0, mode = 0;
> -	char *fspath;
> +	const char *fspath;
> +	char *decorated = NULL;
>  
>  	inode->capabilities = 0;
> +	if (!cfg.fs_config_file && !cfg.mount_point)
> +		return 0;
> +
> +	if (!cfg.mount_point ||
> +	/* have to drop the mountpoint for rootdir of canned
> fsconfig */
> +	    (cfg.fs_config_file && erofs_fspath(path)[0] == '\0')) {
> +		fspath = erofs_fspath(path);
> +	} else {
> +		if (asprintf(&decorated, "%s/%s", cfg.mount_point,
> +			     erofs_fspath(path)) <= 0)
> +			return -ENOMEM;
> +		fspath = decorated;
> +	}
> +
>  	if (cfg.fs_config_file)
> -		canned_fs_config(erofs_fspath(path),
> +		canned_fs_config(fspath,
>  				 S_ISDIR(st->st_mode),
>  				 cfg.target_out_path,
>  				 &uid, &gid, &mode,
> &inode->capabilities);
> -	else if (cfg.mount_point) {
> -		if (asprintf(&fspath, "%s/%s", cfg.mount_point,
> -			     erofs_fspath(path)) <= 0)
> -			return -ENOMEM;
> -
> +	else
>  		fs_config(fspath, S_ISDIR(st->st_mode),
>  			  cfg.target_out_path,
>  			  &uid, &gid, &mode, &inode->capabilities);
> -		free(fspath);
> -	}
> -	st->st_uid = uid;
> -	st->st_gid = gid;
> -	st->st_mode = mode | stat_file_type_mask;
>  
>  	erofs_dbg("/%s -> mode = 0x%x, uid = 0x%x, gid = 0x%x, "
>  		  "capabilities = 0x%" PRIx64 "\n",
> -		  erofs_fspath(path),
> -		  mode, uid, gid, inode->capabilities);
> +		  fspath, mode, uid, gid, inode->capabilities);
> +
> +	if (decorated)
> +		free(decorated);
> +	st->st_uid = uid;
> +	st->st_gid = gid;
> +	st->st_mode = mode | stat_file_type_mask;
>  	return 0;
>  }
>  #else

Re: [PATCH v2] AOSP: erofs-utils: fix sub-directory prefix for canned fs_config

[Gao Xiang]

On Mon, Dec 28, 2020 at 07:46:56PM +0800, Yue Hu wrote:
> Hi Xiang,
> 
> Works fine to me for canned/non-canned fs_config.
> 
> Tested-by: Yue Hu <huyue2@yulong.com>
> 

Okay, got it. thanks you all for this :) 

(also another link for indirect call: https://lwn.net/Articles/774743/
 , so as long as some switch table, I'd like to avoid such usage...)

Thanks,
Gao Xiang

> Thx.

Re: [PATCH v2] AOSP: erofs-utils: fix sub-directory prefix for canned fs_config

[Li GuiFu via Linux-erofs]

Thank for your works, Yue Hu, Jianan, Gao Xiang

On 2020/12/28 18:51, Gao Xiang wrote:
> From: Gao Xiang <hsiangkao@aol.com>
> 
> "failed to find [%s] in canned fs_config" was observed by using
> "--fs-config-file" option as reported by Yue Hu [1].
> 
> The root cause was that the mountpoint prefix to subdirectories is
> also needed if "--mount-point" presents. However, such prefix cannot
> be added by just using erofs_fspath().
> 
> One exception is that the root directory itself needs to be handled
> specially for canned fs_config. For such case, the prefix of the root
> directory has to be dropped instead.
> 
> [1] https://lkml.kernel.org/r/20201222020430.12512-1-zbestahu@gmail.com
> 
> Link: https://lore.kernel.org/r/20201226062736.29920-1-hsiangkao@aol.com
> Fixes: 8a9e8046f170 ("AOSP: erofs-utils: add fs_config support")
> Reported-by: Yue Hu <huyue2@yulong.com>
> Signed-off-by: Gao Xiang <hsiangkao@aol.com>
> ---
> changes since v2:
>  - fix IS_ROOT misuse reported by Jianan, very sorry about this since
>    I know little about canned fs_config.
> 
> (please kindly test again...)
> 
>  lib/inode.c | 39 +++++++++++++++++++++++++--------------
>  1 file changed, 25 insertions(+), 14 deletions(-)
> 
> diff --git a/lib/inode.c b/lib/inode.c
> index 0c4839d..e6159c9 100644
> --- a/lib/inode.c
> +++ b/lib/inode.c
> @@ -696,32 +696,43 @@ int erofs_droid_inode_fsconfig(struct erofs_inode *inode,
>  	/* filesystem_config does not preserve file type bits */
>  	mode_t stat_file_type_mask = st->st_mode & S_IFMT;
>  	unsigned int uid = 0, gid = 0, mode = 0;
> -	char *fspath;
> +	const char *fspath;
> +	char *decorated = NULL;
>  
>  	inode->capabilities = 0;
> +	if (!cfg.fs_config_file && !cfg.mount_point)
> +		return 0;
> +
> +	if (!cfg.mount_point ||
> +	/* have to drop the mountpoint for rootdir of canned fsconfig */
> +	    (cfg.fs_config_file && erofs_fspath(path)[0] == '\0')) {
> +		fspath = erofs_fspath(path);
> +	} else {
> +		if (asprintf(&decorated, "%s/%s", cfg.mount_point,
> +			     erofs_fspath(path)) <= 0)
> +			return -ENOMEM;
> +		fspath = decorated;
> +	}
> +
erofs_fspath has been written for three times, and called always.
What do you think refact it ? Do it with
fspath = erofs_fspath(path);
if need decorated:
   fspath = decorated

Re: [PATCH v2] AOSP: erofs-utils: fix sub-directory prefix for canned fs_config

[Li GuiFu via Linux-erofs]

On 2020/12/28 18:51, Gao Xiang wrote:
> From: Gao Xiang <hsiangkao@aol.com>
> 
> "failed to find [%s] in canned fs_config" was observed by using
> "--fs-config-file" option as reported by Yue Hu [1].
> 
> The root cause was that the mountpoint prefix to subdirectories is
> also needed if "--mount-point" presents. However, such prefix cannot
> be added by just using erofs_fspath().
> 
> One exception is that the root directory itself needs to be handled
> specially for canned fs_config. For such case, the prefix of the root
> directory has to be dropped instead.
> 
> [1] https://lkml.kernel.org/r/20201222020430.12512-1-zbestahu@gmail.com
> 
> Link: https://lore.kernel.org/r/20201226062736.29920-1-hsiangkao@aol.com
> Fixes: 8a9e8046f170 ("AOSP: erofs-utils: add fs_config support")
> Reported-by: Yue Hu <huyue2@yulong.com>
> Signed-off-by: Gao Xiang <hsiangkao@aol.com>
> ---
> changes since v2:
>  - fix IS_ROOT misuse reported by Jianan, very sorry about this since
>    I know little about canned fs_config.
> 
> (please kindly test again...)
> 
>  lib/inode.c | 39 +++++++++++++++++++++++++--------------
>  1 file changed, 25 insertions(+), 14 deletions(-)
> 

It looks good
Reviewed-by: Li Guifu <bluce.lee@aliyun.com>

Thanks,

Re: [PATCH v2] AOSP: erofs-utils: fix sub-directory prefix for canned fs_config

[Gao Xiang]

Hi Guifu,

On Fri, Jan 01, 2021 at 12:31:22AM +0800, Li GuiFu wrote:
> Thank for your works, Yue Hu, Jianan, Gao Xiang
> 
> On 2020/12/28 18:51, Gao Xiang wrote:

...

> > diff --git a/lib/inode.c b/lib/inode.c
> > index 0c4839d..e6159c9 100644
> > --- a/lib/inode.c
> > +++ b/lib/inode.c
> > @@ -696,32 +696,43 @@ int erofs_droid_inode_fsconfig(struct erofs_inode *inode,
> >  	/* filesystem_config does not preserve file type bits */
> >  	mode_t stat_file_type_mask = st->st_mode & S_IFMT;
> >  	unsigned int uid = 0, gid = 0, mode = 0;
> > -	char *fspath;
> > +	const char *fspath;
> > +	char *decorated = NULL;
> >  
> >  	inode->capabilities = 0;
> > +	if (!cfg.fs_config_file && !cfg.mount_point)
> > +		return 0;
> > +
> > +	if (!cfg.mount_point ||
> > +	/* have to drop the mountpoint for rootdir of canned fsconfig */
> > +	    (cfg.fs_config_file && erofs_fspath(path)[0] == '\0')) {
> > +		fspath = erofs_fspath(path);
> > +	} else {
> > +		if (asprintf(&decorated, "%s/%s", cfg.mount_point,
> > +			     erofs_fspath(path)) <= 0)
> > +			return -ENOMEM;
> > +		fspath = decorated;
> > +	}
> > +
> erofs_fspath has been written for three times, and called always.
> What do you think refact it ? Do it with
> fspath = erofs_fspath(path);
> if need decorated:
>    fspath = decorated

Yeah, it sounds reasonable. Let's refine this as a improvement of
an individual patch (if needed) since I have to rebuild AOSP again
(that is a bit complex for me now to open my PC...)

Thanks,
Gao Xiang

>