* [PATCH] AOSP: erofs-utils: fix sub directory prefix path in canned fs_config
@ 2020-12-22 2:04 Yue Hu
2020-12-22 3:44 ` Gao Xiang
0 siblings, 1 reply; 20+ messages in thread
From: Yue Hu @ 2020-12-22 2:04 UTC (permalink / raw
To: xiang, bluce.lee, hsiangkao; +Cc: huyue2, linux-erofs
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
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH] AOSP: erofs-utils: fix sub directory prefix path in canned fs_config
2020-12-22 2:04 [PATCH] AOSP: erofs-utils: fix sub directory prefix path in canned fs_config Yue Hu
@ 2020-12-22 3:44 ` Gao Xiang
2020-12-22 4:47 ` Yue Hu
0 siblings, 1 reply; 20+ messages in thread
From: Gao Xiang @ 2020-12-22 3:44 UTC (permalink / raw
To: Yue Hu; +Cc: xiang, linux-erofs, huyue2
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
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] AOSP: erofs-utils: fix sub directory prefix path in canned fs_config
2020-12-22 3:44 ` Gao Xiang
@ 2020-12-22 4:47 ` Yue Hu
2020-12-22 6:31 ` Gao Xiang
0 siblings, 1 reply; 20+ messages in thread
From: Yue Hu @ 2020-12-22 4:47 UTC (permalink / raw
To: Gao Xiang; +Cc: xiang, linux-erofs, huyue2
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
> >
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] AOSP: erofs-utils: fix sub directory prefix path in canned fs_config
2020-12-22 4:47 ` Yue Hu
@ 2020-12-22 6:31 ` Gao Xiang
2020-12-22 7:04 ` Gao Xiang
0 siblings, 1 reply; 20+ messages in thread
From: Gao Xiang @ 2020-12-22 6:31 UTC (permalink / raw
To: Yue Hu; +Cc: xiang, linux-erofs, huyue2
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] AOSP: erofs-utils: fix sub directory prefix path in canned fs_config
2020-12-22 6:31 ` Gao Xiang
@ 2020-12-22 7:04 ` Gao Xiang
2020-12-22 7:12 ` Yue Hu
2020-12-22 8:10 ` Yue Hu
0 siblings, 2 replies; 20+ messages in thread
From: Gao Xiang @ 2020-12-22 7:04 UTC (permalink / raw
To: Yue Hu; +Cc: xiang, linux-erofs, huyue2
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
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH] AOSP: erofs-utils: fix sub directory prefix path in canned fs_config
2020-12-22 7:04 ` Gao Xiang
@ 2020-12-22 7:12 ` Yue Hu
2020-12-22 8:10 ` Yue Hu
1 sibling, 0 replies; 20+ messages in thread
From: Yue Hu @ 2020-12-22 7:12 UTC (permalink / raw
To: Gao Xiang; +Cc: zbestahu, huyue2, xiang, linux-erofs
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;
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] AOSP: erofs-utils: fix sub directory prefix path in canned fs_config
2020-12-22 7:04 ` Gao Xiang
2020-12-22 7:12 ` Yue Hu
@ 2020-12-22 8:10 ` Yue Hu
2020-12-22 9:13 ` Gao Xiang
1 sibling, 1 reply; 20+ messages in thread
From: Yue Hu @ 2020-12-22 8:10 UTC (permalink / raw
To: Gao Xiang; +Cc: xiang, linux-erofs, huyue2
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;
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] AOSP: erofs-utils: fix sub directory prefix path in canned fs_config
2020-12-22 8:10 ` Yue Hu
@ 2020-12-22 9:13 ` Gao Xiang
2020-12-22 9:27 ` Gao Xiang
2020-12-22 9:30 ` Yue Hu
0 siblings, 2 replies; 20+ messages in thread
From: Gao Xiang @ 2020-12-22 9:13 UTC (permalink / raw
To: Yue Hu; +Cc: xiang, linux-erofs, huyue2
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;
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] AOSP: erofs-utils: fix sub directory prefix path in canned fs_config
2020-12-22 9:13 ` Gao Xiang
@ 2020-12-22 9:27 ` Gao Xiang
2020-12-22 9:30 ` Yue Hu
1 sibling, 0 replies; 20+ messages in thread
From: Gao Xiang @ 2020-12-22 9:27 UTC (permalink / raw
To: Yue Hu; +Cc: xiang, linux-erofs, huyue2
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.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] AOSP: erofs-utils: fix sub directory prefix path in canned fs_config
2020-12-22 9:13 ` Gao Xiang
2020-12-22 9:27 ` Gao Xiang
@ 2020-12-22 9:30 ` Yue Hu
2020-12-22 9:39 ` Gao Xiang
1 sibling, 1 reply; 20+ messages in thread
From: Yue Hu @ 2020-12-22 9:30 UTC (permalink / raw
To: Gao Xiang; +Cc: xiang, linux-erofs, huyue2
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;
> >
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] AOSP: erofs-utils: fix sub directory prefix path in canned fs_config
2020-12-22 9:30 ` Yue Hu
@ 2020-12-22 9:39 ` Gao Xiang
2020-12-22 9:46 ` Yue Hu
2020-12-22 10:34 ` Yue Hu
0 siblings, 2 replies; 20+ messages in thread
From: Gao Xiang @ 2020-12-22 9:39 UTC (permalink / raw
To: Yue Hu; +Cc: xiang, linux-erofs, huyue2
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;
> > >
> >
>
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH] AOSP: erofs-utils: fix sub directory prefix path in canned fs_config
2020-12-22 9:39 ` Gao Xiang
@ 2020-12-22 9:46 ` Yue Hu
2020-12-22 9:59 ` Gao Xiang
2020-12-22 10:34 ` Yue Hu
1 sibling, 1 reply; 20+ messages in thread
From: Yue Hu @ 2020-12-22 9:46 UTC (permalink / raw
To: Gao Xiang; +Cc: xiang, linux-erofs, huyue2
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;
> > > >
> > >
> >
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] AOSP: erofs-utils: fix sub directory prefix path in canned fs_config
2020-12-22 9:46 ` Yue Hu
@ 2020-12-22 9:59 ` Gao Xiang
2020-12-22 10:17 ` Yue Hu
0 siblings, 1 reply; 20+ messages in thread
From: Gao Xiang @ 2020-12-22 9:59 UTC (permalink / raw
To: Yue Hu; +Cc: xiang, linux-erofs, huyue2
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.
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] AOSP: erofs-utils: fix sub directory prefix path in canned fs_config
2020-12-22 9:59 ` Gao Xiang
@ 2020-12-22 10:17 ` Yue Hu
2020-12-22 10:33 ` Gao Xiang
0 siblings, 1 reply; 20+ messages in thread
From: Yue Hu @ 2020-12-22 10:17 UTC (permalink / raw
To: Gao Xiang; +Cc: xiang, linux-erofs, huyue2
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.
> >
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] AOSP: erofs-utils: fix sub directory prefix path in canned fs_config
2020-12-22 10:17 ` Yue Hu
@ 2020-12-22 10:33 ` Gao Xiang
2020-12-22 11:07 ` Yue Hu
0 siblings, 1 reply; 20+ messages in thread
From: Gao Xiang @ 2020-12-22 10:33 UTC (permalink / raw
To: Yue Hu; +Cc: xiang, linux-erofs, huyue2
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.
> > >
> >
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] AOSP: erofs-utils: fix sub directory prefix path in canned fs_config
2020-12-22 9:39 ` Gao Xiang
2020-12-22 9:46 ` Yue Hu
@ 2020-12-22 10:34 ` Yue Hu
2020-12-22 10:47 ` Gao Xiang
1 sibling, 1 reply; 20+ messages in thread
From: Yue Hu @ 2020-12-22 10:34 UTC (permalink / raw
To: Gao Xiang; +Cc: xiang, linux-erofs, huyue2
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;
> > > >
> > >
> >
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] AOSP: erofs-utils: fix sub directory prefix path in canned fs_config
2020-12-22 10:34 ` Yue Hu
@ 2020-12-22 10:47 ` Gao Xiang
2020-12-24 3:45 ` Yue Hu
0 siblings, 1 reply; 20+ messages in thread
From: Gao Xiang @ 2020-12-22 10:47 UTC (permalink / raw
To: Yue Hu; +Cc: xiang, linux-erofs, huyue2
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.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] AOSP: erofs-utils: fix sub directory prefix path in canned fs_config
2020-12-22 10:33 ` Gao Xiang
@ 2020-12-22 11:07 ` Yue Hu
0 siblings, 0 replies; 20+ messages in thread
From: Yue Hu @ 2020-12-22 11:07 UTC (permalink / raw
To: Gao Xiang; +Cc: huyue2, xiang, linux-erofs, zhangwen
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.
> > > >
> > >
> >
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] AOSP: erofs-utils: fix sub directory prefix path in canned fs_config
2020-12-22 10:47 ` Gao Xiang
@ 2020-12-24 3:45 ` Yue Hu
2020-12-24 4:19 ` Gao Xiang
0 siblings, 1 reply; 20+ messages in thread
From: Yue Hu @ 2020-12-24 3:45 UTC (permalink / raw
To: Gao Xiang; +Cc: huyue2, xiang, linux-erofs, zhangwen
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.
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] AOSP: erofs-utils: fix sub directory prefix path in canned fs_config
2020-12-24 3:45 ` Yue Hu
@ 2020-12-24 4:19 ` Gao Xiang
0 siblings, 0 replies; 20+ messages in thread
From: Gao Xiang @ 2020-12-24 4:19 UTC (permalink / raw
To: Yue Hu; +Cc: huyue2, xiang, linux-erofs, zhangwen
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.
> >
>
^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2020-12-24 4:20 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-12-22 2:04 [PATCH] AOSP: erofs-utils: fix sub directory prefix path in canned fs_config Yue Hu
2020-12-22 3:44 ` Gao Xiang
2020-12-22 4:47 ` Yue Hu
2020-12-22 6:31 ` Gao Xiang
2020-12-22 7:04 ` Gao Xiang
2020-12-22 7:12 ` Yue Hu
2020-12-22 8:10 ` Yue Hu
2020-12-22 9:13 ` Gao Xiang
2020-12-22 9:27 ` Gao Xiang
2020-12-22 9:30 ` Yue Hu
2020-12-22 9:39 ` Gao Xiang
2020-12-22 9:46 ` Yue Hu
2020-12-22 9:59 ` Gao Xiang
2020-12-22 10:17 ` Yue Hu
2020-12-22 10:33 ` Gao Xiang
2020-12-22 11:07 ` Yue Hu
2020-12-22 10:34 ` Yue Hu
2020-12-22 10:47 ` Gao Xiang
2020-12-24 3:45 ` Yue Hu
2020-12-24 4:19 ` Gao Xiang
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.