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

[Gao Xiang <hsiangkao@redhat.com>]

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