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

[Doug Ledford <dledford@redhat.com>]

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

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?

> ---
>  super1.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/super1.c b/super1.c
> index 36369d8..be77c33 100644
> --- a/super1.c
> +++ b/super1.c
> @@ -620,7 +620,7 @@ static void getinfo_super1(struct supertype *st, struct mdinfo *info, char *map)
>  	info->data_offset = __le64_to_cpu(sb->data_offset);
>  	info->component_size = __le64_to_cpu(sb->size);
>  	if (sb->feature_map & __le32_to_cpu(MD_FEATURE_BITMAP_OFFSET))
> -		info->bitmap_offset = (long)__le32_to_cpu(sb->bitmap_offset);
> +		info->bitmap_offset = (int32_t)__le32_to_cpu(sb->bitmap_offset);
>  
>  	info->disk.major = 0;
>  	info->disk.minor = 0;
> @@ -1651,7 +1651,7 @@ add_internal_bitmap1(struct supertype *st,
>  		offset = -room;
>  	}
>  
> -	sb->bitmap_offset = (long)__cpu_to_le32(offset);
> +	sb->bitmap_offset = (int32_t)__cpu_to_le32(offset);
>  
>  	sb->feature_map = __cpu_to_le32(__le32_to_cpu(sb->feature_map)
>  					| MD_FEATURE_BITMAP_OFFSET);


-- 
Doug Ledford <dledford@redhat.com>
              GPG KeyID: 0E572FDD
	      http://people.redhat.com/dledford



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 900 bytes --]