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

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

[Yue Hu]

From: Yue Hu <huyue2@yulong.com>

We observe that creating image failed to find [%s] in canned fs_config
using --fs-config-file option under Android 10.

Notice that canned fs_config has a prefix to sub directory if mount_point
presents. However, erofs_fspath() does not contain the prefix.

Moreover, we should not add the mount point to fspath on root inode for
fs_config() branch.

Signed-off-by: Yue Hu <huyue2@yulong.com>
---
 include/erofs/config.h |  4 ++++
 lib/inode.c            | 29 +++++++++++++++++++----------
 mkfs/main.c            | 14 ++++++++++----
 3 files changed, 33 insertions(+), 14 deletions(-)

diff --git a/include/erofs/config.h b/include/erofs/config.h
index 02ddf59..1277eda 100644
--- a/include/erofs/config.h
+++ b/include/erofs/config.h
@@ -58,6 +58,10 @@ struct erofs_configure {
 	char *mount_point;
 	char *target_out_path;
 	char *fs_config_file;
+	void (*fs_config_func)(const char *path, int dir,
+			       const char *target_out_path,
+			       unsigned *uid, unsigned *gid,
+			       unsigned *mode, uint64_t *capabilities);
 #endif
 };
 
diff --git a/lib/inode.c b/lib/inode.c
index eb2e0f2..d0805cd 100644
--- a/lib/inode.c
+++ b/lib/inode.c
@@ -684,20 +684,29 @@ int erofs_droid_inode_fsconfig(struct erofs_inode *inode,
 	char *fspath;
 
 	inode->capabilities = 0;
-	if (cfg.fs_config_file)
-		canned_fs_config(erofs_fspath(path),
-				 S_ISDIR(st->st_mode),
-				 cfg.target_out_path,
-				 &uid, &gid, &mode, &inode->capabilities);
-	else if (cfg.mount_point) {
+
+	if (erofs_fspath(path)[0] == '\0')
+		goto e_fspath;
+
+	if (cfg.mount_point) {
 		if (asprintf(&fspath, "%s/%s", cfg.mount_point,
 			     erofs_fspath(path)) <= 0)
 			return -ENOMEM;
-
-		fs_config(fspath, S_ISDIR(st->st_mode),
-			  cfg.target_out_path,
-			  &uid, &gid, &mode, &inode->capabilities);
+		if (cfg.fs_config_func)
+			cfg.fs_config_func(fspath,
+					   S_ISDIR(st->st_mode),
+					   cfg.target_out_path,
+					   &uid, &gid, &mode,
+					   &inode->capabilities);
 		free(fspath);
+	} else {
+e_fspath:
+		if (cfg.fs_config_func)
+			cfg.fs_config_func(erofs_fspath(path),
+					   S_ISDIR(st->st_mode),
+					   cfg.target_out_path,
+					   &uid, &gid, &mode,
+					   &inode->capabilities);
 	}
 	st->st_uid = uid;
 	st->st_gid = gid;
diff --git a/mkfs/main.c b/mkfs/main.c
index c63b274..684767c 100644
--- a/mkfs/main.c
+++ b/mkfs/main.c
@@ -474,10 +474,16 @@ int main(int argc, char **argv)
 	}
 
 #ifdef WITH_ANDROID
-	if (cfg.fs_config_file &&
-	    load_canned_fs_config(cfg.fs_config_file) < 0) {
-		erofs_err("failed to load fs config %s", cfg.fs_config_file);
-		return 1;
+	cfg.fs_config_func = NULL;
+	if (cfg.fs_config_file) {
+		if (load_canned_fs_config(cfg.fs_config_file) < 0) {
+			erofs_err("failed to load fs config %s",
+					cfg.fs_config_file);
+			return 1;
+		}
+		cfg.fs_config_func = canned_fs_config;
+	} else if (cfg.mount_point) {
+		cfg.fs_config_func = fs_config;
 	}
 #endif
 
-- 
1.9.1

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

[Gao Xiang]

Hi Yue,

On Tue, Dec 22, 2020 at 10:04:30AM +0800, Yue Hu wrote:
> From: Yue Hu <huyue2@yulong.com>
> 
> We observe that creating image failed to find [%s] in canned fs_config
> using --fs-config-file option under Android 10.
> 
> Notice that canned fs_config has a prefix to sub directory if mount_point
> presents. However, erofs_fspath() does not contain the prefix.
> 

Thanks for your patch. Let me play with it a bit this weekend (I'm not
quite familiar with canned fs_config, it would be of great help to add
some hints/steps for me to confirm this issue.... since some other vendors
already use it without report (maybe they don't use canned fs_config.)

> Moreover, we should not add the mount point to fspath on root inode for
> fs_config() branch.

Is there some descriptive words or reference for this? To be honest,
I'm quite unsure about this kind of Android-specific things... :(

Thanks,
Gao Xiang

> 
> Signed-off-by: Yue Hu <huyue2@yulong.com>
> ---
>  include/erofs/config.h |  4 ++++
>  lib/inode.c            | 29 +++++++++++++++++++----------
>  mkfs/main.c            | 14 ++++++++++----
>  3 files changed, 33 insertions(+), 14 deletions(-)
> 
> diff --git a/include/erofs/config.h b/include/erofs/config.h
> index 02ddf59..1277eda 100644
> --- a/include/erofs/config.h
> +++ b/include/erofs/config.h
> @@ -58,6 +58,10 @@ struct erofs_configure {
>  	char *mount_point;
>  	char *target_out_path;
>  	char *fs_config_file;
> +	void (*fs_config_func)(const char *path, int dir,
> +			       const char *target_out_path,
> +			       unsigned *uid, unsigned *gid,
> +			       unsigned *mode, uint64_t *capabilities);
>  #endif
>  };
>  
> diff --git a/lib/inode.c b/lib/inode.c
> index eb2e0f2..d0805cd 100644
> --- a/lib/inode.c
> +++ b/lib/inode.c
> @@ -684,20 +684,29 @@ int erofs_droid_inode_fsconfig(struct erofs_inode *inode,
>  	char *fspath;
>  
>  	inode->capabilities = 0;
> -	if (cfg.fs_config_file)
> -		canned_fs_config(erofs_fspath(path),
> -				 S_ISDIR(st->st_mode),
> -				 cfg.target_out_path,
> -				 &uid, &gid, &mode, &inode->capabilities);
> -	else if (cfg.mount_point) {
> +
> +	if (erofs_fspath(path)[0] == '\0')
> +		goto e_fspath;
> +
> +	if (cfg.mount_point) {
>  		if (asprintf(&fspath, "%s/%s", cfg.mount_point,
>  			     erofs_fspath(path)) <= 0)
>  			return -ENOMEM;
> -
> -		fs_config(fspath, S_ISDIR(st->st_mode),
> -			  cfg.target_out_path,
> -			  &uid, &gid, &mode, &inode->capabilities);
> +		if (cfg.fs_config_func)
> +			cfg.fs_config_func(fspath,
> +					   S_ISDIR(st->st_mode),
> +					   cfg.target_out_path,
> +					   &uid, &gid, &mode,
> +					   &inode->capabilities);
>  		free(fspath);
> +	} else {
> +e_fspath:
> +		if (cfg.fs_config_func)
> +			cfg.fs_config_func(erofs_fspath(path),
> +					   S_ISDIR(st->st_mode),
> +					   cfg.target_out_path,
> +					   &uid, &gid, &mode,
> +					   &inode->capabilities);
>  	}
>  	st->st_uid = uid;
>  	st->st_gid = gid;
> diff --git a/mkfs/main.c b/mkfs/main.c
> index c63b274..684767c 100644
> --- a/mkfs/main.c
> +++ b/mkfs/main.c
> @@ -474,10 +474,16 @@ int main(int argc, char **argv)
>  	}
>  
>  #ifdef WITH_ANDROID
> -	if (cfg.fs_config_file &&
> -	    load_canned_fs_config(cfg.fs_config_file) < 0) {
> -		erofs_err("failed to load fs config %s", cfg.fs_config_file);
> -		return 1;
> +	cfg.fs_config_func = NULL;
> +	if (cfg.fs_config_file) {
> +		if (load_canned_fs_config(cfg.fs_config_file) < 0) {
> +			erofs_err("failed to load fs config %s",
> +					cfg.fs_config_file);
> +			return 1;
> +		}
> +		cfg.fs_config_func = canned_fs_config;
> +	} else if (cfg.mount_point) {
> +		cfg.fs_config_func = fs_config;
>  	}
>  #endif
>  
> -- 
> 1.9.1
> 

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

[Yue Hu]

On Tue, 22 Dec 2020 11:44:55 +0800
Gao Xiang <hsiangkao@redhat.com> wrote:

> Hi Yue,
> 
> On Tue, Dec 22, 2020 at 10:04:30AM +0800, Yue Hu wrote:
> > From: Yue Hu <huyue2@yulong.com>
> > 
> > We observe that creating image failed to find [%s] in canned fs_config
> > using --fs-config-file option under Android 10.
> > 
> > Notice that canned fs_config has a prefix to sub directory if mount_point
> > presents. However, erofs_fspath() does not contain the prefix.
> >   
> 
> Thanks for your patch. Let me play with it a bit this weekend (I'm not
> quite familiar with canned fs_config, it would be of great help to add
> some hints/steps for me to confirm this issue.... since some other vendors
> already use it without report (maybe they don't use canned fs_config.)

Hi Xiang,

It's been observed under QC/QSSI platform with dynamic partition.

such as product.img/product_filesystem_config.txt:

```txt
 0 0 755 selabel=u:object_r:rootfs:s0 capabilities=0x0
product/app 0 0 755 selabel=u:object_r:system_file:s0 capabilities=0x0
product/app/CalculatorGoogle 0 0 755 selabel=u:object_r:system_file:s0 capabilities=0x0
```

product_filesystem_config.txt should be from below build:

$(call fs_config,$(zip_root)/PRODUCT,product/) > $(zip_root)/META/product_filesystem_config.txt

Do not observe this issue in squashfs, so i check related code of squashfs, squashfs have
considered the mount point, also ext4 does. So, erofs should be same as one long used before.

After this patch, build & boot are working fine by test.

Here's a change from mksqushfs: Allow passing fs_config file for generating mksquashfs

> 
> > Moreover, we should not add the mount point to fspath on root inode for
> > fs_config() branch.  
> 
> Is there some descriptive words or reference for this? To be honest,
> I'm quite unsure about this kind of Android-specific things... :(

Refer to change: mksquashfs: Run android_fs_config() on the root inode

I think erofs of AOSP has this issue also. Am i right?

Thx.

> 
> Thanks,
> Gao Xiang
> 
> > 
> > Signed-off-by: Yue Hu <huyue2@yulong.com>
> > ---
> >  include/erofs/config.h |  4 ++++
> >  lib/inode.c            | 29 +++++++++++++++++++----------
> >  mkfs/main.c            | 14 ++++++++++----
> >  3 files changed, 33 insertions(+), 14 deletions(-)
> > 
> > diff --git a/include/erofs/config.h b/include/erofs/config.h
> > index 02ddf59..1277eda 100644
> > --- a/include/erofs/config.h
> > +++ b/include/erofs/config.h
> > @@ -58,6 +58,10 @@ struct erofs_configure {
> >  	char *mount_point;
> >  	char *target_out_path;
> >  	char *fs_config_file;
> > +	void (*fs_config_func)(const char *path, int dir,
> > +			       const char *target_out_path,
> > +			       unsigned *uid, unsigned *gid,
> > +			       unsigned *mode, uint64_t *capabilities);
> >  #endif
> >  };
> >  
> > diff --git a/lib/inode.c b/lib/inode.c
> > index eb2e0f2..d0805cd 100644
> > --- a/lib/inode.c
> > +++ b/lib/inode.c
> > @@ -684,20 +684,29 @@ int erofs_droid_inode_fsconfig(struct erofs_inode *inode,
> >  	char *fspath;
> >  
> >  	inode->capabilities = 0;
> > -	if (cfg.fs_config_file)
> > -		canned_fs_config(erofs_fspath(path),
> > -				 S_ISDIR(st->st_mode),
> > -				 cfg.target_out_path,
> > -				 &uid, &gid, &mode, &inode->capabilities);
> > -	else if (cfg.mount_point) {
> > +
> > +	if (erofs_fspath(path)[0] == '\0')
> > +		goto e_fspath;
> > +
> > +	if (cfg.mount_point) {
> >  		if (asprintf(&fspath, "%s/%s", cfg.mount_point,
> >  			     erofs_fspath(path)) <= 0)
> >  			return -ENOMEM;
> > -
> > -		fs_config(fspath, S_ISDIR(st->st_mode),
> > -			  cfg.target_out_path,
> > -			  &uid, &gid, &mode, &inode->capabilities);
> > +		if (cfg.fs_config_func)
> > +			cfg.fs_config_func(fspath,
> > +					   S_ISDIR(st->st_mode),
> > +					   cfg.target_out_path,
> > +					   &uid, &gid, &mode,
> > +					   &inode->capabilities);
> >  		free(fspath);
> > +	} else {
> > +e_fspath:
> > +		if (cfg.fs_config_func)
> > +			cfg.fs_config_func(erofs_fspath(path),
> > +					   S_ISDIR(st->st_mode),
> > +					   cfg.target_out_path,
> > +					   &uid, &gid, &mode,
> > +					   &inode->capabilities);
> >  	}
> >  	st->st_uid = uid;
> >  	st->st_gid = gid;
> > diff --git a/mkfs/main.c b/mkfs/main.c
> > index c63b274..684767c 100644
> > --- a/mkfs/main.c
> > +++ b/mkfs/main.c
> > @@ -474,10 +474,16 @@ int main(int argc, char **argv)
> >  	}
> >  
> >  #ifdef WITH_ANDROID
> > -	if (cfg.fs_config_file &&
> > -	    load_canned_fs_config(cfg.fs_config_file) < 0) {
> > -		erofs_err("failed to load fs config %s", cfg.fs_config_file);
> > -		return 1;
> > +	cfg.fs_config_func = NULL;
> > +	if (cfg.fs_config_file) {
> > +		if (load_canned_fs_config(cfg.fs_config_file) < 0) {
> > +			erofs_err("failed to load fs config %s",
> > +					cfg.fs_config_file);
> > +			return 1;
> > +		}
> > +		cfg.fs_config_func = canned_fs_config;
> > +	} else if (cfg.mount_point) {
> > +		cfg.fs_config_func = fs_config;
> >  	}
> >  #endif
> >  
> > -- 
> > 1.9.1
> >   
> 

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

[Gao Xiang]

On Tue, Dec 22, 2020 at 12:47:33PM +0800, Yue Hu wrote:
> On Tue, 22 Dec 2020 11:44:55 +0800
> Gao Xiang <hsiangkao@redhat.com> wrote:
> 
> > Hi Yue,
> > 
> > On Tue, Dec 22, 2020 at 10:04:30AM +0800, Yue Hu wrote:
> > > From: Yue Hu <huyue2@yulong.com>
> > > 
> > > We observe that creating image failed to find [%s] in canned fs_config
> > > using --fs-config-file option under Android 10.
> > > 
> > > Notice that canned fs_config has a prefix to sub directory if mount_point
> > > presents. However, erofs_fspath() does not contain the prefix.
> > >   
> > 
> > Thanks for your patch. Let me play with it a bit this weekend (I'm not
> > quite familiar with canned fs_config, it would be of great help to add
> > some hints/steps for me to confirm this issue.... since some other vendors
> > already use it without report (maybe they don't use canned fs_config.)
> 
> Hi Xiang,
> 
> It's been observed under QC/QSSI platform with dynamic partition.
> 
> such as product.img/product_filesystem_config.txt:
> 
> ```txt
>  0 0 755 selabel=u:object_r:rootfs:s0 capabilities=0x0
> product/app 0 0 755 selabel=u:object_r:system_file:s0 capabilities=0x0
> product/app/CalculatorGoogle 0 0 755 selabel=u:object_r:system_file:s0 capabilities=0x0
> ```
> 
> product_filesystem_config.txt should be from below build:
> 
> $(call fs_config,$(zip_root)/PRODUCT,product/) > $(zip_root)/META/product_filesystem_config.txt
> 
> Do not observe this issue in squashfs, so i check related code of squashfs, squashfs have
> considered the mount point, also ext4 does. So, erofs should be same as one long used before.
> 
> After this patch, build & boot are working fine by test.
> 
> Here's a change from mksqushfs: Allow passing fs_config file for generating mksquashfs

Ok, I will try to find some clue to verify later.

> 
> > 
> > > Moreover, we should not add the mount point to fspath on root inode for
> > > fs_config() branch.  
> > 
> > Is there some descriptive words or reference for this? To be honest,
> > I'm quite unsure about this kind of Android-specific things... :(
> 
> Refer to change: mksquashfs: Run android_fs_config() on the root inode
> 
> I think erofs of AOSP has this issue also. Am i right?

Not quite sure if it effects non-canned fs_config after
reading the commit message...
https://android.googlesource.com/platform/external/squashfs-tools/+/85a6bc1e52bb911f195c5dc0890717913938c2d1%5E%21/#F0

And no permission to access Bug 72745016, so...
maybe we need to limit this to non-canned fs_config only?
(at least confirming the original case would be better)

BTW, Also, from its testcase command line in the commit message:
"mksquashfs path system.raw.img -fs-config-file fs_config -android-fs-config"

I'm not sure if "--mount-point" is passed in so I think for
such case no need to use such "goto" as well? 

Thanks,
Gao Xiang

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

[Gao Xiang]

Hi Yue,

On Tue, Dec 22, 2020 at 02:31:12PM +0800, Gao Xiang wrote:

...

> 
> Ok, I will try to find some clue to verify later.
> 
> > 
> > > 
> > > > Moreover, we should not add the mount point to fspath on root inode for
> > > > fs_config() branch.  
> > > 
> > > Is there some descriptive words or reference for this? To be honest,
> > > I'm quite unsure about this kind of Android-specific things... :(
> > 
> > Refer to change: mksquashfs: Run android_fs_config() on the root inode
> > 
> > I think erofs of AOSP has this issue also. Am i right?
> 
> Not quite sure if it effects non-canned fs_config after
> reading the commit message...
> https://android.googlesource.com/platform/external/squashfs-tools/+/85a6bc1e52bb911f195c5dc0890717913938c2d1%5E%21/#F0
> 
> And no permission to access Bug 72745016, so...
> maybe we need to limit this to non-canned fs_config only?
> (at least confirming the original case would be better)
> 
> BTW, Also, from its testcase command line in the commit message:
> "mksquashfs path system.raw.img -fs-config-file fs_config -android-fs-config"
> 
> I'm not sure if "--mount-point" is passed in so I think for
> such case no need to use such "goto" as well? 
> 
> Thanks,
> Gao Xiang

Could you verify the following patch if possible? (without compilation,
I don't have test environment now since AOSP code is on my PC)

From: Yue Hu <huyue2@yulong.com>
Date: Tue, 22 Dec 2020 14:52:22 +0800
Subject: [PATCH] AOSP: erofs-utils: fix sub-directory prefix for canned
 fs_config

"failed to find [%s] in canned fs_config" is observed by using
"--fs-config-file" option under Android 10.

Notice that canned fs_config has a prefix to sub-directory if
"--mount-point" presents. However, such prefix cannot be added by
just using erofs_fspath().

Fixes: 8a9e8046f170 ("AOSP: erofs-utils: add fs_config support")
Signed-off-by: Yue Hu <huyue2@yulong.com>
Signed-off-by: Gao Xiang <hsiangkao@aol.com>
---
 lib/inode.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/lib/inode.c b/lib/inode.c
index 3d634fc..9469074 100644
--- a/lib/inode.c
+++ b/lib/inode.c
@@ -701,21 +701,25 @@ int erofs_droid_inode_fsconfig(struct erofs_inode *inode,
 	char *fspath;
 
 	inode->capabilities = 0;
+	if (!cfg.mount_point)
+		fspath = erofs_fspath(path);
+	else if (asprintf(&fspath, "%s/%s", cfg.mount_point,
+			  erofs_fspath(path)) <= 0)
+		return -ENOMEM;
+
+
 	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);
+
+	if (cfg.mount_point)
 		free(fspath);
-	}
 	st->st_uid = uid;
 	st->st_gid = gid;
 	st->st_mode = mode | stat_file_type_mask;
-- 
2.27.0

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

[Yue Hu]

On Tue, 22 Dec 2020 15:04:58 +0800
Gao Xiang <hsiangkao@redhat.com> wrote:

> Hi Yue,
> 
> On Tue, Dec 22, 2020 at 02:31:12PM +0800, Gao Xiang wrote:
> 
> ...
> 
> > 
> > Ok, I will try to find some clue to verify later.
> >   
> > >   
> > > >   
> > > > > Moreover, we should not add the mount point to fspath on root inode for
> > > > > fs_config() branch.    
> > > > 
> > > > Is there some descriptive words or reference for this? To be honest,
> > > > I'm quite unsure about this kind of Android-specific things... :(  
> > > 
> > > Refer to change: mksquashfs: Run android_fs_config() on the root inode
> > > 
> > > I think erofs of AOSP has this issue also. Am i right?  
> > 
> > Not quite sure if it effects non-canned fs_config after
> > reading the commit message...
> > https://android.googlesource.com/platform/external/squashfs-tools/+/85a6bc1e52bb911f195c5dc0890717913938c2d1%5E%21/#F0
> > 
> > And no permission to access Bug 72745016, so...
> > maybe we need to limit this to non-canned fs_config only?
> > (at least confirming the original case would be better)
> > 
> > BTW, Also, from its testcase command line in the commit message:
> > "mksquashfs path system.raw.img -fs-config-file fs_config -android-fs-config"
> > 
> > I'm not sure if "--mount-point" is passed in so I think for
> > such case no need to use such "goto" as well? 
> > 
> > Thanks,
> > Gao Xiang  
> 
> Could you verify the following patch if possible? (without compilation,
> I don't have test environment now since AOSP code is on my PC)


Ok, i will verify today.

Thx.

> 
> From: Yue Hu <huyue2@yulong.com>
> Date: Tue, 22 Dec 2020 14:52:22 +0800
> Subject: [PATCH] AOSP: erofs-utils: fix sub-directory prefix for canned
>  fs_config
> 
> "failed to find [%s] in canned fs_config" is observed by using
> "--fs-config-file" option under Android 10.
> 
> Notice that canned fs_config has a prefix to sub-directory if
> "--mount-point" presents. However, such prefix cannot be added by
> just using erofs_fspath().
> 
> Fixes: 8a9e8046f170 ("AOSP: erofs-utils: add fs_config support")
> Signed-off-by: Yue Hu <huyue2@yulong.com>
> Signed-off-by: Gao Xiang <hsiangkao@aol.com>
> ---
>  lib/inode.c | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/lib/inode.c b/lib/inode.c
> index 3d634fc..9469074 100644
> --- a/lib/inode.c
> +++ b/lib/inode.c
> @@ -701,21 +701,25 @@ int erofs_droid_inode_fsconfig(struct erofs_inode *inode,
>  	char *fspath;
>  
>  	inode->capabilities = 0;
> +	if (!cfg.mount_point)
> +		fspath = erofs_fspath(path);
> +	else if (asprintf(&fspath, "%s/%s", cfg.mount_point,
> +			  erofs_fspath(path)) <= 0)
> +		return -ENOMEM;
> +
> +
>  	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);
> +
> +	if (cfg.mount_point)
>  		free(fspath);
> -	}
>  	st->st_uid = uid;
>  	st->st_gid = gid;
>  	st->st_mode = mode | stat_file_type_mask;

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

[Yue Hu]

On Tue, 22 Dec 2020 15:04:58 +0800
Gao Xiang <hsiangkao@redhat.com> wrote:

> Hi Yue,
> 
> On Tue, Dec 22, 2020 at 02:31:12PM +0800, Gao Xiang wrote:
> 
> ...
> 
> > 
> > Ok, I will try to find some clue to verify later.
> >   
> > >   
> > > >   
> > > > > Moreover, we should not add the mount point to fspath on root inode for
> > > > > fs_config() branch.    
> > > > 
> > > > Is there some descriptive words or reference for this? To be honest,
> > > > I'm quite unsure about this kind of Android-specific things... :(  
> > > 
> > > Refer to change: mksquashfs: Run android_fs_config() on the root inode
> > > 
> > > I think erofs of AOSP has this issue also. Am i right?  
> > 
> > Not quite sure if it effects non-canned fs_config after
> > reading the commit message...
> > https://android.googlesource.com/platform/external/squashfs-tools/+/85a6bc1e52bb911f195c5dc0890717913938c2d1%5E%21/#F0
> > 
> > And no permission to access Bug 72745016, so...
> > maybe we need to limit this to non-canned fs_config only?
> > (at least confirming the original case would be better)
> > 
> > BTW, Also, from its testcase command line in the commit message:
> > "mksquashfs path system.raw.img -fs-config-file fs_config -android-fs-config"
> > 
> > I'm not sure if "--mount-point" is passed in so I think for
> > such case no need to use such "goto" as well? 
> > 
> > Thanks,
> > Gao Xiang  
> 
> Could you verify the following patch if possible? (without compilation,
> I don't have test environment now since AOSP code is on my PC)
> 
> From: Yue Hu <huyue2@yulong.com>
> Date: Tue, 22 Dec 2020 14:52:22 +0800
> Subject: [PATCH] AOSP: erofs-utils: fix sub-directory prefix for canned
>  fs_config
> 
> "failed to find [%s] in canned fs_config" is observed by using
> "--fs-config-file" option under Android 10.
> 
> Notice that canned fs_config has a prefix to sub-directory if
> "--mount-point" presents. However, such prefix cannot be added by
> just using erofs_fspath().
> 
> Fixes: 8a9e8046f170 ("AOSP: erofs-utils: add fs_config support")
> Signed-off-by: Yue Hu <huyue2@yulong.com>
> Signed-off-by: Gao Xiang <hsiangkao@aol.com>
> ---
>  lib/inode.c | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/lib/inode.c b/lib/inode.c
> index 3d634fc..9469074 100644
> --- a/lib/inode.c
> +++ b/lib/inode.c
> @@ -701,21 +701,25 @@ int erofs_droid_inode_fsconfig(struct erofs_inode *inode,
>  	char *fspath;
>  
>  	inode->capabilities = 0;
> +	if (!cfg.mount_point)
> +		fspath = erofs_fspath(path);

lib/inode.c:688:10: error: assigning to 'char *' from 'const char *' discards...
                fspath = erofs_fspath(path);
                       ^ ~~~~~~~~~~~~~~~~~~

-           fspath = erofs_fspath(path);
+         fspath = (char *)erofs_fspath(path);


> +	else if (asprintf(&fspath, "%s/%s", cfg.mount_point,
> +			  erofs_fspath(path)) <= 0)

The argument of path will be root directory. And canned fs_config for root directory as
below:

0 0 755 selabel=u:object_r:rootfs:s0 capabilities=0x0

So, cannot add mount point to root directory for canned fs_config. And what about non-canned
fs_config?

Thx.


> +		return -ENOMEM;
> +
> +
>  	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);
> +
> +	if (cfg.mount_point)
>  		free(fspath);
> -	}
>  	st->st_uid = uid;
>  	st->st_gid = gid;
>  	st->st_mode = mode | stat_file_type_mask;

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

[Gao Xiang]

On Tue, Dec 22, 2020 at 04:10:58PM +0800, Yue Hu wrote:
> On Tue, 22 Dec 2020 15:04:58 +0800
> Gao Xiang <hsiangkao@redhat.com> wrote:
> 
> > Hi Yue,
> > 
> > On Tue, Dec 22, 2020 at 02:31:12PM +0800, Gao Xiang wrote:
> > 
> > ...
> > 
> > > 
> > > Ok, I will try to find some clue to verify later.
> > >   
> > > >   
> > > > >   
> > > > > > Moreover, we should not add the mount point to fspath on root inode for
> > > > > > fs_config() branch.    
> > > > > 
> > > > > Is there some descriptive words or reference for this? To be honest,
> > > > > I'm quite unsure about this kind of Android-specific things... :(  
> > > > 
> > > > Refer to change: mksquashfs: Run android_fs_config() on the root inode
> > > > 
> > > > I think erofs of AOSP has this issue also. Am i right?  
> > > 
> > > Not quite sure if it effects non-canned fs_config after
> > > reading the commit message...
> > > https://android.googlesource.com/platform/external/squashfs-tools/+/85a6bc1e52bb911f195c5dc0890717913938c2d1%5E%21/#F0
> > > 
> > > And no permission to access Bug 72745016, so...
> > > maybe we need to limit this to non-canned fs_config only?
> > > (at least confirming the original case would be better)
> > > 
> > > BTW, Also, from its testcase command line in the commit message:
> > > "mksquashfs path system.raw.img -fs-config-file fs_config -android-fs-config"
> > > 
> > > I'm not sure if "--mount-point" is passed in so I think for
> > > such case no need to use such "goto" as well? 
> > > 
> > > Thanks,
> > > Gao Xiang  
> > 
> > Could you verify the following patch if possible? (without compilation,
> > I don't have test environment now since AOSP code is on my PC)
> > 
> > From: Yue Hu <huyue2@yulong.com>
> > Date: Tue, 22 Dec 2020 14:52:22 +0800
> > Subject: [PATCH] AOSP: erofs-utils: fix sub-directory prefix for canned
> >  fs_config
> > 
> > "failed to find [%s] in canned fs_config" is observed by using
> > "--fs-config-file" option under Android 10.
> > 
> > Notice that canned fs_config has a prefix to sub-directory if
> > "--mount-point" presents. However, such prefix cannot be added by
> > just using erofs_fspath().
> > 
> > Fixes: 8a9e8046f170 ("AOSP: erofs-utils: add fs_config support")
> > Signed-off-by: Yue Hu <huyue2@yulong.com>
> > Signed-off-by: Gao Xiang <hsiangkao@aol.com>
> > ---
> >  lib/inode.c | 18 +++++++++++-------
> >  1 file changed, 11 insertions(+), 7 deletions(-)
> > 
> > diff --git a/lib/inode.c b/lib/inode.c
> > index 3d634fc..9469074 100644
> > --- a/lib/inode.c
> > +++ b/lib/inode.c
> > @@ -701,21 +701,25 @@ int erofs_droid_inode_fsconfig(struct erofs_inode *inode,
> >  	char *fspath;
> >  
> >  	inode->capabilities = 0;
> > +	if (!cfg.mount_point)
> > +		fspath = erofs_fspath(path);
> 
> lib/inode.c:688:10: error: assigning to 'char *' from 'const char *' discards...
>                 fspath = erofs_fspath(path);
>                        ^ ~~~~~~~~~~~~~~~~~~
> 
> -           fspath = erofs_fspath(path);
> +         fspath = (char *)erofs_fspath(path);

oops, I think it can be modified as a temporary workaround, will submit a formal
patch after verification.

> 
> 
> > +	else if (asprintf(&fspath, "%s/%s", cfg.mount_point,
> > +			  erofs_fspath(path)) <= 0)
> 
> The argument of path will be root directory. And canned fs_config for root directory as
> below:
> 
> 0 0 755 selabel=u:object_r:rootfs:s0 capabilities=0x0
> 
> So, cannot add mount point to root directory for canned fs_config. And what about non-canned
> fs_config?

Not quite sure what you mean. For non-canned fs_config, we didn't observed any strange
before (I ported to cuttlefish and hikey960 with boot success, also as I mentioned before
some other vendors already use it.)

I think the following commit is only useful for squashfs since its (non)root inode
workflows are different, so need to add in two difference place. But mkfs.erofs is not.
https://android.googlesource.com/platform/external/squashfs-tools/+/85a6bc1e52bb911f195c5dc0890717913938c2d1%5E%21/#F0

For root inode is erofs, I think erofs_fspath(path) would return "", so that case
is included as well.... Am I missing something?

Thanks,
Gao Xiang

> 
> Thx.
> 
> 
> > +		return -ENOMEM;
> > +
> > +
> >  	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);
> > +
> > +	if (cfg.mount_point)
> >  		free(fspath);
> > -	}
> >  	st->st_uid = uid;
> >  	st->st_gid = gid;
> >  	st->st_mode = mode | stat_file_type_mask;
> 

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

[Gao Xiang]

On Tue, Dec 22, 2020 at 05:13:40PM +0800, Gao Xiang wrote:

...

> > > +		fspath = erofs_fspath(path);
> > 
> > lib/inode.c:688:10: error: assigning to 'char *' from 'const char *' discards...
> >                 fspath = erofs_fspath(path);
> >                        ^ ~~~~~~~~~~~~~~~~~~
> > 
> > -           fspath = erofs_fspath(path);
> > +         fspath = (char *)erofs_fspath(path);
> 
> oops, I think it can be modified as a temporary workaround, will submit a formal
> patch after verification.
> 
> > 
> > 
> > > +	else if (asprintf(&fspath, "%s/%s", cfg.mount_point,
> > > +			  erofs_fspath(path)) <= 0)
> > 
> > The argument of path will be root directory. And canned fs_config for root directory as
> > below:
> > 
> > 0 0 755 selabel=u:object_r:rootfs:s0 capabilities=0x0
> > 
> > So, cannot add mount point to root directory for canned fs_config. And what about non-canned
> > fs_config?
> 
> Not quite sure what you mean. For non-canned fs_config, we didn't observed any strange
> before (I ported to cuttlefish and hikey960 with boot success, also as I mentioned before
> some other vendors already use it.)
> 
> I think the following commit is only useful for squashfs since its (non)root inode
> workflows are different, so need to add in two difference place. But mkfs.erofs is not.
> https://android.googlesource.com/platform/external/squashfs-tools/+/85a6bc1e52bb911f195c5dc0890717913938c2d1%5E%21/#F0
> 
> For root inode is erofs, I think erofs_fspath(path) would return "", so that case
> is included as well.... Am I missing something?

Also I don't find any special handling logic in make_ext4fs for root inode
https://android.googlesource.com/platform/system/extras/+/refs/heads/oreo-release/ext4_utils/make_ext4fs.c#229

Actually I think the squashfs commit above might be wrong if "--mount-point" is
specified if my understanding is correct. Anyway, could you help verify it in advance?

Thanks,
Gao Xiang

> 
> Thanks,
> Gao Xiang
> 
> > 
> > Thx.

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

[Yue Hu]

On Tue, 22 Dec 2020 17:13:40 +0800
Gao Xiang <hsiangkao@redhat.com> wrote:

> On Tue, Dec 22, 2020 at 04:10:58PM +0800, Yue Hu wrote:
> > On Tue, 22 Dec 2020 15:04:58 +0800
> > Gao Xiang <hsiangkao@redhat.com> wrote:
> >   
> > > Hi Yue,
> > > 
> > > On Tue, Dec 22, 2020 at 02:31:12PM +0800, Gao Xiang wrote:
> > > 
> > > ...
> > >   
> > > > 
> > > > Ok, I will try to find some clue to verify later.
> > > >     
> > > > >     
> > > > > >     
> > > > > > > Moreover, we should not add the mount point to fspath on root inode for
> > > > > > > fs_config() branch.      
> > > > > > 
> > > > > > Is there some descriptive words or reference for this? To be honest,
> > > > > > I'm quite unsure about this kind of Android-specific things... :(    
> > > > > 
> > > > > Refer to change: mksquashfs: Run android_fs_config() on the root inode
> > > > > 
> > > > > I think erofs of AOSP has this issue also. Am i right?    
> > > > 
> > > > Not quite sure if it effects non-canned fs_config after
> > > > reading the commit message...
> > > > https://android.googlesource.com/platform/external/squashfs-tools/+/85a6bc1e52bb911f195c5dc0890717913938c2d1%5E%21/#F0
> > > > 
> > > > And no permission to access Bug 72745016, so...
> > > > maybe we need to limit this to non-canned fs_config only?
> > > > (at least confirming the original case would be better)
> > > > 
> > > > BTW, Also, from its testcase command line in the commit message:
> > > > "mksquashfs path system.raw.img -fs-config-file fs_config -android-fs-config"
> > > > 
> > > > I'm not sure if "--mount-point" is passed in so I think for
> > > > such case no need to use such "goto" as well? 
> > > > 
> > > > Thanks,
> > > > Gao Xiang    
> > > 
> > > Could you verify the following patch if possible? (without compilation,
> > > I don't have test environment now since AOSP code is on my PC)
> > > 
> > > From: Yue Hu <huyue2@yulong.com>
> > > Date: Tue, 22 Dec 2020 14:52:22 +0800
> > > Subject: [PATCH] AOSP: erofs-utils: fix sub-directory prefix for canned
> > >  fs_config
> > > 
> > > "failed to find [%s] in canned fs_config" is observed by using
> > > "--fs-config-file" option under Android 10.
> > > 
> > > Notice that canned fs_config has a prefix to sub-directory if
> > > "--mount-point" presents. However, such prefix cannot be added by
> > > just using erofs_fspath().
> > > 
> > > Fixes: 8a9e8046f170 ("AOSP: erofs-utils: add fs_config support")
> > > Signed-off-by: Yue Hu <huyue2@yulong.com>
> > > Signed-off-by: Gao Xiang <hsiangkao@aol.com>
> > > ---
> > >  lib/inode.c | 18 +++++++++++-------
> > >  1 file changed, 11 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/lib/inode.c b/lib/inode.c
> > > index 3d634fc..9469074 100644
> > > --- a/lib/inode.c
> > > +++ b/lib/inode.c
> > > @@ -701,21 +701,25 @@ int erofs_droid_inode_fsconfig(struct erofs_inode *inode,
> > >  	char *fspath;
> > >  
> > >  	inode->capabilities = 0;
> > > +	if (!cfg.mount_point)
> > > +		fspath = erofs_fspath(path);  
> > 
> > lib/inode.c:688:10: error: assigning to 'char *' from 'const char *' discards...
> >                 fspath = erofs_fspath(path);
> >                        ^ ~~~~~~~~~~~~~~~~~~
> > 
> > -           fspath = erofs_fspath(path);
> > +         fspath = (char *)erofs_fspath(path);  
> 
> oops, I think it can be modified as a temporary workaround, will submit a formal
> patch after verification.
> 
> > 
> >   
> > > +	else if (asprintf(&fspath, "%s/%s", cfg.mount_point,
> > > +			  erofs_fspath(path)) <= 0)  
> > 
> > The argument of path will be root directory. And canned fs_config for root directory as
> > below:
> > 
> > 0 0 755 selabel=u:object_r:rootfs:s0 capabilities=0x0
> > 
> > So, cannot add mount point to root directory for canned fs_config. And what about non-canned
> > fs_config?  
> 
> Not quite sure what you mean. For non-canned fs_config, we didn't observed any strange
> before (I ported to cuttlefish and hikey960 with boot success, also as I mentioned before
> some other vendors already use it.)
> 
> I think the following commit is only useful for squashfs since its (non)root inode
> workflows are different, so need to add in two difference place. But mkfs.erofs is not.
> https://android.googlesource.com/platform/external/squashfs-tools/+/85a6bc1e52bb911f195c5dc0890717913938c2d1%5E%21/#F0
> 
> For root inode is erofs, I think erofs_fspath(path) would return "", so that case
> is included as well.... Am I missing something?

Yes, erofs_fspath(path) returns "" for root inode. However, the above patch add the mount
point to fspath when specify it, so the real path is "vendor/" which does not exist in canned
fs_config file. build will report below error:

failed to find [/vendor/] in canned fs_config

> 
> Thanks,
> Gao Xiang
> 
> > 
> > Thx.
> > 
> >   
> > > +		return -ENOMEM;
> > > +
> > > +
> > >  	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);
> > > +
> > > +	if (cfg.mount_point)
> > >  		free(fspath);
> > > -	}
> > >  	st->st_uid = uid;
> > >  	st->st_gid = gid;
> > >  	st->st_mode = mode | stat_file_type_mask;  
> >   
> 

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

[Gao Xiang]

On Tue, Dec 22, 2020 at 05:30:14PM +0800, Yue Hu wrote:

...

> > > 
> > >   
> > > > +	else if (asprintf(&fspath, "%s/%s", cfg.mount_point,
> > > > +			  erofs_fspath(path)) <= 0)  
> > > 
> > > The argument of path will be root directory. And canned fs_config for root directory as
> > > below:
> > > 
> > > 0 0 755 selabel=u:object_r:rootfs:s0 capabilities=0x0
> > > 
> > > So, cannot add mount point to root directory for canned fs_config. And what about non-canned
> > > fs_config?  
> > 
> > Not quite sure what you mean. For non-canned fs_config, we didn't observed any strange
> > before (I ported to cuttlefish and hikey960 with boot success, also as I mentioned before
> > some other vendors already use it.)
> > 
> > I think the following commit is only useful for squashfs since its (non)root inode
> > workflows are different, so need to add in two difference place. But mkfs.erofs is not.
> > https://android.googlesource.com/platform/external/squashfs-tools/+/85a6bc1e52bb911f195c5dc0890717913938c2d1%5E%21/#F0
> > 
> > For root inode is erofs, I think erofs_fspath(path) would return "", so that case
> > is included as well.... Am I missing something?
> 
> Yes, erofs_fspath(path) returns "" for root inode. However, the above patch add the mount
> point to fspath when specify it, so the real path is "vendor/" which does not exist in canned
> fs_config file. build will report below error:
> 
> failed to find [/vendor/] in canned fs_config

Hmmm... such design is quite strange for me....
Could you try the following diff?

diff --git a/lib/inode.c b/lib/inode.c
index 9469074..9af6179 100644
--- a/lib/inode.c
+++ b/lib/inode.c
@@ -698,11 +698,14 @@ 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;
+	bool alloced;
 	char *fspath;
 
 	inode->capabilities = 0;
-	if (!cfg.mount_point)
-		fspath = erofs_fspath(path);
+
+	alloced = (cfg.mount_point && erofs_fspath(path)[0] != '\0');
+	if (!alloced)
+		fspath = (char *)erofs_fspath(path);
 	else if (asprintf(&fspath, "%s/%s", cfg.mount_point,
 			  erofs_fspath(path)) <= 0)
 		return -ENOMEM;
@@ -718,7 +721,7 @@ int erofs_droid_inode_fsconfig(struct erofs_inode *inode,
 			  cfg.target_out_path,
 			  &uid, &gid, &mode, &inode->capabilities);
 
-	if (cfg.mount_point)
+	if (alloced)
 		free(fspath);
 	st->st_uid = uid;
 	st->st_gid = gid;

if it works, will redo a formal patch then....

Thanks,
Gao Xiang

> 
> > 
> > Thanks,
> > Gao Xiang
> > 
> > > 
> > > Thx.
> > > 
> > >   
> > > > +		return -ENOMEM;
> > > > +
> > > > +
> > > >  	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);
> > > > +
> > > > +	if (cfg.mount_point)
> > > >  		free(fspath);
> > > > -	}
> > > >  	st->st_uid = uid;
> > > >  	st->st_gid = gid;
> > > >  	st->st_mode = mode | stat_file_type_mask;  
> > >   
> > 
> 

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

[Yue Hu]

On Tue, 22 Dec 2020 17:39:52 +0800
Gao Xiang <hsiangkao@redhat.com> wrote:

> On Tue, Dec 22, 2020 at 05:30:14PM +0800, Yue Hu wrote:
> 
> ...
> 
> > > > 
> > > >     
> > > > > +	else if (asprintf(&fspath, "%s/%s", cfg.mount_point,
> > > > > +			  erofs_fspath(path)) <= 0)    
> > > > 
> > > > The argument of path will be root directory. And canned fs_config for root directory as
> > > > below:
> > > > 
> > > > 0 0 755 selabel=u:object_r:rootfs:s0 capabilities=0x0
> > > > 
> > > > So, cannot add mount point to root directory for canned fs_config. And what about non-canned
> > > > fs_config?    
> > > 
> > > Not quite sure what you mean. For non-canned fs_config, we didn't observed any strange
> > > before (I ported to cuttlefish and hikey960 with boot success, also as I mentioned before
> > > some other vendors already use it.)
> > > 
> > > I think the following commit is only useful for squashfs since its (non)root inode
> > > workflows are different, so need to add in two difference place. But mkfs.erofs is not.
> > > https://android.googlesource.com/platform/external/squashfs-tools/+/85a6bc1e52bb911f195c5dc0890717913938c2d1%5E%21/#F0
> > > 
> > > For root inode is erofs, I think erofs_fspath(path) would return "", so that case
> > > is included as well.... Am I missing something?  
> > 
> > Yes, erofs_fspath(path) returns "" for root inode. However, the above patch add the mount
> > point to fspath when specify it, so the real path is "vendor/" which does not exist in canned
> > fs_config file. build will report below error:
> > 
> > failed to find [/vendor/] in canned fs_config  
> 
> Hmmm... such design is quite strange for me....

:) i checked the squashfs before, seems root directory is handled in some position. Separated
with sub directory fs_config. so i add the goto code in the 1st patch.

> Could you try the following diff?

Let's me verify.

> 
> diff --git a/lib/inode.c b/lib/inode.c
> index 9469074..9af6179 100644
> --- a/lib/inode.c
> +++ b/lib/inode.c
> @@ -698,11 +698,14 @@ 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;
> +	bool alloced;
>  	char *fspath;
>  
>  	inode->capabilities = 0;
> -	if (!cfg.mount_point)
> -		fspath = erofs_fspath(path);
> +
> +	alloced = (cfg.mount_point && erofs_fspath(path)[0] != '\0');
> +	if (!alloced)
> +		fspath = (char *)erofs_fspath(path);
>  	else if (asprintf(&fspath, "%s/%s", cfg.mount_point,
>  			  erofs_fspath(path)) <= 0)
>  		return -ENOMEM;
> @@ -718,7 +721,7 @@ int erofs_droid_inode_fsconfig(struct erofs_inode *inode,
>  			  cfg.target_out_path,
>  			  &uid, &gid, &mode, &inode->capabilities);
>  
> -	if (cfg.mount_point)
> +	if (alloced)
>  		free(fspath);
>  	st->st_uid = uid;
>  	st->st_gid = gid;
> 
> if it works, will redo a formal patch then....
> 
> Thanks,
> Gao Xiang
> 
> >   
> > > 
> > > Thanks,
> > > Gao Xiang
> > >   
> > > > 
> > > > Thx.
> > > > 
> > > >     
> > > > > +		return -ENOMEM;
> > > > > +
> > > > > +
> > > > >  	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);
> > > > > +
> > > > > +	if (cfg.mount_point)
> > > > >  		free(fspath);
> > > > > -	}
> > > > >  	st->st_uid = uid;
> > > > >  	st->st_gid = gid;
> > > > >  	st->st_mode = mode | stat_file_type_mask;    
> > > >     
> > >   
> >   
> 

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

[Gao Xiang]

On Tue, Dec 22, 2020 at 05:46:23PM +0800, Yue Hu wrote:
> On Tue, 22 Dec 2020 17:39:52 +0800
> Gao Xiang <hsiangkao@redhat.com> wrote:
> 
> > On Tue, Dec 22, 2020 at 05:30:14PM +0800, Yue Hu wrote:
> > 
> > ...
> > 
> > > > > 
> > > > >     
> > > > > > +	else if (asprintf(&fspath, "%s/%s", cfg.mount_point,
> > > > > > +			  erofs_fspath(path)) <= 0)    
> > > > > 
> > > > > The argument of path will be root directory. And canned fs_config for root directory as
> > > > > below:
> > > > > 
> > > > > 0 0 755 selabel=u:object_r:rootfs:s0 capabilities=0x0
> > > > > 
> > > > > So, cannot add mount point to root directory for canned fs_config. And what about non-canned
> > > > > fs_config?    
> > > > 
> > > > Not quite sure what you mean. For non-canned fs_config, we didn't observed any strange
> > > > before (I ported to cuttlefish and hikey960 with boot success, also as I mentioned before
> > > > some other vendors already use it.)
> > > > 
> > > > I think the following commit is only useful for squashfs since its (non)root inode
> > > > workflows are different, so need to add in two difference place. But mkfs.erofs is not.
> > > > https://android.googlesource.com/platform/external/squashfs-tools/+/85a6bc1e52bb911f195c5dc0890717913938c2d1%5E%21/#F0
> > > > 
> > > > For root inode is erofs, I think erofs_fspath(path) would return "", so that case
> > > > is included as well.... Am I missing something?  
> > > 
> > > Yes, erofs_fspath(path) returns "" for root inode. However, the above patch add the mount
> > > point to fspath when specify it, so the real path is "vendor/" which does not exist in canned
> > > fs_config file. build will report below error:
> > > 
> > > failed to find [/vendor/] in canned fs_config  
> > 
> > Hmmm... such design is quite strange for me....
> 
> :) i checked the squashfs before, seems root directory is handled in some position. Separated
> with sub directory fs_config. so i add the goto code in the 1st patch.

What confuses me a lot is that we didn't get any strange without canned fs_config
if mount point prefix is added. I will look into other implementation about this
later (Another guess is that relates to Android 10 only?).

Yeah, the opensource fs_config implementation might be different from HUAWEI
internal fs_config version since such part was not originally written by me and
I didn't pay more attention about this part when I was in my previous company.
But anyway, this cleanup opensource version is already used for some vendors
as well and I don't get such report... And any formal description about this
would be helpful for me to understand how fs_config really works..

Thanks,
Gao Xiang

> 
> > Could you try the following diff?
> 
> Let's me verify.
> 

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

[Yue Hu]

On Tue, 22 Dec 2020 17:59:06 +0800
Gao Xiang <hsiangkao@redhat.com> wrote:

> On Tue, Dec 22, 2020 at 05:46:23PM +0800, Yue Hu wrote:
> > On Tue, 22 Dec 2020 17:39:52 +0800
> > Gao Xiang <hsiangkao@redhat.com> wrote:
> >   
> > > On Tue, Dec 22, 2020 at 05:30:14PM +0800, Yue Hu wrote:
> > > 
> > > ...
> > >   
> > > > > > 
> > > > > >       
> > > > > > > +	else if (asprintf(&fspath, "%s/%s", cfg.mount_point,
> > > > > > > +			  erofs_fspath(path)) <= 0)      
> > > > > > 
> > > > > > The argument of path will be root directory. And canned fs_config for root directory as
> > > > > > below:
> > > > > > 
> > > > > > 0 0 755 selabel=u:object_r:rootfs:s0 capabilities=0x0
> > > > > > 
> > > > > > So, cannot add mount point to root directory for canned fs_config. And what about non-canned
> > > > > > fs_config?      
> > > > > 
> > > > > Not quite sure what you mean. For non-canned fs_config, we didn't observed any strange
> > > > > before (I ported to cuttlefish and hikey960 with boot success, also as I mentioned before
> > > > > some other vendors already use it.)
> > > > > 
> > > > > I think the following commit is only useful for squashfs since its (non)root inode
> > > > > workflows are different, so need to add in two difference place. But mkfs.erofs is not.
> > > > > https://android.googlesource.com/platform/external/squashfs-tools/+/85a6bc1e52bb911f195c5dc0890717913938c2d1%5E%21/#F0
> > > > > 
> > > > > For root inode is erofs, I think erofs_fspath(path) would return "", so that case
> > > > > is included as well.... Am I missing something?    
> > > > 
> > > > Yes, erofs_fspath(path) returns "" for root inode. However, the above patch add the mount
> > > > point to fspath when specify it, so the real path is "vendor/" which does not exist in canned
> > > > fs_config file. build will report below error:
> > > > 
> > > > failed to find [/vendor/] in canned fs_config    
> > > 
> > > Hmmm... such design is quite strange for me....  
> > 
> > :) i checked the squashfs before, seems root directory is handled in some position. Separated
> > with sub directory fs_config. so i add the goto code in the 1st patch.  
> 
> What confuses me a lot is that we didn't get any strange without canned fs_config
> if mount point prefix is added. I will look into other implementation about this
> later (Another guess is that relates to Android 10 only?).

maybe relates to dynamic partition(intro from Android 10) which not be enabled by some vendors.

> 
> Yeah, the opensource fs_config implementation might be different from HUAWEI
> internal fs_config version since such part was not originally written by me and
> I didn't pay more attention about this part when I was in my previous company.
> But anyway, this cleanup opensource version is already used for some vendors
> as well and I don't get such report... And any formal description about this
> would be helpful for me to understand how fs_config really works..

Now i'm not familar with fs_config also :) I will continue to check when i have
enough time.

Anyway, i observed the issue in canned fs_config since i'm using it. so i decide
to report it and patch it to upstream to verify if it's a real one.

Thx.

> 
> Thanks,
> Gao Xiang
> 
> >   
> > > Could you try the following diff?  
> > 
> > Let's me verify.
> >   
> 

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

[Gao Xiang]

On Tue, Dec 22, 2020 at 06:17:51PM +0800, Yue Hu wrote:
> On Tue, 22 Dec 2020 17:59:06 +0800
> Gao Xiang <hsiangkao@redhat.com> wrote:
> 
> > On Tue, Dec 22, 2020 at 05:46:23PM +0800, Yue Hu wrote:
> > > On Tue, 22 Dec 2020 17:39:52 +0800
> > > Gao Xiang <hsiangkao@redhat.com> wrote:
> > >   
> > > > On Tue, Dec 22, 2020 at 05:30:14PM +0800, Yue Hu wrote:
> > > > 
> > > > ...
> > > >   
> > > > > > > 
> > > > > > >       
> > > > > > > > +	else if (asprintf(&fspath, "%s/%s", cfg.mount_point,
> > > > > > > > +			  erofs_fspath(path)) <= 0)      
> > > > > > > 
> > > > > > > The argument of path will be root directory. And canned fs_config for root directory as
> > > > > > > below:
> > > > > > > 
> > > > > > > 0 0 755 selabel=u:object_r:rootfs:s0 capabilities=0x0
> > > > > > > 
> > > > > > > So, cannot add mount point to root directory for canned fs_config. And what about non-canned
> > > > > > > fs_config?      
> > > > > > 
> > > > > > Not quite sure what you mean. For non-canned fs_config, we didn't observed any strange
> > > > > > before (I ported to cuttlefish and hikey960 with boot success, also as I mentioned before
> > > > > > some other vendors already use it.)
> > > > > > 
> > > > > > I think the following commit is only useful for squashfs since its (non)root inode
> > > > > > workflows are different, so need to add in two difference place. But mkfs.erofs is not.
> > > > > > https://android.googlesource.com/platform/external/squashfs-tools/+/85a6bc1e52bb911f195c5dc0890717913938c2d1%5E%21/#F0
> > > > > > 
> > > > > > For root inode is erofs, I think erofs_fspath(path) would return "", so that case
> > > > > > is included as well.... Am I missing something?    
> > > > > 
> > > > > Yes, erofs_fspath(path) returns "" for root inode. However, the above patch add the mount
> > > > > point to fspath when specify it, so the real path is "vendor/" which does not exist in canned
> > > > > fs_config file. build will report below error:
> > > > > 
> > > > > failed to find [/vendor/] in canned fs_config    
> > > > 
> > > > Hmmm... such design is quite strange for me....  
> > > 
> > > :) i checked the squashfs before, seems root directory is handled in some position. Separated
> > > with sub directory fs_config. so i add the goto code in the 1st patch.  
> > 
> > What confuses me a lot is that we didn't get any strange without canned fs_config
> > if mount point prefix is added. I will look into other implementation about this
> > later (Another guess is that relates to Android 10 only?).
> 
> maybe relates to dynamic partition(intro from Android 10) which not be enabled by some vendors.

I think some of them use dynamic partition AFAIK, but might not be with QSSI
enabled (I'm not sure, anyway, that is minor...)

> 
> > 
> > Yeah, the opensource fs_config implementation might be different from HUAWEI
> > internal fs_config version since such part was not originally written by me and
> > I didn't pay more attention about this part when I was in my previous company.
> > But anyway, this cleanup opensource version is already used for some vendors
> > as well and I don't get such report... And any formal description about this
> > would be helpful for me to understand how fs_config really works..
> 
> Now i'm not familar with fs_config also :) I will continue to check when i have
> enough time.
> 
> Anyway, i observed the issue in canned fs_config since i'm using it. so i decide
> to report it and patch it to upstream to verify if it's a real one.

Yeah, that is somewhat bad and needs fixing if canned fs_config doesn't work
as expected...
My confusion for now is that how to deal with root dir properly (it seems
make_ext4fs doesn't even care about rootdir fs_config at all if my understanding
is correct.)

Also,
https://android.googlesource.com/platform/system/core/+/master/libcutils/fs_config.cpp
https://android.googlesource.com/platform/system/core/+/master/libcutils/canned_fs_config.cpp

are implemented quite different. So look forward to your test result (I tend
to add prefix for fs_config, but drop prefix for canned_fs_config instead.)

Thanks,
Gao Xiang

> 
> Thx.
> 
> > 
> > Thanks,
> > Gao Xiang
> > 
> > >   
> > > > Could you try the following diff?  
> > > 
> > > Let's me verify.
> > >   
> > 
> 

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

[Yue Hu]

On Tue, 22 Dec 2020 17:39:52 +0800
Gao Xiang <hsiangkao@redhat.com> wrote:

> On Tue, Dec 22, 2020 at 05:30:14PM +0800, Yue Hu wrote:
> 
> ...
> 
> > > > 
> > > >     
> > > > > +	else if (asprintf(&fspath, "%s/%s", cfg.mount_point,
> > > > > +			  erofs_fspath(path)) <= 0)    
> > > > 
> > > > The argument of path will be root directory. And canned fs_config for root directory as
> > > > below:
> > > > 
> > > > 0 0 755 selabel=u:object_r:rootfs:s0 capabilities=0x0
> > > > 
> > > > So, cannot add mount point to root directory for canned fs_config. And what about non-canned
> > > > fs_config?    
> > > 
> > > Not quite sure what you mean. For non-canned fs_config, we didn't observed any strange
> > > before (I ported to cuttlefish and hikey960 with boot success, also as I mentioned before
> > > some other vendors already use it.)
> > > 
> > > I think the following commit is only useful for squashfs since its (non)root inode
> > > workflows are different, so need to add in two difference place. But mkfs.erofs is not.
> > > https://android.googlesource.com/platform/external/squashfs-tools/+/85a6bc1e52bb911f195c5dc0890717913938c2d1%5E%21/#F0
> > > 
> > > For root inode is erofs, I think erofs_fspath(path) would return "", so that case
> > > is included as well.... Am I missing something?  
> > 
> > Yes, erofs_fspath(path) returns "" for root inode. However, the above patch add the mount
> > point to fspath when specify it, so the real path is "vendor/" which does not exist in canned
> > fs_config file. build will report below error:
> > 
> > failed to find [/vendor/] in canned fs_config  
> 
> Hmmm... such design is quite strange for me....
> Could you try the following diff?
> 
> diff --git a/lib/inode.c b/lib/inode.c
> index 9469074..9af6179 100644
> --- a/lib/inode.c
> +++ b/lib/inode.c
> @@ -698,11 +698,14 @@ 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;
> +	bool alloced;
>  	char *fspath;
>  
>  	inode->capabilities = 0;
> -	if (!cfg.mount_point)
> -		fspath = erofs_fspath(path);
> +
> +	alloced = (cfg.mount_point && erofs_fspath(path)[0] != '\0');
> +	if (!alloced)
> +		fspath = (char *)erofs_fspath(path);
>  	else if (asprintf(&fspath, "%s/%s", cfg.mount_point,
>  			  erofs_fspath(path)) <= 0)
>  		return -ENOMEM;
> @@ -718,7 +721,7 @@ int erofs_droid_inode_fsconfig(struct erofs_inode *inode,
>  			  cfg.target_out_path,
>  			  &uid, &gid, &mode, &inode->capabilities);
>  
> -	if (cfg.mount_point)
> +	if (alloced)
>  		free(fspath);
>  	st->st_uid = uid;
>  	st->st_gid = gid;
> 
> if it works, will redo a formal patch then....

Works for me for canned fs_config.

Thx.

> 
> Thanks,
> Gao Xiang
> 
> >   
> > > 
> > > Thanks,
> > > Gao Xiang
> > >   
> > > > 
> > > > Thx.
> > > > 
> > > >     
> > > > > +		return -ENOMEM;
> > > > > +
> > > > > +
> > > > >  	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);
> > > > > +
> > > > > +	if (cfg.mount_point)
> > > > >  		free(fspath);
> > > > > -	}
> > > > >  	st->st_uid = uid;
> > > > >  	st->st_gid = gid;
> > > > >  	st->st_mode = mode | stat_file_type_mask;    
> > > >     
> > >   
> >   
> 

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

[Gao Xiang]

On Tue, Dec 22, 2020 at 06:34:11PM +0800, Yue Hu wrote:

...

> > 
> > Hmmm... such design is quite strange for me....
> > Could you try the following diff?
> > 
> > diff --git a/lib/inode.c b/lib/inode.c
> > index 9469074..9af6179 100644
> > --- a/lib/inode.c
> > +++ b/lib/inode.c
> > @@ -698,11 +698,14 @@ 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;
> > +	bool alloced;
> >  	char *fspath;
> >  
> >  	inode->capabilities = 0;
> > -	if (!cfg.mount_point)
> > -		fspath = erofs_fspath(path);
> > +
> > +	alloced = (cfg.mount_point && erofs_fspath(path)[0] != '\0');
> > +	if (!alloced)
> > +		fspath = (char *)erofs_fspath(path);
> >  	else if (asprintf(&fspath, "%s/%s", cfg.mount_point,
> >  			  erofs_fspath(path)) <= 0)
> >  		return -ENOMEM;
> > @@ -718,7 +721,7 @@ int erofs_droid_inode_fsconfig(struct erofs_inode *inode,
> >  			  cfg.target_out_path,
> >  			  &uid, &gid, &mode, &inode->capabilities);
> >  
> > -	if (cfg.mount_point)
> > +	if (alloced)
> >  		free(fspath);
> >  	st->st_uid = uid;
> >  	st->st_gid = gid;
> > 
> > if it works, will redo a formal patch then....
> 
> Works for me for canned fs_config.

Ok, if it also works fine for non-canned fs_config on your side,
I will redo a formal patch later... sigh :(

Thanks,
Gao Xiang

> 
> Thx.

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

[Yue Hu]

On Tue, 22 Dec 2020 18:33:20 +0800
Gao Xiang <hsiangkao@redhat.com> wrote:

> On Tue, Dec 22, 2020 at 06:17:51PM +0800, Yue Hu wrote:
> > On Tue, 22 Dec 2020 17:59:06 +0800
> > Gao Xiang <hsiangkao@redhat.com> wrote:
> >   
> > > On Tue, Dec 22, 2020 at 05:46:23PM +0800, Yue Hu wrote:  
> > > > On Tue, 22 Dec 2020 17:39:52 +0800
> > > > Gao Xiang <hsiangkao@redhat.com> wrote:
> > > >     
> > > > > On Tue, Dec 22, 2020 at 05:30:14PM +0800, Yue Hu wrote:
> > > > > 
> > > > > ...
> > > > >     
> > > > > > > > 
> > > > > > > >         
> > > > > > > > > +	else if (asprintf(&fspath, "%s/%s", cfg.mount_point,
> > > > > > > > > +			  erofs_fspath(path)) <= 0)        
> > > > > > > > 
> > > > > > > > The argument of path will be root directory. And canned fs_config for root directory as
> > > > > > > > below:
> > > > > > > > 
> > > > > > > > 0 0 755 selabel=u:object_r:rootfs:s0 capabilities=0x0
> > > > > > > > 
> > > > > > > > So, cannot add mount point to root directory for canned fs_config. And what about non-canned
> > > > > > > > fs_config?        
> > > > > > > 
> > > > > > > Not quite sure what you mean. For non-canned fs_config, we didn't observed any strange
> > > > > > > before (I ported to cuttlefish and hikey960 with boot success, also as I mentioned before
> > > > > > > some other vendors already use it.)
> > > > > > > 
> > > > > > > I think the following commit is only useful for squashfs since its (non)root inode
> > > > > > > workflows are different, so need to add in two difference place. But mkfs.erofs is not.
> > > > > > > https://android.googlesource.com/platform/external/squashfs-tools/+/85a6bc1e52bb911f195c5dc0890717913938c2d1%5E%21/#F0
> > > > > > > 
> > > > > > > For root inode is erofs, I think erofs_fspath(path) would return "", so that case
> > > > > > > is included as well.... Am I missing something?      
> > > > > > 
> > > > > > Yes, erofs_fspath(path) returns "" for root inode. However, the above patch add the mount
> > > > > > point to fspath when specify it, so the real path is "vendor/" which does not exist in canned
> > > > > > fs_config file. build will report below error:
> > > > > > 
> > > > > > failed to find [/vendor/] in canned fs_config      
> > > > > 
> > > > > Hmmm... such design is quite strange for me....    
> > > > 
> > > > :) i checked the squashfs before, seems root directory is handled in some position. Separated
> > > > with sub directory fs_config. so i add the goto code in the 1st patch.    
> > > 
> > > What confuses me a lot is that we didn't get any strange without canned fs_config
> > > if mount point prefix is added. I will look into other implementation about this
> > > later (Another guess is that relates to Android 10 only?).  
> > 
> > maybe relates to dynamic partition(intro from Android 10) which not be enabled by some vendors.  
> 
> I think some of them use dynamic partition AFAIK, but might not be with QSSI
> enabled (I'm not sure, anyway, that is minor...)
> 
> >   
> > > 
> > > Yeah, the opensource fs_config implementation might be different from HUAWEI
> > > internal fs_config version since such part was not originally written by me and
> > > I didn't pay more attention about this part when I was in my previous company.
> > > But anyway, this cleanup opensource version is already used for some vendors
> > > as well and I don't get such report... And any formal description about this
> > > would be helpful for me to understand how fs_config really works..  
> > 
> > Now i'm not familar with fs_config also :) I will continue to check when i have
> > enough time.
> > 
> > Anyway, i observed the issue in canned fs_config since i'm using it. so i decide
> > to report it and patch it to upstream to verify if it's a real one.  
> 
> Yeah, that is somewhat bad and needs fixing if canned fs_config doesn't work
> as expected...
> My confusion for now is that how to deal with root dir properly (it seems
> make_ext4fs doesn't even care about rootdir fs_config at all if my understanding
> is correct.)
> 
> Also,
> https://android.googlesource.com/platform/system/core/+/master/libcutils/fs_config.cpp
> https://android.googlesource.com/platform/system/core/+/master/libcutils/canned_fs_config.cpp
> 
> are implemented quite different. So look forward to your test result (I tend
> to add prefix for fs_config, but drop prefix for canned_fs_config instead.)

It works for canned fs_config i'm using. We can simplify the test enviroment.
canned fs_config file content/format (e.g. mount point is vendor) as below: 

 0 2000 755 selabel=u:object_r:rootfs:s0 capabilities=0x0
vendor/app 0 2000 755 selabel=u:object_r:vendor_app_file:s0 capabilities=0x0
vendor/app/CACertService 0 2000 755 selabel=u:object_r:vendor_app_file:s0 capabilities=0x0
vendor/app/CACertService/CACertService.apk 0 0 644 selabel=u:object_r:vendor_app_file:s0 capabilities=0x0
vendor/app/CACertService/oat 0 2000 755 selabel=u:object_r:vendor_app_file:s0 capabilities=0x0
vendor/app/CACertService/oat/arm64 0 2000 755 selabel=u:object_r:vendor_app_file:s0 capabilities=0x0

The 1st line is for root inode search and the others are for sub inode like search.

> 
> Thanks,
> Gao Xiang
> 
> > 
> > Thx.
> >   
> > > 
> > > Thanks,
> > > Gao Xiang
> > >   
> > > >     
> > > > > Could you try the following diff?    
> > > > 
> > > > Let's me verify.
> > > >     
> > >   
> >   
> 

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

[Yue Hu]

On Tue, 22 Dec 2020 18:47:00 +0800
Gao Xiang <hsiangkao@redhat.com> wrote:

> On Tue, Dec 22, 2020 at 06:34:11PM +0800, Yue Hu wrote:
> 
> ...
> 
> > > 
> > > Hmmm... such design is quite strange for me....
> > > Could you try the following diff?
> > > 
> > > diff --git a/lib/inode.c b/lib/inode.c
> > > index 9469074..9af6179 100644
> > > --- a/lib/inode.c
> > > +++ b/lib/inode.c
> > > @@ -698,11 +698,14 @@ 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;
> > > +	bool alloced;
> > >  	char *fspath;
> > >  
> > >  	inode->capabilities = 0;
> > > -	if (!cfg.mount_point)
> > > -		fspath = erofs_fspath(path);
> > > +
> > > +	alloced = (cfg.mount_point && erofs_fspath(path)[0] != '\0');
> > > +	if (!alloced)
> > > +		fspath = (char *)erofs_fspath(path);
> > >  	else if (asprintf(&fspath, "%s/%s", cfg.mount_point,
> > >  			  erofs_fspath(path)) <= 0)
> > >  		return -ENOMEM;
> > > @@ -718,7 +721,7 @@ int erofs_droid_inode_fsconfig(struct erofs_inode *inode,
> > >  			  cfg.target_out_path,
> > >  			  &uid, &gid, &mode, &inode->capabilities);
> > >  
> > > -	if (cfg.mount_point)
> > > +	if (alloced)
> > >  		free(fspath);
> > >  	st->st_uid = uid;
> > >  	st->st_gid = gid;
> > > 
> > > if it works, will redo a formal patch then....  
> > 
> > Works for me for canned fs_config.  
> 
> Ok, if it also works fine for non-canned fs_config on your side,
> I will redo a formal patch later... sigh :(

Hi Xiang,

I just remove the "--fs-config-file" in my test enviroment, after that,
build and boot are all working fine. 

If canned fs_config never used by others before, i think it's a bug. 

Thx.

> 
> Thanks,
> Gao Xiang
> 
> > 
> > Thx.  
> 

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

[Gao Xiang]

Hi Yue,

On Thu, Dec 24, 2020 at 11:45:42AM +0800, Yue Hu wrote:
> On Tue, 22 Dec 2020 18:47:00 +0800
> Gao Xiang <hsiangkao@redhat.com> wrote:
> 

...

> > > Works for me for canned fs_config.  
> > 
> > Ok, if it also works fine for non-canned fs_config on your side,
> > I will redo a formal patch later... sigh :(
> 
> Hi Xiang,
> 
> I just remove the "--fs-config-file" in my test enviroment, after that,
> build and boot are all working fine. 
> 
> If canned fs_config never used by others before, i think it's a bug. 

Thanks for your report. I agree with your conclusion. Let me seek some
extra free time later to form a formal patch and test on my PC then.

I'll finish this this week, don't worry :)

Thanks,
Gao Xiang

> 
> Thx.
> 
> > 
> > Thanks,
> > Gao Xiang
> > 
> > > 
> > > Thx.  
> > 
>