Re: [PATCH 2/3] ext2: Merge loops in ext2_xattr_set()

[Jan Kara <jack@suse.cz>]

On Thu 16-05-19 09:13:34, cgxu519@zoho.com.cn wrote:
> On Wed, 2019-05-15 at 16:01 +0200, Jan Kara wrote:
> > There are two very similar loops when searching xattr to set. Just merge
> > them.
> > 
> > Signed-off-by: Jan Kara <jack@suse.cz>
> > ---
> >  fs/ext2/xattr.c | 32 +++++++++++---------------------
> >  1 file changed, 11 insertions(+), 21 deletions(-)
> > 
> > diff --git a/fs/ext2/xattr.c b/fs/ext2/xattr.c
> > index fb2e008d4406..26a049ca89fb 100644
> > --- a/fs/ext2/xattr.c
> > +++ b/fs/ext2/xattr.c
> > @@ -437,27 +437,7 @@ ext2_xattr_set(struct inode *inode, int name_index, const
> > char *name,
> >  			goto cleanup;
> >  		}
> >  		/* Find the named attribute. */
> > -		here = FIRST_ENTRY(bh);
> > -		while (!IS_LAST_ENTRY(here)) {
> > -			struct ext2_xattr_entry *next = EXT2_XATTR_NEXT(here);
> > -			if ((char *)next >= end)
> > -				goto bad_block;
> > -			if (!here->e_value_block && here->e_value_size) {
> > -				size_t offs = le16_to_cpu(here->e_value_offs);
> > -				if (offs < min_offs)
> > -					min_offs = offs;
> > -			}
> > -			not_found = name_index - here->e_name_index;
> > -			if (!not_found)
> > -				not_found = name_len - here->e_name_len;
> > -			if (!not_found)
> > -				not_found = memcmp(name, here->e_name,name_len);
> > -			if (not_found <= 0)
> > -				break;
> > -			here = next;
> > -		}
> > -		last = here;
> > -		/* We still need to compute min_offs and last. */
> > +		last = FIRST_ENTRY(bh);
> >  		while (!IS_LAST_ENTRY(last)) {
> >  			struct ext2_xattr_entry *next = EXT2_XATTR_NEXT(last);
> >  			if ((char *)next >= end)
> > @@ -467,8 +447,18 @@ ext2_xattr_set(struct inode *inode, int name_index, const
> > char *name,
> >  				if (offs < min_offs)
> >  					min_offs = offs;
> >  			}
> > +			if (not_found) {
> > +				if (name_index == last->e_name_index &&
> > +				    name_len == last->e_name_len &&
> > +				    !memcmp(name, last->e_name,name_len)) {
> > +					not_found = 0;
> > +					here = last;
> > +				}
> > +			}
> >  			last = next;
> >  		}
> > +		if (not_found)
> > +			here = last;
> 
> Entry name is sorted so I think for new entry we should find right place for it
> not just appending to last.

Ah, good catch! I actually didn't find a place which would use the fact
that names are sorted (and that's why xfstests passed fine as well) but
you're right that the old code worked that way and we should keep that.
Thanks!

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR