Re: [PATCH V3] xfs_repair: add support for validating dirent ftype field

[Dave Chinner <david@fromorbit.com>]

On Fri, Jan 24, 2014 at 08:42:42AM -0500, Brian Foster wrote:
> On 01/23/2014 05:52 PM, Dave Chinner wrote:
> > @@ -2189,6 +2238,59 @@ out_fix:
> >   * shortform directory v2 processing routines -- entry verification and
> >   * bad entry deletion (pruning).
> >   */
> > +static struct xfs_dir2_sf_entry *
> > +shortform_dir2_junk(
> > +	struct xfs_mount	*mp,
> > +	struct xfs_dir2_sf_hdr	*sfp,
> > +	struct xfs_dir2_sf_entry *sfep,
> > +	xfs_ino_t		lino,
> > +	int			*max_size,
> > +	int			*index,
> > +	int			*bytes_deleted,
> > +	int			*ino_dirty)
> > +{
> > +	struct xfs_dir2_sf_entry *tmp_sfep;
> > +	int			tmp_len;
> > +	int			tmp_elen;
> > +
> > +	if (lino == orphanage_ino)
> > +		orphanage_ino = 0;
> > +	if (no_modify) {
> > +		do_warn(_("would junk entry\n"));
> > +		return NULL;
> 
> Argh, sorry I missed this last time. ;) This looks like a problem. In
> the no_modify case, we return NULL and 'continue' in the caller, which
> skips the loop end logic and leads to bad things.
> 
> We could calculate and return the next entry here, but it might be
> cleaner to use a goto instead of continue in the callers and not
> duplicate the logic.

Argh, my bad. I missed that too. As it is, we already calculate the
next entry in this function - tmp_sfep

> 
> > +	}
> > +
> > +	tmp_elen = xfs_dir3_sf_entsize(mp, sfp,
> > +					sfep->namelen);
> > +	tmp_sfep = (xfs_dir2_sf_entry_t *)
> > +		((__psint_t) sfep + tmp_elen);
> > +	tmp_len = *max_size - ((__psint_t) tmp_sfep
> > +				- (__psint_t) sfp);
> > +	*max_size -= tmp_elen;
> > +	*bytes_deleted += tmp_elen;
> > +
> > +	memmove(sfep, tmp_sfep, tmp_len);

What this code is doing is moving all the subsequent entries after
the one being junked down over the entry we are junking...

> > +
> > +	sfp->count -= 1;
> > +	memset((void *)((__psint_t)sfep + tmp_len), 0,
> > +			tmp_elen);

And then zeroing the remaining bytes of the region that is no
longer used.

So the no modify check needs to happen after calculating tmp_sfep
and return that...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs