[PATCH] mkfs: check sizes of important structs at build time

[PATCH] mkfs: check sizes of important structs at build time

[Hitoshi Mitake]

Current nilfs_check_ondisk_sizes() checks sizes of important structs
at run time. The checking should be done at build time. This patch
adds a new macro, BUILD_BUG_ON(), for this purpose. It is similar to
static_assert() of C++11. If an argument is true, the macro causes a
bulid error.

Below is an example of BUILD_BUG_ON(). When the checked conditions are
true like below:

/* intentional change for testing BUILD_BUG_ON() */

static __attribute__((used)) void nilfs_check_ondisk_sizes(void)
{
	BUILD_BUG_ON(sizeof(struct nilfs_inode) > NILFS_MIN_BLOCKSIZE);
...

build process of mkfs.o causes errors like this:

gcc -std=gnu99 -DHAVE_CONFIG_H -I. -I../..  -I../../include  -Wall -g -O2 -MT mkfs.o -MD -MP -MF .deps/mkfs.Tpo -c -o mkfs.o mkfs.c
mkfs.c: In function 'nilfs_check_ondisk_sizes':
mkfs.c:429:2: error: negative width in bit-field '<anonymous>'
mkfs.c:430:2: error: negative width in bit-field '<anonymous>'
mkfs.c:431:2: error: negative width in bit-field '<anonymous>'
mkfs.c:432:2: error: negative width in bit-field '<anonymous>'
mkfs.c:433:2: error: negative width in bit-field '<anonymous>'
mkfs.c:434:2: error: negative width in bit-field '<anonymous>'
mkfs.c:435:2: error: negative width in bit-field '<anonymous>'

Signed-off-by: Hitoshi Mitake <mitake.hitoshi-Zyj7fXuS5i5L9jVzuh4AOg@public.gmane.org>
---
 sbin/mkfs/mkfs.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/sbin/mkfs/mkfs.c b/sbin/mkfs/mkfs.c
index 4e153ce..8eb00bf 100644
--- a/sbin/mkfs/mkfs.c
+++ b/sbin/mkfs/mkfs.c
@@ -85,6 +85,9 @@
 typedef __u64  blocknr_t;
 
 #define BUG_ON(x)	   assert(!(x))
+/* Force a compilation error if the condition is true */
+#define BUILD_BUG_ON(condition) ((void)sizeof(struct { int: -!!(condition); }))
+
 #define ROUNDUP_DIV(n, m)	(((n) - 1) / (m) + 1)
 #define max_t(type, x, y) \
 	({ type __x = (x); type __y = (y); __x > __y ? __x : __y; })
@@ -417,16 +420,15 @@ static unsigned count_dat_blocks(unsigned nr_dat_entries)
 	return nblocks;
 }
 
-static void nilfs_check_ondisk_sizes(void)
+static __attribute__((used)) void nilfs_check_ondisk_sizes(void)
 {
-	if (sizeof(struct nilfs_inode) > NILFS_MIN_BLOCKSIZE ||
-	    sizeof(struct nilfs_sufile_header) > NILFS_MIN_BLOCKSIZE ||
-	    sizeof(struct nilfs_segment_usage) > NILFS_MIN_BLOCKSIZE ||
-	    sizeof(struct nilfs_cpfile_header) > NILFS_MIN_BLOCKSIZE ||
-	    sizeof(struct nilfs_checkpoint) > NILFS_MIN_BLOCKSIZE ||
-	    sizeof(struct nilfs_dat_entry) > NILFS_MIN_BLOCKSIZE ||
-	    sizeof(struct nilfs_super_root) > NILFS_MIN_BLOCKSIZE)
-		perr("Internal error: too large on-disk structure");
+	BUILD_BUG_ON(sizeof(struct nilfs_inode) > NILFS_MIN_BLOCKSIZE);
+	BUILD_BUG_ON(sizeof(struct nilfs_sufile_header) > NILFS_MIN_BLOCKSIZE);
+	BUILD_BUG_ON(sizeof(struct nilfs_segment_usage) > NILFS_MIN_BLOCKSIZE);
+	BUILD_BUG_ON(sizeof(struct nilfs_cpfile_header) > NILFS_MIN_BLOCKSIZE);
+	BUILD_BUG_ON(sizeof(struct nilfs_checkpoint) > NILFS_MIN_BLOCKSIZE);
+	BUILD_BUG_ON(sizeof(struct nilfs_dat_entry) > NILFS_MIN_BLOCKSIZE);
+	BUILD_BUG_ON(sizeof(struct nilfs_super_root) > NILFS_MIN_BLOCKSIZE);
 }
 
 static unsigned long
@@ -523,8 +525,6 @@ static void init_disk_layout(struct nilfs_disk_info *di, int fd,
 		     "or shorten segments with -B option.", dev_size,
 		     (unsigned long long)segment_size * min_nsegments);
 	di->nseginfo = 0;
-
-	nilfs_check_ondisk_sizes();
 }
 
 static struct nilfs_segment_info *new_segment(struct nilfs_disk_info *di)
-- 
1.8.1.2

--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mkfs: check sizes of important structs at build time

[Vyacheslav Dubeyko]

On Jan 4, 2014, at 4:29 PM, Hitoshi Mitake wrote:

> Current nilfs_check_ondisk_sizes() checks sizes of important structs
> at run time. The checking should be done at build time. This patch
> adds a new macro, BUILD_BUG_ON(), for this purpose. It is similar to
> static_assert() of C++11. If an argument is true, the macro causes a
> bulid error.
> 
> Below is an example of BUILD_BUG_ON(). When the checked conditions are
> true like below:
> 
> /* intentional change for testing BUILD_BUG_ON() */
> 
> static __attribute__((used)) void nilfs_check_ondisk_sizes(void)
> {
> 	BUILD_BUG_ON(sizeof(struct nilfs_inode) > NILFS_MIN_BLOCKSIZE);

So, why do we need to have function for the case of checking on compilation
phase?

I suppose that we need to have some run-time check anyway. Your approach
is correct for the current state of the code. But I feel a necessity in run-time check
anyway. Maybe it looks like a paranoia. :) Maybe it needs to extend checking
in this place.

Thanks,
Vyacheslav Dubeyko.

--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mkfs: check sizes of important structs at build time

[Hitoshi Mitake]

On Sat, Jan 4, 2014 at 11:52 PM, Vyacheslav Dubeyko <slava-yeENwD64cLxBDgjK7y7TUQ@public.gmane.org> wrote:
>
> On Jan 4, 2014, at 4:29 PM, Hitoshi Mitake wrote:
>
>> Current nilfs_check_ondisk_sizes() checks sizes of important structs
>> at run time. The checking should be done at build time. This patch
>> adds a new macro, BUILD_BUG_ON(), for this purpose. It is similar to
>> static_assert() of C++11. If an argument is true, the macro causes a
>> bulid error.
>>
>> Below is an example of BUILD_BUG_ON(). When the checked conditions are
>> true like below:
>>
>> /* intentional change for testing BUILD_BUG_ON() */
>>
>> static __attribute__((used)) void nilfs_check_ondisk_sizes(void)
>> {
>>       BUILD_BUG_ON(sizeof(struct nilfs_inode) > NILFS_MIN_BLOCKSIZE);
>
> So, why do we need to have function for the case of checking on compilation
> phase?

Just for excluding the checking from other part of code and improve readability.

>
> I suppose that we need to have some run-time check anyway. Your approach
> is correct for the current state of the code. But I feel a necessity in run-time check
> anyway. Maybe it looks like a paranoia. :) Maybe it needs to extend checking
> in this place.

Do you mean both of the build time check and the run time check? If
so, I agree with your opinion. I'll send v2 based on this policy.

Thanks,
Hitoshi
--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mkfs: check sizes of important structs at build time

[Vyacheslav Dubeyko]

On Jan 4, 2014, at 4:54 PM, Hitoshi Mitake wrote:

> On Sat, Jan 4, 2014 at 11:52 PM, Vyacheslav Dubeyko <slava-yeENwD64cLxBDgjK7y7TUQ@public.gmane.org> wrote:
>> 
>> On Jan 4, 2014, at 4:29 PM, Hitoshi Mitake wrote:
>> 
>>> Current nilfs_check_ondisk_sizes() checks sizes of important structs
>>> at run time. The checking should be done at build time. This patch
>>> adds a new macro, BUILD_BUG_ON(), for this purpose. It is similar to
>>> static_assert() of C++11. If an argument is true, the macro causes a
>>> bulid error.
>>> 
>>> Below is an example of BUILD_BUG_ON(). When the checked conditions are
>>> true like below:
>>> 
>>> /* intentional change for testing BUILD_BUG_ON() */
>>> 
>>> static __attribute__((used)) void nilfs_check_ondisk_sizes(void)
>>> {
>>>      BUILD_BUG_ON(sizeof(struct nilfs_inode) > NILFS_MIN_BLOCKSIZE);
>> 
>> So, why do we need to have function for the case of checking on compilation
>> phase?
> 
> Just for excluding the checking from other part of code and improve readability.
> 

I think that we can have only macro instead of the function nilfs_check_ondisk_sizes().
And this macros can be placed in the begin of main() call. I think that it will be enough
for the compilation phase check.

>> 
>> I suppose that we need to have some run-time check anyway. Your approach
>> is correct for the current state of the code. But I feel a necessity in run-time check
>> anyway. Maybe it looks like a paranoia. :) Maybe it needs to extend checking
>> in this place.
> 
> Do you mean both of the build time check and the run time check? If
> so, I agree with your opinion. I'll send v2 based on this policy.
> 

I mean that block size can be different during volume creation and maybe
it makes sense to extend a block size related checking for run-time phase.
That's all. But right now I haven't any concrete suggestions.

Thanks,
Vyacheslav Dubeyko.

--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mkfs: check sizes of important structs at build time

[Hitoshi Mitake]

At Sat, 4 Jan 2014 18:39:58 +0300,
Vyacheslav Dubeyko wrote:
> 
> 
> On Jan 4, 2014, at 4:54 PM, Hitoshi Mitake wrote:
> 
> > On Sat, Jan 4, 2014 at 11:52 PM, Vyacheslav Dubeyko <slava-yeENwD64cLxBDgjK7y7TUQ@public.gmane.org> wrote:
> >> 
> >> On Jan 4, 2014, at 4:29 PM, Hitoshi Mitake wrote:
> >> 
> >>> Current nilfs_check_ondisk_sizes() checks sizes of important structs
> >>> at run time. The checking should be done at build time. This patch
> >>> adds a new macro, BUILD_BUG_ON(), for this purpose. It is similar to
> >>> static_assert() of C++11. If an argument is true, the macro causes a
> >>> bulid error.
> >>> 
> >>> Below is an example of BUILD_BUG_ON(). When the checked conditions are
> >>> true like below:
> >>> 
> >>> /* intentional change for testing BUILD_BUG_ON() */
> >>> 
> >>> static __attribute__((used)) void nilfs_check_ondisk_sizes(void)
> >>> {
> >>>      BUILD_BUG_ON(sizeof(struct nilfs_inode) > NILFS_MIN_BLOCKSIZE);
> >> 
> >> So, why do we need to have function for the case of checking on compilation
> >> phase?
> > 
> > Just for excluding the checking from other part of code and improve readability.
> > 
> 
> I think that we can have only macro instead of the function nilfs_check_ondisk_sizes().
> And this macros can be placed in the begin of main() call. I think that it will be enough
> for the compilation phase check.

Ah, I see.

> 
> >> 
> >> I suppose that we need to have some run-time check anyway. Your approach
> >> is correct for the current state of the code. But I feel a necessity in run-time check
> >> anyway. Maybe it looks like a paranoia. :) Maybe it needs to extend checking
> >> in this place.
> > 
> > Do you mean both of the build time check and the run time check? If
> > so, I agree with your opinion. I'll send v2 based on this policy.
> > 
> 
> I mean that block size can be different during volume creation and maybe
> it makes sense to extend a block size related checking for run-time phase.
> That's all. But right now I haven't any concrete suggestions.

Currently, the check is comparison between sizes of important structs and the
minimal block size. So we don't need a function for it. I think adding the
function for runtime checking when nilfs requires it would be enough.

Thanks,
Hitoshi

--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mkfs: check sizes of important structs at build time

[Ryusuke Konishi]

On Sat,  4 Jan 2014 22:29:31 +0900, Hitoshi Mitake wrote:
> Current nilfs_check_ondisk_sizes() checks sizes of important structs
> at run time. The checking should be done at build time. This patch
> adds a new macro, BUILD_BUG_ON(), for this purpose. It is similar to
> static_assert() of C++11. If an argument is true, the macro causes a
> bulid error.
> 
> Below is an example of BUILD_BUG_ON(). When the checked conditions are
> true like below:
> 
> /* intentional change for testing BUILD_BUG_ON() */
> 
> static __attribute__((used)) void nilfs_check_ondisk_sizes(void)
> {
> 	BUILD_BUG_ON(sizeof(struct nilfs_inode) > NILFS_MIN_BLOCKSIZE);
> ...
> 
> build process of mkfs.o causes errors like this:
> 
> gcc -std=gnu99 -DHAVE_CONFIG_H -I. -I../..  -I../../include  -Wall -g -O2 -MT mkfs.o -MD -MP -MF .deps/mkfs.Tpo -c -o mkfs.o mkfs.c
> mkfs.c: In function 'nilfs_check_ondisk_sizes':
> mkfs.c:429:2: error: negative width in bit-field '<anonymous>'
> mkfs.c:430:2: error: negative width in bit-field '<anonymous>'
> mkfs.c:431:2: error: negative width in bit-field '<anonymous>'
> mkfs.c:432:2: error: negative width in bit-field '<anonymous>'
> mkfs.c:433:2: error: negative width in bit-field '<anonymous>'
> mkfs.c:434:2: error: negative width in bit-field '<anonymous>'
> mkfs.c:435:2: error: negative width in bit-field '<anonymous>'
> 
> Signed-off-by: Hitoshi Mitake <mitake.hitoshi-Zyj7fXuS5i5L9jVzuh4AOg@public.gmane.org>

This is an interesting patch.

I am inclined to apply this since the every test in the
nilfs_check_ondisk_sizes function is static.

If we will add a new check that depends on block size in a future, we
need to add a separate runtime check function as Vyacheslav wrote, but
I think you are doing right thing.

One my question is why you used bit operator.  The BUILD_BUG_ON marcro
of kernel is implemented with negative array index.
Is there any reason for this ?

Thanks,
Ryusuke Konishi
--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mkfs: check sizes of important structs at build time

[Hitoshi Mitake]

At Sun, 05 Jan 2014 01:08:43 +0900 (JST),
Ryusuke Konishi wrote:
> 
> On Sat,  4 Jan 2014 22:29:31 +0900, Hitoshi Mitake wrote:
> > Current nilfs_check_ondisk_sizes() checks sizes of important structs
> > at run time. The checking should be done at build time. This patch
> > adds a new macro, BUILD_BUG_ON(), for this purpose. It is similar to
> > static_assert() of C++11. If an argument is true, the macro causes a
> > bulid error.
> > 
> > Below is an example of BUILD_BUG_ON(). When the checked conditions are
> > true like below:
> > 
> > /* intentional change for testing BUILD_BUG_ON() */
> > 
> > static __attribute__((used)) void nilfs_check_ondisk_sizes(void)
> > {
> > 	BUILD_BUG_ON(sizeof(struct nilfs_inode) > NILFS_MIN_BLOCKSIZE);
> > ...
> > 
> > build process of mkfs.o causes errors like this:
> > 
> > gcc -std=gnu99 -DHAVE_CONFIG_H -I. -I../..  -I../../include  -Wall -g -O2 -MT mkfs.o -MD -MP -MF .deps/mkfs.Tpo -c -o mkfs.o mkfs.c
> > mkfs.c: In function 'nilfs_check_ondisk_sizes':
> > mkfs.c:429:2: error: negative width in bit-field '<anonymous>'
> > mkfs.c:430:2: error: negative width in bit-field '<anonymous>'
> > mkfs.c:431:2: error: negative width in bit-field '<anonymous>'
> > mkfs.c:432:2: error: negative width in bit-field '<anonymous>'
> > mkfs.c:433:2: error: negative width in bit-field '<anonymous>'
> > mkfs.c:434:2: error: negative width in bit-field '<anonymous>'
> > mkfs.c:435:2: error: negative width in bit-field '<anonymous>'
> > 
> > Signed-off-by: Hitoshi Mitake <mitake.hitoshi-Zyj7fXuS5i5L9jVzuh4AOg@public.gmane.org>
> 
> This is an interesting patch.
> 
> I am inclined to apply this since the every test in the
> nilfs_check_ondisk_sizes function is static.
> 
> If we will add a new check that depends on block size in a future, we
> need to add a separate runtime check function as Vyacheslav wrote, but
> I think you are doing right thing.
> 
> One my question is why you used bit operator.  The BUILD_BUG_ON marcro
> of kernel is implemented with negative array index.
> Is there any reason for this ?

If I remember correctly, I found in the BUILD_BUG_ON() in the code of Xen. I
don't have any opinion about how we implement the check. If you like the way of
array with negative length, I will employ it in v2.

BTW, there is another approach of the implementation.

#define static_assert(a, b) do { switch (0) case 0: case (a): ; } while (0)
# from xv6: http://pdos.csail.mit.edu/6.828/2012/xv6.html

This duplicated case of switch statement can be used to implement
BUILD_BUG_ON() (the b can be used as an error message).

Which one do you like?

Thanks,
Hitoshi
--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mkfs: check sizes of important structs at build time

[Ryusuke Konishi]

On Mon, 06 Jan 2014 00:22:22 +0900, Hitoshi Mitake wrote:
> At Sun, 05 Jan 2014 01:08:43 +0900 (JST),
> Ryusuke Konishi wrote:
>> 
>> On Sat,  4 Jan 2014 22:29:31 +0900, Hitoshi Mitake wrote:
>> > Current nilfs_check_ondisk_sizes() checks sizes of important structs
>> > at run time. The checking should be done at build time. This patch
>> > adds a new macro, BUILD_BUG_ON(), for this purpose. It is similar to
>> > static_assert() of C++11. If an argument is true, the macro causes a
>> > bulid error.
>> > 
>> > Below is an example of BUILD_BUG_ON(). When the checked conditions are
>> > true like below:
>> > 
>> > /* intentional change for testing BUILD_BUG_ON() */
>> > 
>> > static __attribute__((used)) void nilfs_check_ondisk_sizes(void)
>> > {
>> > 	BUILD_BUG_ON(sizeof(struct nilfs_inode) > NILFS_MIN_BLOCKSIZE);
>> > ...
>> > 
>> > build process of mkfs.o causes errors like this:
>> > 
>> > gcc -std=gnu99 -DHAVE_CONFIG_H -I. -I../..  -I../../include  -Wall -g -O2 -MT mkfs.o -MD -MP -MF .deps/mkfs.Tpo -c -o mkfs.o mkfs.c
>> > mkfs.c: In function 'nilfs_check_ondisk_sizes':
>> > mkfs.c:429:2: error: negative width in bit-field '<anonymous>'
>> > mkfs.c:430:2: error: negative width in bit-field '<anonymous>'
>> > mkfs.c:431:2: error: negative width in bit-field '<anonymous>'
>> > mkfs.c:432:2: error: negative width in bit-field '<anonymous>'
>> > mkfs.c:433:2: error: negative width in bit-field '<anonymous>'
>> > mkfs.c:434:2: error: negative width in bit-field '<anonymous>'
>> > mkfs.c:435:2: error: negative width in bit-field '<anonymous>'
>> > 
>> > Signed-off-by: Hitoshi Mitake <mitake.hitoshi-Zyj7fXuS5i5L9jVzuh4AOg@public.gmane.org>
>> 
>> This is an interesting patch.
>> 
>> I am inclined to apply this since the every test in the
>> nilfs_check_ondisk_sizes function is static.
>> 
>> If we will add a new check that depends on block size in a future, we
>> need to add a separate runtime check function as Vyacheslav wrote, but
>> I think you are doing right thing.
>> 
>> One my question is why you used bit operator.  The BUILD_BUG_ON marcro
>> of kernel is implemented with negative array index.
>> Is there any reason for this ?
> 
> If I remember correctly, I found in the BUILD_BUG_ON() in the code of Xen. I
> don't have any opinion about how we implement the check. If you like the way of
> array with negative length, I will employ it in v2.
> 
> BTW, there is another approach of the implementation.
> 
> #define static_assert(a, b) do { switch (0) case 0: case (a): ; } while (0)
> # from xv6: http://pdos.csail.mit.edu/6.828/2012/xv6.html
> 
> This duplicated case of switch statement can be used to implement
> BUILD_BUG_ON() (the b can be used as an error message).
> 
> Which one do you like?

Thanks for letting me know.

If there is no known differences in those implementations, I don't
dwell on them, all seems ok.

I applied the first patch this time.

Thank you.

Ryusuke Konishi
--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html