* [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 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 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: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.