xfs
[Top] [All Lists]

Re: [PATCH 4/9] Factor common inode cluster buffer lookup code

To: David Chinner <dgc@xxxxxxx>
Subject: Re: [PATCH 4/9] Factor common inode cluster buffer lookup code
From: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Date: Fri, 23 Nov 2007 17:40:13 +0000
Cc: xfs-oss <xfs@xxxxxxxxxxx>, lkml <linux-kernel@xxxxxxxxxxxxxxx>
In-reply-to: <20071122003642.GJ114266761@xxxxxxx>
References: <20071122003642.GJ114266761@xxxxxxx>
Sender: xfs-bounce@xxxxxxxxxxx
User-agent: Mutt/1.4.2.3i
On Thu, Nov 22, 2007 at 11:36:42AM +1100, David Chinner wrote:
> +STATIC int
> +xfs_ino_to_imap(
> +     xfs_mount_t     *mp,
> +     xfs_trans_t     *tp,
> +     xfs_ino_t       ino,
> +     xfs_imap_t      *imap,
> +     uint            imap_flags)
> +{
> +     int             error;
> +
> +     error = xfs_imap(mp, tp, ino, imap, imap_flags);
> +     if (error) {
> +             cmn_err(CE_WARN, "xfs_ino_to_imap: xfs_imap()  returned an "
> +                             "error %d on %s.  Returning error.",
> +                             error, mp->m_fsname);
> +             return error;
> +     }
> +
> +     /*
> +      * If the inode number maps to a block outside the bounds
> +      * of the file system then return NULL rather than calling
> +      * read_buf and panicing when we get an error from the
> +      * driver.
> +      */
> +     if ((imap->im_blkno + imap->im_len) >
> +         XFS_FSB_TO_BB(mp, mp->m_sb.sb_dblocks)) {
> +             xfs_fs_cmn_err(CE_ALERT, mp, "xfs_ino_to_imap: "
> +                     "(imap->im_blkno (0x%llx) + imap->im_len (0x%llx)) > "
> +                     " XFS_FSB_TO_BB(mp, mp->m_sb.sb_dblocks) (0x%llx)",
> +                     (unsigned long long) imap->im_blkno,
> +                     (unsigned long long) imap->im_len,
> +                     XFS_FSB_TO_BB(mp, mp->m_sb.sb_dblocks));
> +             return XFS_ERROR(EINVAL);
> +     }

What about just adding this verification to xfs_imap instead of creating
this wrapper for two of it's three callers?

Otherwise this patch looks fine to me.


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