[PATCH] gfs2 endianness bug: be16 assigned to be32 field

[PATCH] gfs2 endianness bug: be16 assigned to be32 field

[Al Viro]

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/gfs2/dir.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/fs/gfs2/dir.c b/fs/gfs2/dir.c
index 459498c..d43caf0 100644
--- a/fs/gfs2/dir.c
+++ b/fs/gfs2/dir.c
@@ -815,7 +815,7 @@ static struct gfs2_leaf *new_leaf(struct
 	leaf = (struct gfs2_leaf *)bh->b_data;
 	leaf->lf_depth = cpu_to_be16(depth);
 	leaf->lf_entries = 0;
-	leaf->lf_dirent_format = cpu_to_be16(GFS2_FORMAT_DE);
+	leaf->lf_dirent_format = cpu_to_be32(GFS2_FORMAT_DE);
 	leaf->lf_next = 0;
 	memset(leaf->lf_reserved, 0, sizeof(leaf->lf_reserved));
 	dent = (struct gfs2_dirent *)(leaf+1);
-- 
1.4.2.GIT

Re: [PATCH] gfs2 endianness bug: be16 assigned to be32 field

[Linus Torvalds]

On Sat, 14 Oct 2006, Al Viro wrote:
>
> -	leaf->lf_dirent_format = cpu_to_be16(GFS2_FORMAT_DE);
> +	leaf->lf_dirent_format = cpu_to_be32(GFS2_FORMAT_DE);

Hmm. Doesn't this change the on-disk format on a LE machine (eg x86)?

In other words, this change makes me nervous. A quick grep seems to 
indicate that nothing actually _uses_ this field, so maybe we don't really 
care, but I think we should double-check that this is what the GFS2 people 
really want.

If we don't want to change the format on a LE machine, then maybe the 
gfs2_leaf structure should be changed to be

	..
	__be16 lf_dirent_format;
	__be16 lf_unused;
	..

which should keep the bits in the same position on LE.

Regardless, the old code was clearly wrong, since it gives different 
on-disk format for a big-endian and a little-endian machine. Al's fix is 
proper, but perhaps people would prefer something that breaks the BE 
format rather than the LE format. Hmm?

Steven?

		Linus

Re: [PATCH] gfs2 endianness bug: be16 assigned to be32 field

[Steven Whitehouse]

Hi,

On Sat, 2006-10-14 at 11:32 -0700, Linus Torvalds wrote:
> 
> On Sat, 14 Oct 2006, Al Viro wrote:
> >
> > -	leaf->lf_dirent_format = cpu_to_be16(GFS2_FORMAT_DE);
> > +	leaf->lf_dirent_format = cpu_to_be32(GFS2_FORMAT_DE);
> 
> Hmm. Doesn't this change the on-disk format on a LE machine (eg x86)?
> 
> In other words, this change makes me nervous. A quick grep seems to 
> indicate that nothing actually _uses_ this field, so maybe we don't really 
> care, but I think we should double-check that this is what the GFS2 people 
> really want.
> 
> If we don't want to change the format on a LE machine, then maybe the 
> gfs2_leaf structure should be changed to be
> 
> 	..
> 	__be16 lf_dirent_format;
> 	__be16 lf_unused;
> 	..
> 
> which should keep the bits in the same position on LE.
> 
Its a tricky question, but I think on balance that Al's proposed fix is
the right one...

> Regardless, the old code was clearly wrong, since it gives different 
> on-disk format for a big-endian and a little-endian machine. Al's fix is 
> proper, but perhaps people would prefer something that breaks the BE 
> format rather than the LE format. Hmm?
> 
> Steven?
> 
> 		Linus

As you say, this field isn't actually used by anything. Its whole reason
for existing is to be forwards compatible with GFS1. So I'd be inclined
to accept the patch as proposed in order that as few filesystems as
possible will have the incorrect format. We'll make a note of this
problem and ensure that fsck can fix up the error and if we need to use
this field in future, then we'll take into account that there may be
some filesystems with the incorrect format number.

I think its fairly unlikely that we will need to use it in the near
future though... Its original purpose was to allow future changes in the
metadata format, but this is also allowed for by the mh_format field in
the common metadata header, so I'm not entirely sure why a second format
field was included in this structure (the directory leaf header).

Steve.

Re: [PATCH] gfs2 endianness bug: be16 assigned to be32 field

[Steven Whitehouse]

Hi,

I the light of my earlier reply to Linus, I've applied this to the GFS2
git tree. Thanks,

Steve.
 
On Sat, 2006-10-14 at 16:49 +0100, Al Viro wrote:
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---
>  fs/gfs2/dir.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/fs/gfs2/dir.c b/fs/gfs2/dir.c
> index 459498c..d43caf0 100644
> --- a/fs/gfs2/dir.c
> +++ b/fs/gfs2/dir.c
> @@ -815,7 +815,7 @@ static struct gfs2_leaf *new_leaf(struct
>  	leaf = (struct gfs2_leaf *)bh->b_data;
>  	leaf->lf_depth = cpu_to_be16(depth);
>  	leaf->lf_entries = 0;
> -	leaf->lf_dirent_format = cpu_to_be16(GFS2_FORMAT_DE);
> +	leaf->lf_dirent_format = cpu_to_be32(GFS2_FORMAT_DE);
>  	leaf->lf_next = 0;
>  	memset(leaf->lf_reserved, 0, sizeof(leaf->lf_reserved));
>  	dent = (struct gfs2_dirent *)(leaf+1);