* [PATCH] erofs: fix erofs_module_init & exit @ 2018-07-05 5:03 Gao Xiang 2018-07-05 16:19 ` Chao Yu 2018-07-06 11:30 ` [PATCH v2] " Gao Xiang 0 siblings, 2 replies; 8+ messages in thread From: Gao Xiang @ 2018-07-05 5:03 UTC (permalink / raw) This patch adds missing parts and EROFS ondisk layout check as well. Signed-off-by: Gao Xiang <gaoxiang25 at huawei.com> --- fs/erofs/erofs_fs.h | 13 +++++++++++++ fs/erofs/super.c | 28 +++++++++++++++++++++------- 2 files changed, 34 insertions(+), 7 deletions(-) diff --git a/fs/erofs/erofs_fs.h b/fs/erofs/erofs_fs.h index ed943d4..0dbf08d 100644 --- a/fs/erofs/erofs_fs.h +++ b/fs/erofs/erofs_fs.h @@ -253,5 +253,18 @@ enum { #define EROFS_NAME_LEN 255 +/* check the EROFS on-disk layout strictly at compile time */ +static inline void erofs_check_ondisk_layout(void) +{ + BUILD_BUG_ON(sizeof(struct erofs_super_block) != 128); + BUILD_BUG_ON(sizeof(struct erofs_inode_v1) != 32); + BUILD_BUG_ON(sizeof(struct erofs_inode_v2) != 64); + BUILD_BUG_ON(sizeof(struct erofs_xattr_ibody_header) != 12); + BUILD_BUG_ON(sizeof(struct erofs_xattr_entry) != 4); + BUILD_BUG_ON(sizeof(struct erofs_extent_header) != 16); + BUILD_BUG_ON(sizeof(struct erofs_decompressed_index_vle) != 8); + BUILD_BUG_ON(sizeof(struct erofs_dirent) != 12); +} + #endif diff --git a/fs/erofs/super.c b/fs/erofs/super.c index a1826b9..d104d88 100644 --- a/fs/erofs/super.c +++ b/fs/erofs/super.c @@ -37,6 +37,12 @@ static int erofs_init_inode_cache(void) return erofs_inode_cachep != NULL ? 0 : -ENOMEM; } +static void erofs_exit_inode_cache(void) +{ + BUG_ON(erofs_inode_cachep == NULL); + kmem_cache_destroy(erofs_inode_cachep); +} + static struct inode *alloc_inode(struct super_block *sb) { struct erofs_vnode *vi = @@ -398,22 +404,30 @@ int __init erofs_module_init(void) { int err; + erofs_check_ondisk_layout(); infoln("Initializing erofs " EROFS_VERSION); err = erofs_init_inode_cache(); - if (!err) { - err = register_filesystem(&erofs_fs_type); - if (!err) { - infoln("Successfully to initialize erofs"); - return 0; - } - } + if (err) + goto icache_err; + + err = register_filesystem(&erofs_fs_type); + if (err) + goto fs_err; + + infoln("Successfully to initialize erofs"); + return 0; + +fs_err: + erofs_exit_inode_cache(); +icache_err: return err; } void __exit erofs_module_exit(void) { unregister_filesystem(&erofs_fs_type); + erofs_exit_inode_cache(); infoln("Successfully finalize erofs"); } -- 1.9.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH] erofs: fix erofs_module_init & exit 2018-07-05 5:03 [PATCH] erofs: fix erofs_module_init & exit Gao Xiang @ 2018-07-05 16:19 ` Chao Yu 2018-07-05 16:39 ` Gao Xiang 2018-07-06 11:30 ` [PATCH v2] " Gao Xiang 1 sibling, 1 reply; 8+ messages in thread From: Chao Yu @ 2018-07-05 16:19 UTC (permalink / raw) Hi Xiang, On 2018/7/5 13:03, Gao Xiang wrote: > This patch adds missing parts and EROFS ondisk > layout check as well. > > Signed-off-by: Gao Xiang <gaoxiang25 at huawei.com> > --- > fs/erofs/erofs_fs.h | 13 +++++++++++++ > fs/erofs/super.c | 28 +++++++++++++++++++++------- > 2 files changed, 34 insertions(+), 7 deletions(-) > > diff --git a/fs/erofs/erofs_fs.h b/fs/erofs/erofs_fs.h > index ed943d4..0dbf08d 100644 > --- a/fs/erofs/erofs_fs.h > +++ b/fs/erofs/erofs_fs.h > @@ -253,5 +253,18 @@ enum { > > #define EROFS_NAME_LEN 255 > > +/* check the EROFS on-disk layout strictly at compile time */ > +static inline void erofs_check_ondisk_layout(void) > +{ > + BUILD_BUG_ON(sizeof(struct erofs_super_block) != 128); > + BUILD_BUG_ON(sizeof(struct erofs_inode_v1) != 32); > + BUILD_BUG_ON(sizeof(struct erofs_inode_v2) != 64); > + BUILD_BUG_ON(sizeof(struct erofs_xattr_ibody_header) != 12); > + BUILD_BUG_ON(sizeof(struct erofs_xattr_entry) != 4); > + BUILD_BUG_ON(sizeof(struct erofs_extent_header) != 16); > + BUILD_BUG_ON(sizeof(struct erofs_decompressed_index_vle) != 8); > + BUILD_BUG_ON(sizeof(struct erofs_dirent) != 12); We'd better define some macros include above constants. Other parts look good to me. Thanks, > +} > + > #endif > > diff --git a/fs/erofs/super.c b/fs/erofs/super.c > index a1826b9..d104d88 100644 > --- a/fs/erofs/super.c > +++ b/fs/erofs/super.c > @@ -37,6 +37,12 @@ static int erofs_init_inode_cache(void) > return erofs_inode_cachep != NULL ? 0 : -ENOMEM; > } > > +static void erofs_exit_inode_cache(void) > +{ > + BUG_ON(erofs_inode_cachep == NULL); > + kmem_cache_destroy(erofs_inode_cachep); > +} > + > static struct inode *alloc_inode(struct super_block *sb) > { > struct erofs_vnode *vi = > @@ -398,22 +404,30 @@ int __init erofs_module_init(void) > { > int err; > > + erofs_check_ondisk_layout(); > infoln("Initializing erofs " EROFS_VERSION); > > err = erofs_init_inode_cache(); > - if (!err) { > - err = register_filesystem(&erofs_fs_type); > - if (!err) { > - infoln("Successfully to initialize erofs"); > - return 0; > - } > - } > + if (err) > + goto icache_err; > + > + err = register_filesystem(&erofs_fs_type); > + if (err) > + goto fs_err; > + > + infoln("Successfully to initialize erofs"); > + return 0; > + > +fs_err: > + erofs_exit_inode_cache(); > +icache_err: > return err; > } > > void __exit erofs_module_exit(void) > { > unregister_filesystem(&erofs_fs_type); > + erofs_exit_inode_cache(); > infoln("Successfully finalize erofs"); > } > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] erofs: fix erofs_module_init & exit 2018-07-05 16:19 ` Chao Yu @ 2018-07-05 16:39 ` Gao Xiang 2018-07-06 1:30 ` Chao Yu 0 siblings, 1 reply; 8+ messages in thread From: Gao Xiang @ 2018-07-05 16:39 UTC (permalink / raw) Hi Chao, On 2018/7/6 0:19, Chao Yu wrote: > Hi Xiang, > > On 2018/7/5 13:03, Gao Xiang wrote: >> This patch adds missing parts and EROFS ondisk >> layout check as well. >> >> Signed-off-by: Gao Xiang <gaoxiang25 at huawei.com> >> --- >> fs/erofs/erofs_fs.h | 13 +++++++++++++ >> fs/erofs/super.c | 28 +++++++++++++++++++++------- >> 2 files changed, 34 insertions(+), 7 deletions(-) >> >> diff --git a/fs/erofs/erofs_fs.h b/fs/erofs/erofs_fs.h >> index ed943d4..0dbf08d 100644 >> --- a/fs/erofs/erofs_fs.h >> +++ b/fs/erofs/erofs_fs.h >> @@ -253,5 +253,18 @@ enum { >> >> #define EROFS_NAME_LEN 255 >> >> +/* check the EROFS on-disk layout strictly at compile time */ >> +static inline void erofs_check_ondisk_layout(void) >> +{ >> + BUILD_BUG_ON(sizeof(struct erofs_super_block) != 128); >> + BUILD_BUG_ON(sizeof(struct erofs_inode_v1) != 32); >> + BUILD_BUG_ON(sizeof(struct erofs_inode_v2) != 64); >> + BUILD_BUG_ON(sizeof(struct erofs_xattr_ibody_header) != 12); >> + BUILD_BUG_ON(sizeof(struct erofs_xattr_entry) != 4); >> + BUILD_BUG_ON(sizeof(struct erofs_extent_header) != 16); >> + BUILD_BUG_ON(sizeof(struct erofs_decompressed_index_vle) != 8); >> + BUILD_BUG_ON(sizeof(struct erofs_dirent) != 12); > > We'd better define some macros include above constants. > > Other parts look good to me. > > Thanks, > I was also thinking of wrapping in macros, but I thought over again, it seems not too necessary because: 1) the above code is already well straight-forward to view and change if needed... BUILD_BUG_ON(sizeof(struct erofs_super_block) != 128); <==> #define EROFS_SUPER_BLOCK_ONDISK_SIZE 128 EROFS_SUPER_BLOCK_ONDISK_SIZE is not really needed since we could use sizeof(struct erofs_super_block) directly in code. 2) Besides, I also observed the other file systems, such as xfs, sysv, minix, and ubifs, eg. fs/jffs2/super.c: 366 /* Paranoia checks for on-medium structures. If we ask GCC 367 to pack them with __attribute__((packed)) then it _also_ 368 assumes that they're not aligned -- so it emits crappy 369 code on some architectures. Ideally we want an attribute 370 which means just 'no padding', without the alignment 371 thing. But GCC doesn't have that -- we have to just 372 hope the structs are the right sizes, instead. */ 373 BUILD_BUG_ON(sizeof(struct jffs2_unknown_node) != 12); 374 BUILD_BUG_ON(sizeof(struct jffs2_raw_dirent) != 40); 375 BUILD_BUG_ON(sizeof(struct jffs2_raw_inode) != 68); 376 BUILD_BUG_ON(sizeof(struct jffs2_raw_summary) != 32); fs/minix/inode.c: 174 BUILD_BUG_ON(32 != sizeof (struct minix_inode)); 175 BUILD_BUG_ON(64 != sizeof(struct minix2_inode)); fs/ubifs/super.c: 359 BUILD_BUG_ON(1024 != sizeof (struct xenix_super_block)); 360 BUILD_BUG_ON(512 != sizeof (struct sysv4_super_block)); 361 BUILD_BUG_ON(512 != sizeof (struct sysv2_super_block)); 362 BUILD_BUG_ON(500 != sizeof (struct coh_super_block)); 363 BUILD_BUG_ON(64 != sizeof (struct sysv_inode)); fs/ubifs/super.c 2211 BUILD_BUG_ON(UBIFS_MAX_NODE_SZ & 7); 2212 BUILD_BUG_ON(MIN_WRITE_SZ & 7); 2213 2214 /* Check min. node size */ 2215 BUILD_BUG_ON(UBIFS_INO_NODE_SZ < MIN_WRITE_SZ); 2216 BUILD_BUG_ON(UBIFS_DENT_NODE_SZ < MIN_WRITE_SZ); 2217 BUILD_BUG_ON(UBIFS_XENT_NODE_SZ < MIN_WRITE_SZ); 2218 BUILD_BUG_ON(UBIFS_TRUN_NODE_SZ < MIN_WRITE_SZ); 2219 2220 BUILD_BUG_ON(UBIFS_MAX_DENT_NODE_SZ > UBIFS_MAX_NODE_SZ); 2221 BUILD_BUG_ON(UBIFS_MAX_XENT_NODE_SZ > UBIFS_MAX_NODE_SZ); 2222 BUILD_BUG_ON(UBIFS_MAX_DATA_NODE_SZ > UBIFS_MAX_NODE_SZ); 2223 BUILD_BUG_ON(UBIFS_MAX_INO_NODE_SZ > UBIFS_MAX_NODE_SZ); 2224 2225 /* Defined node sizes */ 2226 BUILD_BUG_ON(UBIFS_SB_NODE_SZ != 4096); 2227 BUILD_BUG_ON(UBIFS_MST_NODE_SZ != 512); 2228 BUILD_BUG_ON(UBIFS_INO_NODE_SZ != 160); 2229 BUILD_BUG_ON(UBIFS_REF_NODE_SZ != 64); how do you think? :) If you think it is necessary, I can change into marcos immediately~ Thanks, Gao Xiang >> +} >> + >> #endif >> >> diff --git a/fs/erofs/super.c b/fs/erofs/super.c >> index a1826b9..d104d88 100644 >> --- a/fs/erofs/super.c >> +++ b/fs/erofs/super.c >> @@ -37,6 +37,12 @@ static int erofs_init_inode_cache(void) >> return erofs_inode_cachep != NULL ? 0 : -ENOMEM; >> } >> >> +static void erofs_exit_inode_cache(void) >> +{ >> + BUG_ON(erofs_inode_cachep == NULL); >> + kmem_cache_destroy(erofs_inode_cachep); >> +} >> + >> static struct inode *alloc_inode(struct super_block *sb) >> { >> struct erofs_vnode *vi = >> @@ -398,22 +404,30 @@ int __init erofs_module_init(void) >> { >> int err; >> >> + erofs_check_ondisk_layout(); >> infoln("Initializing erofs " EROFS_VERSION); >> >> err = erofs_init_inode_cache(); >> - if (!err) { >> - err = register_filesystem(&erofs_fs_type); >> - if (!err) { >> - infoln("Successfully to initialize erofs"); >> - return 0; >> - } >> - } >> + if (err) >> + goto icache_err; >> + >> + err = register_filesystem(&erofs_fs_type); >> + if (err) >> + goto fs_err; >> + >> + infoln("Successfully to initialize erofs"); >> + return 0; >> + >> +fs_err: >> + erofs_exit_inode_cache(); >> +icache_err: >> return err; >> } >> >> void __exit erofs_module_exit(void) >> { >> unregister_filesystem(&erofs_fs_type); >> + erofs_exit_inode_cache(); >> infoln("Successfully finalize erofs"); >> } >> >> ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] erofs: fix erofs_module_init & exit 2018-07-05 16:39 ` Gao Xiang @ 2018-07-06 1:30 ` Chao Yu 2018-07-06 2:09 ` Gao Xiang 0 siblings, 1 reply; 8+ messages in thread From: Chao Yu @ 2018-07-06 1:30 UTC (permalink / raw) Hi Xiang, On 2018/7/6 0:39, Gao Xiang wrote: > Hi Chao, > > On 2018/7/6 0:19, Chao Yu wrote: >> Hi Xiang, >> >> On 2018/7/5 13:03, Gao Xiang wrote: >>> This patch adds missing parts and EROFS ondisk >>> layout check as well. >>> >>> Signed-off-by: Gao Xiang <gaoxiang25 at huawei.com> >>> --- >>> fs/erofs/erofs_fs.h | 13 +++++++++++++ >>> fs/erofs/super.c | 28 +++++++++++++++++++++------- >>> 2 files changed, 34 insertions(+), 7 deletions(-) >>> >>> diff --git a/fs/erofs/erofs_fs.h b/fs/erofs/erofs_fs.h >>> index ed943d4..0dbf08d 100644 >>> --- a/fs/erofs/erofs_fs.h >>> +++ b/fs/erofs/erofs_fs.h >>> @@ -253,5 +253,18 @@ enum { >>> >>> #define EROFS_NAME_LEN 255 >>> >>> +/* check the EROFS on-disk layout strictly at compile time */ >>> +static inline void erofs_check_ondisk_layout(void) >>> +{ >>> + BUILD_BUG_ON(sizeof(struct erofs_super_block) != 128); >>> + BUILD_BUG_ON(sizeof(struct erofs_inode_v1) != 32); >>> + BUILD_BUG_ON(sizeof(struct erofs_inode_v2) != 64); >>> + BUILD_BUG_ON(sizeof(struct erofs_xattr_ibody_header) != 12); >>> + BUILD_BUG_ON(sizeof(struct erofs_xattr_entry) != 4); >>> + BUILD_BUG_ON(sizeof(struct erofs_extent_header) != 16); >>> + BUILD_BUG_ON(sizeof(struct erofs_decompressed_index_vle) != 8); >>> + BUILD_BUG_ON(sizeof(struct erofs_dirent) != 12); >> >> We'd better define some macros include above constants. >> >> Other parts look good to me. >> >> Thanks, >> > > I was also thinking of wrapping in macros, but I thought over again, > it seems not too necessary because: > > 1) the above code is already well straight-forward to view and change if needed... > > BUILD_BUG_ON(sizeof(struct erofs_super_block) != 128); > <==> #define EROFS_SUPER_BLOCK_ONDISK_SIZE 128 > > EROFS_SUPER_BLOCK_ONDISK_SIZE is not really needed since > we could use sizeof(struct erofs_super_block) directly in code. > > 2) Besides, I also observed the other file systems, such as xfs, sysv, minix, and ubifs, eg. > > fs/jffs2/super.c: > 366 /* Paranoia checks for on-medium structures. If we ask GCC > 367 to pack them with __attribute__((packed)) then it _also_ > 368 assumes that they're not aligned -- so it emits crappy > 369 code on some architectures. Ideally we want an attribute > 370 which means just 'no padding', without the alignment > 371 thing. But GCC doesn't have that -- we have to just > 372 hope the structs are the right sizes, instead. */ > 373 BUILD_BUG_ON(sizeof(struct jffs2_unknown_node) != 12); > 374 BUILD_BUG_ON(sizeof(struct jffs2_raw_dirent) != 40); > 375 BUILD_BUG_ON(sizeof(struct jffs2_raw_inode) != 68); > 376 BUILD_BUG_ON(sizeof(struct jffs2_raw_summary) != 32); > > fs/minix/inode.c: > 174 BUILD_BUG_ON(32 != sizeof (struct minix_inode)); > 175 BUILD_BUG_ON(64 != sizeof(struct minix2_inode)); > > fs/ubifs/super.c: > 359 BUILD_BUG_ON(1024 != sizeof (struct xenix_super_block)); > 360 BUILD_BUG_ON(512 != sizeof (struct sysv4_super_block)); > 361 BUILD_BUG_ON(512 != sizeof (struct sysv2_super_block)); > 362 BUILD_BUG_ON(500 != sizeof (struct coh_super_block)); > 363 BUILD_BUG_ON(64 != sizeof (struct sysv_inode)); > > fs/ubifs/super.c > 2211 BUILD_BUG_ON(UBIFS_MAX_NODE_SZ & 7); > 2212 BUILD_BUG_ON(MIN_WRITE_SZ & 7); > 2213 > 2214 /* Check min. node size */ > 2215 BUILD_BUG_ON(UBIFS_INO_NODE_SZ < MIN_WRITE_SZ); > 2216 BUILD_BUG_ON(UBIFS_DENT_NODE_SZ < MIN_WRITE_SZ); > 2217 BUILD_BUG_ON(UBIFS_XENT_NODE_SZ < MIN_WRITE_SZ); > 2218 BUILD_BUG_ON(UBIFS_TRUN_NODE_SZ < MIN_WRITE_SZ); > 2219 > 2220 BUILD_BUG_ON(UBIFS_MAX_DENT_NODE_SZ > UBIFS_MAX_NODE_SZ); > 2221 BUILD_BUG_ON(UBIFS_MAX_XENT_NODE_SZ > UBIFS_MAX_NODE_SZ); > 2222 BUILD_BUG_ON(UBIFS_MAX_DATA_NODE_SZ > UBIFS_MAX_NODE_SZ); > 2223 BUILD_BUG_ON(UBIFS_MAX_INO_NODE_SZ > UBIFS_MAX_NODE_SZ); > 2224 > 2225 /* Defined node sizes */ > 2226 BUILD_BUG_ON(UBIFS_SB_NODE_SZ != 4096); > 2227 BUILD_BUG_ON(UBIFS_MST_NODE_SZ != 512); > 2228 BUILD_BUG_ON(UBIFS_INO_NODE_SZ != 160); > 2229 BUILD_BUG_ON(UBIFS_REF_NODE_SZ != 64); > > how do you think? :) If you think it is necessary, I can change into marcos immediately~ Alright, it's trivial, let's keep the code as original state. :) Reviewed-by: Chao Yu <yuchao0 at huawei.com> Thanks, > > Thanks, > Gao Xiang > >>> +} >>> + >>> #endif >>> >>> diff --git a/fs/erofs/super.c b/fs/erofs/super.c >>> index a1826b9..d104d88 100644 >>> --- a/fs/erofs/super.c >>> +++ b/fs/erofs/super.c >>> @@ -37,6 +37,12 @@ static int erofs_init_inode_cache(void) >>> return erofs_inode_cachep != NULL ? 0 : -ENOMEM; >>> } >>> >>> +static void erofs_exit_inode_cache(void) >>> +{ >>> + BUG_ON(erofs_inode_cachep == NULL); >>> + kmem_cache_destroy(erofs_inode_cachep); >>> +} >>> + >>> static struct inode *alloc_inode(struct super_block *sb) >>> { >>> struct erofs_vnode *vi = >>> @@ -398,22 +404,30 @@ int __init erofs_module_init(void) >>> { >>> int err; >>> >>> + erofs_check_ondisk_layout(); >>> infoln("Initializing erofs " EROFS_VERSION); >>> >>> err = erofs_init_inode_cache(); >>> - if (!err) { >>> - err = register_filesystem(&erofs_fs_type); >>> - if (!err) { >>> - infoln("Successfully to initialize erofs"); >>> - return 0; >>> - } >>> - } >>> + if (err) >>> + goto icache_err; >>> + >>> + err = register_filesystem(&erofs_fs_type); >>> + if (err) >>> + goto fs_err; >>> + >>> + infoln("Successfully to initialize erofs"); >>> + return 0; >>> + >>> +fs_err: >>> + erofs_exit_inode_cache(); >>> +icache_err: >>> return err; >>> } >>> >>> void __exit erofs_module_exit(void) >>> { >>> unregister_filesystem(&erofs_fs_type); >>> + erofs_exit_inode_cache(); >>> infoln("Successfully finalize erofs"); >>> } >>> >>> > > . > ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] erofs: fix erofs_module_init & exit 2018-07-06 1:30 ` Chao Yu @ 2018-07-06 2:09 ` Gao Xiang 2018-07-06 11:34 ` Gao Xiang 0 siblings, 1 reply; 8+ messages in thread From: Gao Xiang @ 2018-07-06 2:09 UTC (permalink / raw) Hi Chao, On 2018/7/6 9:30, Chao Yu wrote: > Hi Xiang, > > On 2018/7/6 0:39, Gao Xiang wrote: >> Hi Chao, >> >> On 2018/7/6 0:19, Chao Yu wrote: >>> Hi Xiang, >>> >>> On 2018/7/5 13:03, Gao Xiang wrote: >>>> This patch adds missing parts and EROFS ondisk >>>> layout check as well. >>>> >>>> Signed-off-by: Gao Xiang <gaoxiang25 at huawei.com> >>>> --- >>>> fs/erofs/erofs_fs.h | 13 +++++++++++++ >>>> fs/erofs/super.c | 28 +++++++++++++++++++++------- >>>> 2 files changed, 34 insertions(+), 7 deletions(-) >>>> >>>> diff --git a/fs/erofs/erofs_fs.h b/fs/erofs/erofs_fs.h >>>> index ed943d4..0dbf08d 100644 >>>> --- a/fs/erofs/erofs_fs.h >>>> +++ b/fs/erofs/erofs_fs.h >>>> @@ -253,5 +253,18 @@ enum { >>>> >>>> #define EROFS_NAME_LEN 255 >>>> >>>> +/* check the EROFS on-disk layout strictly at compile time */ >>>> +static inline void erofs_check_ondisk_layout(void) >>>> +{ >>>> + BUILD_BUG_ON(sizeof(struct erofs_super_block) != 128); >>>> + BUILD_BUG_ON(sizeof(struct erofs_inode_v1) != 32); >>>> + BUILD_BUG_ON(sizeof(struct erofs_inode_v2) != 64); >>>> + BUILD_BUG_ON(sizeof(struct erofs_xattr_ibody_header) != 12); >>>> + BUILD_BUG_ON(sizeof(struct erofs_xattr_entry) != 4); >>>> + BUILD_BUG_ON(sizeof(struct erofs_extent_header) != 16); >>>> + BUILD_BUG_ON(sizeof(struct erofs_decompressed_index_vle) != 8); >>>> + BUILD_BUG_ON(sizeof(struct erofs_dirent) != 12); >>> We'd better define some macros include above constants. >>> >>> Other parts look good to me. >>> >>> Thanks, >>> >> I was also thinking of wrapping in macros, but I thought over again, >> it seems not too necessary because: >> >> 1) the above code is already well straight-forward to view and change if needed... >> >> BUILD_BUG_ON(sizeof(struct erofs_super_block) != 128); >> <==> #define EROFS_SUPER_BLOCK_ONDISK_SIZE 128 >> >> EROFS_SUPER_BLOCK_ONDISK_SIZE is not really needed since >> we could use sizeof(struct erofs_super_block) directly in code. >> >> 2) Besides, I also observed the other file systems, such as xfs, sysv, minix, and ubifs, eg. >> >> fs/jffs2/super.c: >> 366 /* Paranoia checks for on-medium structures. If we ask GCC >> 367 to pack them with __attribute__((packed)) then it _also_ >> 368 assumes that they're not aligned -- so it emits crappy >> 369 code on some architectures. Ideally we want an attribute >> 370 which means just 'no padding', without the alignment >> 371 thing. But GCC doesn't have that -- we have to just >> 372 hope the structs are the right sizes, instead. */ >> 373 BUILD_BUG_ON(sizeof(struct jffs2_unknown_node) != 12); >> 374 BUILD_BUG_ON(sizeof(struct jffs2_raw_dirent) != 40); >> 375 BUILD_BUG_ON(sizeof(struct jffs2_raw_inode) != 68); >> 376 BUILD_BUG_ON(sizeof(struct jffs2_raw_summary) != 32); >> >> fs/minix/inode.c: >> 174 BUILD_BUG_ON(32 != sizeof (struct minix_inode)); >> 175 BUILD_BUG_ON(64 != sizeof(struct minix2_inode)); >> >> fs/ubifs/super.c: >> 359 BUILD_BUG_ON(1024 != sizeof (struct xenix_super_block)); >> 360 BUILD_BUG_ON(512 != sizeof (struct sysv4_super_block)); >> 361 BUILD_BUG_ON(512 != sizeof (struct sysv2_super_block)); >> 362 BUILD_BUG_ON(500 != sizeof (struct coh_super_block)); >> 363 BUILD_BUG_ON(64 != sizeof (struct sysv_inode)); >> >> fs/ubifs/super.c >> 2211 BUILD_BUG_ON(UBIFS_MAX_NODE_SZ & 7); >> 2212 BUILD_BUG_ON(MIN_WRITE_SZ & 7); >> 2213 >> 2214 /* Check min. node size */ >> 2215 BUILD_BUG_ON(UBIFS_INO_NODE_SZ < MIN_WRITE_SZ); >> 2216 BUILD_BUG_ON(UBIFS_DENT_NODE_SZ < MIN_WRITE_SZ); >> 2217 BUILD_BUG_ON(UBIFS_XENT_NODE_SZ < MIN_WRITE_SZ); >> 2218 BUILD_BUG_ON(UBIFS_TRUN_NODE_SZ < MIN_WRITE_SZ); >> 2219 >> 2220 BUILD_BUG_ON(UBIFS_MAX_DENT_NODE_SZ > UBIFS_MAX_NODE_SZ); >> 2221 BUILD_BUG_ON(UBIFS_MAX_XENT_NODE_SZ > UBIFS_MAX_NODE_SZ); >> 2222 BUILD_BUG_ON(UBIFS_MAX_DATA_NODE_SZ > UBIFS_MAX_NODE_SZ); >> 2223 BUILD_BUG_ON(UBIFS_MAX_INO_NODE_SZ > UBIFS_MAX_NODE_SZ); >> 2224 >> 2225 /* Defined node sizes */ >> 2226 BUILD_BUG_ON(UBIFS_SB_NODE_SZ != 4096); >> 2227 BUILD_BUG_ON(UBIFS_MST_NODE_SZ != 512); >> 2228 BUILD_BUG_ON(UBIFS_INO_NODE_SZ != 160); >> 2229 BUILD_BUG_ON(UBIFS_REF_NODE_SZ != 64); >> >> how do you think? :) If you think it is necessary, I can change into marcos immediately~ > Alright, it's trivial, let's keep the code as original state. :) > > Reviewed-by: Chao Yu <yuchao0 at huawei.com> > :), Thanks for the review~ It seems that 'erofs_check_ondisk_layout_definations' is more proper than 'erofs_check_ondisk_layout', which seems used for the runtime sanity check.. Let me send the next version :) Thanks, Gao Xiang > Thanks, > >> Thanks, >> Gao Xiang >> >>>> +} >>>> + >>>> #endif >>>> >>>> diff --git a/fs/erofs/super.c b/fs/erofs/super.c >>>> index a1826b9..d104d88 100644 >>>> --- a/fs/erofs/super.c >>>> +++ b/fs/erofs/super.c >>>> @@ -37,6 +37,12 @@ static int erofs_init_inode_cache(void) >>>> return erofs_inode_cachep != NULL ? 0 : -ENOMEM; >>>> } >>>> >>>> +static void erofs_exit_inode_cache(void) >>>> +{ >>>> + BUG_ON(erofs_inode_cachep == NULL); >>>> + kmem_cache_destroy(erofs_inode_cachep); >>>> +} >>>> + >>>> static struct inode *alloc_inode(struct super_block *sb) >>>> { >>>> struct erofs_vnode *vi = >>>> @@ -398,22 +404,30 @@ int __init erofs_module_init(void) >>>> { >>>> int err; >>>> >>>> + erofs_check_ondisk_layout(); >>>> infoln("Initializing erofs " EROFS_VERSION); >>>> >>>> err = erofs_init_inode_cache(); >>>> - if (!err) { >>>> - err = register_filesystem(&erofs_fs_type); >>>> - if (!err) { >>>> - infoln("Successfully to initialize erofs"); >>>> - return 0; >>>> - } >>>> - } >>>> + if (err) >>>> + goto icache_err; >>>> + >>>> + err = register_filesystem(&erofs_fs_type); >>>> + if (err) >>>> + goto fs_err; >>>> + >>>> + infoln("Successfully to initialize erofs"); >>>> + return 0; >>>> + >>>> +fs_err: >>>> + erofs_exit_inode_cache(); >>>> +icache_err: >>>> return err; >>>> } >>>> >>>> void __exit erofs_module_exit(void) >>>> { >>>> unregister_filesystem(&erofs_fs_type); >>>> + erofs_exit_inode_cache(); >>>> infoln("Successfully finalize erofs"); >>>> } >>>> >>>> >> . >> ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] erofs: fix erofs_module_init & exit 2018-07-06 2:09 ` Gao Xiang @ 2018-07-06 11:34 ` Gao Xiang 2018-07-06 11:56 ` Chao Yu 0 siblings, 1 reply; 8+ messages in thread From: Gao Xiang @ 2018-07-06 11:34 UTC (permalink / raw) Hi Chao, On 2018/7/6 10:09, Gao Xiang wrote: >> Alright, it's trivial, let's keep the code as original state. :) >> >> Reviewed-by: Chao Yu <yuchao0 at huawei.com> >> > :), Thanks for the review~ > > It seems that 'erofs_check_ondisk_layout_definations' is more proper than 'erofs_check_ondisk_layout', > which seems used for the runtime sanity check.. > > Let me send the next version :) > > > Thanks, > Gao Xiang > Too many chores today... Sorry for late response.. Could you check the v2 patch and merge it? :) Thanks, Gao Xiang ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] erofs: fix erofs_module_init & exit 2018-07-06 11:34 ` Gao Xiang @ 2018-07-06 11:56 ` Chao Yu 0 siblings, 0 replies; 8+ messages in thread From: Chao Yu @ 2018-07-06 11:56 UTC (permalink / raw) Hi Xiang, On 2018/7/6 19:34, Gao Xiang wrote: > Hi Chao, > > On 2018/7/6 10:09, Gao Xiang wrote: >>> Alright, it's trivial, let's keep the code as original state. :) >>> >>> Reviewed-by: Chao Yu <yuchao0 at huawei.com> >>> >> :), Thanks for the review~ >> >> It seems that 'erofs_check_ondisk_layout_definations' is more proper than 'erofs_check_ondisk_layout', >> which seems used for the runtime sanity check.. >> >> Let me send the next version :) >> >> >> Thanks, >> Gao Xiang >> > > Too many chores today... Sorry for late response.. > Could you check the v2 patch and merge it? :) I have checked it, and updated it in erofs branch. :) Thanks, > > Thanks, > Gao Xiang > ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2] erofs: fix erofs_module_init & exit 2018-07-05 5:03 [PATCH] erofs: fix erofs_module_init & exit Gao Xiang 2018-07-05 16:19 ` Chao Yu @ 2018-07-06 11:30 ` Gao Xiang 1 sibling, 0 replies; 8+ messages in thread From: Gao Xiang @ 2018-07-06 11:30 UTC (permalink / raw) This patch adds missing parts and EROFS ondisk layout check as well. Reviewed-by: Chao Yu <yuchao0 at huawei.com> Signed-off-by: Gao Xiang <gaoxiang25 at huawei.com> --- change log v2: - erofs_check_ondisk_layout => erofs_check_ondisk_layout_definitions fs/erofs/erofs_fs.h | 13 +++++++++++++ fs/erofs/super.c | 28 +++++++++++++++++++++------- 2 files changed, 34 insertions(+), 7 deletions(-) diff --git a/fs/erofs/erofs_fs.h b/fs/erofs/erofs_fs.h index ed943d4..11300414 100644 --- a/fs/erofs/erofs_fs.h +++ b/fs/erofs/erofs_fs.h @@ -253,5 +253,18 @@ enum { #define EROFS_NAME_LEN 255 +/* check the EROFS on-disk layout strictly at compile time */ +static inline void erofs_check_ondisk_layout_definitions(void) +{ + BUILD_BUG_ON(sizeof(struct erofs_super_block) != 128); + BUILD_BUG_ON(sizeof(struct erofs_inode_v1) != 32); + BUILD_BUG_ON(sizeof(struct erofs_inode_v2) != 64); + BUILD_BUG_ON(sizeof(struct erofs_xattr_ibody_header) != 12); + BUILD_BUG_ON(sizeof(struct erofs_xattr_entry) != 4); + BUILD_BUG_ON(sizeof(struct erofs_extent_header) != 16); + BUILD_BUG_ON(sizeof(struct erofs_decompressed_index_vle) != 8); + BUILD_BUG_ON(sizeof(struct erofs_dirent) != 12); +} + #endif diff --git a/fs/erofs/super.c b/fs/erofs/super.c index a1826b9..0dcf9c7 100644 --- a/fs/erofs/super.c +++ b/fs/erofs/super.c @@ -37,6 +37,12 @@ static int erofs_init_inode_cache(void) return erofs_inode_cachep != NULL ? 0 : -ENOMEM; } +static void erofs_exit_inode_cache(void) +{ + BUG_ON(erofs_inode_cachep == NULL); + kmem_cache_destroy(erofs_inode_cachep); +} + static struct inode *alloc_inode(struct super_block *sb) { struct erofs_vnode *vi = @@ -398,22 +404,30 @@ int __init erofs_module_init(void) { int err; + erofs_check_ondisk_layout_definitions(); infoln("Initializing erofs " EROFS_VERSION); err = erofs_init_inode_cache(); - if (!err) { - err = register_filesystem(&erofs_fs_type); - if (!err) { - infoln("Successfully to initialize erofs"); - return 0; - } - } + if (err) + goto icache_err; + + err = register_filesystem(&erofs_fs_type); + if (err) + goto fs_err; + + infoln("Successfully to initialize erofs"); + return 0; + +fs_err: + erofs_exit_inode_cache(); +icache_err: return err; } void __exit erofs_module_exit(void) { unregister_filesystem(&erofs_fs_type); + erofs_exit_inode_cache(); infoln("Successfully finalize erofs"); } -- 1.9.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
end of thread, other threads:[~2018-07-06 11:56 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-07-05 5:03 [PATCH] erofs: fix erofs_module_init & exit Gao Xiang 2018-07-05 16:19 ` Chao Yu 2018-07-05 16:39 ` Gao Xiang 2018-07-06 1:30 ` Chao Yu 2018-07-06 2:09 ` Gao Xiang 2018-07-06 11:34 ` Gao Xiang 2018-07-06 11:56 ` Chao Yu 2018-07-06 11:30 ` [PATCH v2] " Gao Xiang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox