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

[Yue Hu <zbestahu@gmail.com>]

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
> >   
>