On Wed, Oct 26, 2016 at 10:15:53AM -0700, Darrick J. Wong wrote:
> On Wed, Oct 26, 2016 at 03:57:58AM -0700, Christoph Hellwig wrote:
> > > --- a/repair/dinode.c
> > > +++ b/repair/dinode.c
> > > @@ -722,6 +722,9 @@ _("Fatal error: inode %" PRIu64 " - blkmap_set_ext(): %s\n"
> > > * checking each entry without setting the
> > > * block bitmap
> > > */
> > > + if (type == XR_INO_DATA &&
> > > + xfs_sb_version_hasreflink(&mp->m_sb))
> > > + goto skip_dup;
> > > if (search_dup_extent(agno, agbno, ebno)) {
> > > do_warn(
> > > _("%s fork in ino %" PRIu64 " claims dup extent, "
> > > @@ -731,6 +734,7 @@ _("%s fork in ino %" PRIu64 " claims dup extent, "
> > > irec.br_blockcount);
> > > goto done;
> > > }
> > > +skip_dup:
> >
> > For some weird reason this goto makes me sad. I'm getting into
> > major nitpick terretory here, but can we avoid it? Either duplicate
> > the *tot increment or find some funky condition?
> >
> > Otherwise this looks fine.
>
> Ugh, unnecessary goto. Sad! ;)
>
> But in all seriousness an 'else if' could do the same work, so:
>
> if (type == XR_INO_DATA &&
> xfs_sb_version_hasreflink(&mp->m_sb))
> ; /* avoid the dup extent check below */
> else if (search_dup_extent(agno, agbno, ebno)) {
> do_warn(...);
> }
> *tot += irec.br_blockcount;
Even that is unnecessary, yes?
if ((type != XR_INO_DATA ||
!xfs_sb_version_hasreflink(&mp->m_sb)) &&
search_dup_extent(agno, agbno, ebno) {
do_warn(...);
}
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
