On Thu, Aug 15, 2013 at 09:07:41AM -0500, Mark Tinguely wrote:
> On 08/14/13 19:33, Dave Chinner wrote:
> >On Wed, Aug 14, 2013 at 08:33:03AM -0500, Mark Tinguely wrote:
> >>>I don't understand this check.
> >>>
> >>>We've already dereferenced ip several lines above to increment the
> >>>link count and get the inode number stored in ino, so the ip != NULL
> >>>is unnecessary.
> >>>
> >>>We've just allocated the inode, so why would the magic number be
> >>>wrong? And why would the inode number lie in a non-existent
> >>>allocation group?
> >>>
> >>
> >>just being being paranoid.
> >
> >It's the same code as in the kernel - if is that broken that it
> >can't tell it didn't allocate a real inode, then we've got bigger
> >problems. We design the code to return an error when it fails so we
> >don't have to robustly check every possible error condition at every
> >call site. So really the only check needed is "if (!irec) {...}"
>
> The inode number check came from find_inode_rec(ag, agino)
> (repair/incore.h). A bad inode number will also return a NULL irec.
Yes, it will. But we just allocated the inode, so it's guaranteed to
have a valid inode number. We've got an inode *from a trusted
source* so we don't need to validate it the same way we need to
validate an inode number that we've pulled of disk that might be
corrupted.
Paranoia is great when using data from an untrusted source. But when
the data is from a trusted source, extreme paranoia is not necessary
and just makes the code hard to read...
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
