Re: [PATCH 1/2] Fix sign extension of bitmap_offset in super1.c

[NeilBrown <neilb@suse.de>]

[-- Attachment #1: Type: text/plain, Size: 2209 bytes --]

On Thu, 26 Apr 2012 17:21:36 +0200 Jes Sorensen <Jes.Sorensen@redhat.com>
wrote:

> On 04/26/12 17:18, Doug Ledford wrote:
> > On 04/26/2012 11:12 AM, Jes.Sorensen@redhat.com wrote:
> >> > From: Jes Sorensen <Jes.Sorensen@redhat.com>
> >> > 
> >> > fbdef49811c9e2b54e2064d9af68cfffa77c6e77 incorrectly tried to fix sign
> >> > extension of the bitmap offset. However mdinfo->bitmap_offset is a u32
> >> > and needs to be converted to a 32 bit signed integer before the sign
> >> > extension.
> >> > 
> >> > Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
> > I was scratching my head over this patch, saying to myself "But won't
> > that cause us to truncate large values of bitmap_offset?"  And it will,
> > but I see your point now, that's *exactly* the problem if we don't do
> > the sign conversion before the extension, the actual bitmap_offset
> > should really be signed in order to support negative offsets, but since
> > it isn't, when we save a negative offset into bitmap_offset it appears
> > as a really large positive offset, and then when we sign extend to long,
> > it keeps the large size positive offset instead of picking up the
> > negative offset.  Gotcha.  So, I see why this works, but do you think it
> > should be fixed this way, or by converting bitmap_offset to type int32
> > instead of uint32?
> 
> Heh, I have to admit I cheated too and asked Richard Henderson for help
> as I couldn't figure out why the sign conversion failed. Otherwise I
> would probably still have been scratching my head over it :)
> 
> I noticed other parts of the code already handled it this way, so my fix
> is consistent with that, but we could do both. Neil?

The reason that "bitmap_offset" in 'mdp_superblock_1' is '__u32' is simply
that there is no '__s32'.
I wouldn't be against changing it, but I think you've concluded that it keeps
the byte swapping simpler if we don't.

But then I read recently that Rob Pike thinks byte swapping is always wrong

http://commandcenter.blogspot.com/2012/04/byte-order-fallacy.html

so maybe we shouldn't be todo that....

I'll just take your patches as they are I think - they look good.

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]