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

[Yue Hu <zbestahu@gmail.com>]

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