xfs
[Top] [All Lists]

Re: [PATCH 2/3] xfs_repair: Document & robustify blkmap_next_off()

To: Eric Sandeen <sandeen@xxxxxxxxxx>
Subject: Re: [PATCH 2/3] xfs_repair: Document & robustify blkmap_next_off()
From: Ben Myers <bpm@xxxxxxx>
Date: Thu, 31 May 2012 16:27:14 -0500
Cc: xfs-oss <xfs@xxxxxxxxxxx>
In-reply-to: <4FBABD6A.6070800@xxxxxxxxxx>
References: <4FBABCAB.20300@xxxxxxxxxx> <4FBABD6A.6070800@xxxxxxxxxx>
User-agent: Mutt/1.5.20 (2009-06-14)
On Mon, May 21, 2012 at 05:10:50PM -0500, Eric Sandeen wrote:
> blkmap_next_off() was cryptic (to me), so document what it does.
> Also catch cases when the passed in extent index 't' is beyond
> the number of extents in the blkmap, so that:
> 
>         ext = blkmap->exts + *t;
> 
> doesn't walk off the end of the array into garbage.
> 
> Signed-off-by: Eric Sandeen <sandeen@xxxxxxxxxx>
> ---
> 
> diff --git a/repair/bmap.c b/repair/bmap.c
> index c43ca7f..85d66dc 100644
> --- a/repair/bmap.c
> +++ b/repair/bmap.c
> @@ -206,8 +206,25 @@ blkmap_last_off(
>       return ext->startoff + ext->blockcount;
>  }
>  
> -/*
> - * Return the next offset in a block map.
> +/**
> + * blkmap_next_off - Return next logical block offset in a block map.
> + * @blkmap:  blockmap to use
> + * @o:               current file logical block number
> + * @t:               current extent index into blockmap (in/out)
> + *
> + * Given a logical block offset in a file, return the next mapped logical 
> offset
> + * The map index "t" tracks the current extent number in the block map, and
> + * is updated automatically if the returned offset resides within the next
> + * mapped extent.
> + *
> + * If the blockmap contains no extents, or no more logical offsets are 
> mapped,
> + * or the extent index exceeds the number of extents in the map,
> + * return NULLDFILOFF.
> + *
> + * If offset o is beyond extent index t, the first offset in the next extent
> + * after extent t will be returned.
> + *
> + * Intended to be called starting with offset 0, index 0, and iterated.
>   */
>  xfs_dfiloff_t
>  blkmap_next_off(
> @@ -223,10 +240,12 @@ blkmap_next_off(
>               *t = 0;
>               return blkmap->exts[0].startoff;
>       }
> +     if (*t >= blkmap->nexts)
> +             return NULLDFILOFF;
>       ext = blkmap->exts + *t;
>       if (o < ext->startoff + ext->blockcount - 1)
>               return o + 1;
> -     if (*t >= blkmap->nexts - 1)
> +     if (*t == blkmap->nexts - 1)
>               return NULLDFILOFF;
>       (*t)++;
>       return ext[1].startoff;

Ok, so you have 50 extents, and when *t == 49 you don't want to go looking at
ext[50].startoff.  Looks good to me.

Reviewed-by: Ben Myers <bpm@xxxxxxx>

<Prev in Thread] Current Thread [Next in Thread>