diff mbox series

[5/5] xfs: reinitialize rm_flags when unpacking an offset into an rmap irec

Message ID 156685618619.2853674.16603505107055424362.stgit@magnolia (mailing list archive)
State Accepted
Headers show
Series xfs: remove unnecessary parameters and returns | expand

Commit Message

Darrick J. Wong Aug. 26, 2019, 9:49 p.m. UTC 
From: Darrick J. Wong <darrick.wong@oracle.com>

In xfs_rmap_irec_offset_unpack, we should always clear the contents of
rm_flags before we begin unpacking the encoded (ondisk) offset into the
incore rm_offset and incore rm_flags fields.  Remove the open-coded
field zeroing as this encourages api misuse.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_rmap.c |    1 -
 fs/xfs/libxfs/xfs_rmap.h |    1 +
 2 files changed, 1 insertion(+), 1 deletion(-)

Comments

Dave Chinner Aug. 26, 2019, 11:26 p.m. UTC | #1 
On Mon, Aug 26, 2019 at 02:49:46PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> In xfs_rmap_irec_offset_unpack, we should always clear the contents of
> rm_flags before we begin unpacking the encoded (ondisk) offset into the
> incore rm_offset and incore rm_flags fields.  Remove the open-coded
> field zeroing as this encourages api misuse.

Makes sense. Flags are meaningless until they've been extracted.

Reviewed-by: Dave Chinner <dchinner@redhat.com>
Christoph Hellwig Aug. 29, 2019, 7:29 a.m. UTC | #2 
On Mon, Aug 26, 2019 at 02:49:46PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> In xfs_rmap_irec_offset_unpack, we should always clear the contents of
> rm_flags before we begin unpacking the encoded (ondisk) offset into the
> incore rm_offset and incore rm_flags fields.  Remove the open-coded
> field zeroing as this encourages api misuse.

This one doesn't fit the series' theme, does it? :)

> +++ b/fs/xfs/libxfs/xfs_rmap.c
> @@ -168,7 +168,6 @@ xfs_rmap_btrec_to_irec(
>  	union xfs_btree_rec	*rec,
>  	struct xfs_rmap_irec	*irec)
>  {
> -	irec->rm_flags = 0;
>  	irec->rm_startblock = be32_to_cpu(rec->rmap.rm_startblock);
>  	irec->rm_blockcount = be32_to_cpu(rec->rmap.rm_blockcount);
>  	irec->rm_owner = be64_to_cpu(rec->rmap.rm_owner);
> diff --git a/fs/xfs/libxfs/xfs_rmap.h b/fs/xfs/libxfs/xfs_rmap.h
> index 0c2c3cb73429..abe633403fd1 100644
> --- a/fs/xfs/libxfs/xfs_rmap.h
> +++ b/fs/xfs/libxfs/xfs_rmap.h
> @@ -68,6 +68,7 @@ xfs_rmap_irec_offset_unpack(
>  	if (offset & ~(XFS_RMAP_OFF_MASK | XFS_RMAP_OFF_FLAGS))
>  		return -EFSCORRUPTED;
>  	irec->rm_offset = XFS_RMAP_OFF(offset);
> +	irec->rm_flags = 0;

The change looks sensible-ish.  But why do we even have a separate
xfs_rmap_irec_offset_unpack with a single caller nd out of the
way in a header?  Wouldn't it make sense to just merge the two
functions?
Darrick J. Wong Aug. 29, 2019, 4:01 p.m. UTC | #3 
On Thu, Aug 29, 2019 at 12:29:57AM -0700, Christoph Hellwig wrote:
> On Mon, Aug 26, 2019 at 02:49:46PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > In xfs_rmap_irec_offset_unpack, we should always clear the contents of
> > rm_flags before we begin unpacking the encoded (ondisk) offset into the
> > incore rm_offset and incore rm_flags fields.  Remove the open-coded
> > field zeroing as this encourages api misuse.
> 
> This one doesn't fit the series' theme, does it? :)

Nope, there's always one cling-on patch. :/

> > +++ b/fs/xfs/libxfs/xfs_rmap.c
> > @@ -168,7 +168,6 @@ xfs_rmap_btrec_to_irec(
> >  	union xfs_btree_rec	*rec,
> >  	struct xfs_rmap_irec	*irec)
> >  {
> > -	irec->rm_flags = 0;
> >  	irec->rm_startblock = be32_to_cpu(rec->rmap.rm_startblock);
> >  	irec->rm_blockcount = be32_to_cpu(rec->rmap.rm_blockcount);
> >  	irec->rm_owner = be64_to_cpu(rec->rmap.rm_owner);
> > diff --git a/fs/xfs/libxfs/xfs_rmap.h b/fs/xfs/libxfs/xfs_rmap.h
> > index 0c2c3cb73429..abe633403fd1 100644
> > --- a/fs/xfs/libxfs/xfs_rmap.h
> > +++ b/fs/xfs/libxfs/xfs_rmap.h
> > @@ -68,6 +68,7 @@ xfs_rmap_irec_offset_unpack(
> >  	if (offset & ~(XFS_RMAP_OFF_MASK | XFS_RMAP_OFF_FLAGS))
> >  		return -EFSCORRUPTED;
> >  	irec->rm_offset = XFS_RMAP_OFF(offset);
> > +	irec->rm_flags = 0;
> 
> The change looks sensible-ish.  But why do we even have a separate
> xfs_rmap_irec_offset_unpack with a single caller nd out of the
> way in a header?  Wouldn't it make sense to just merge the two
> functions?

xfs_repair uses libxfs_rmap_irec_offset_unpack, which is why it's a
separate function.

--D
Christoph Hellwig Aug. 30, 2019, 3:31 p.m. UTC | #4 
On Thu, Aug 29, 2019 at 09:01:18AM -0700, Darrick J. Wong wrote:
> > >  	irec->rm_owner = be64_to_cpu(rec->rmap.rm_owner);
> > > diff --git a/fs/xfs/libxfs/xfs_rmap.h b/fs/xfs/libxfs/xfs_rmap.h
> > > index 0c2c3cb73429..abe633403fd1 100644
> > > --- a/fs/xfs/libxfs/xfs_rmap.h
> > > +++ b/fs/xfs/libxfs/xfs_rmap.h
> > > @@ -68,6 +68,7 @@ xfs_rmap_irec_offset_unpack(
> > >  	if (offset & ~(XFS_RMAP_OFF_MASK | XFS_RMAP_OFF_FLAGS))
> > >  		return -EFSCORRUPTED;
> > >  	irec->rm_offset = XFS_RMAP_OFF(offset);
> > > +	irec->rm_flags = 0;
> > 
> > The change looks sensible-ish.  But why do we even have a separate
> > xfs_rmap_irec_offset_unpack with a single caller nd out of the
> > way in a header?  Wouldn't it make sense to just merge the two
> > functions?
> 
> xfs_repair uses libxfs_rmap_irec_offset_unpack, which is why it's a
> separate function.

True.  Athough repair would also benefit a lot from a version of
xfs_rmap_btrec_to_irec that just takes a bmbt_key instead, but that is
for another time.
diff mbox series

Patch

diff --git a/fs/xfs/libxfs/xfs_rmap.c b/fs/xfs/libxfs/xfs_rmap.c
index 56769826a5ca..424c5f7343e1 100644
--- a/fs/xfs/libxfs/xfs_rmap.c
+++ b/fs/xfs/libxfs/xfs_rmap.c
@@ -168,7 +168,6 @@  xfs_rmap_btrec_to_irec(
 	union xfs_btree_rec	*rec,
 	struct xfs_rmap_irec	*irec)
 {
-	irec->rm_flags = 0;
 	irec->rm_startblock = be32_to_cpu(rec->rmap.rm_startblock);
 	irec->rm_blockcount = be32_to_cpu(rec->rmap.rm_blockcount);
 	irec->rm_owner = be64_to_cpu(rec->rmap.rm_owner);
diff --git a/fs/xfs/libxfs/xfs_rmap.h b/fs/xfs/libxfs/xfs_rmap.h
index 0c2c3cb73429..abe633403fd1 100644
--- a/fs/xfs/libxfs/xfs_rmap.h
+++ b/fs/xfs/libxfs/xfs_rmap.h
@@ -68,6 +68,7 @@  xfs_rmap_irec_offset_unpack(
 	if (offset & ~(XFS_RMAP_OFF_MASK | XFS_RMAP_OFF_FLAGS))
 		return -EFSCORRUPTED;
 	irec->rm_offset = XFS_RMAP_OFF(offset);
+	irec->rm_flags = 0;
 	if (offset & XFS_RMAP_OFF_ATTR_FORK)
 		irec->rm_flags |= XFS_RMAP_ATTR_FORK;
 	if (offset & XFS_RMAP_OFF_BMBT_BLOCK)