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