Re: [PATCH RESEND v2 07/12] e2fsck: Support casefold directories when rehashing

From: Gabriel Krisman Bertazi <krisman@collabora.com>
To: Arnaud Ferraris <arnaud.ferraris@collabora.com>
Cc: linux-ext4@vger.kernel.org, drosen@google.com,
	ebiggers@kernel.org, tytso@mit.edu
Subject: Re: [PATCH RESEND v2 07/12] e2fsck: Support casefold directories when rehashing
Date: Tue, 15 Dec 2020 14:34:45 -0300	[thread overview]
Message-ID: <87r1nrt1l6.fsf@collabora.com> (raw)
In-Reply-To: <40566e74-abd8-13df-45b9-2cf26f89ad54@collabora.com> (Arnaud Ferraris's message of "Tue, 15 Dec 2020 18:17:19 +0100")

Arnaud Ferraris <arnaud.ferraris@collabora.com> writes:

> Le 10/12/2020 à 21:53, Gabriel Krisman Bertazi a écrit :
>> Arnaud Ferraris <arnaud.ferraris@collabora.com> writes:
>> 
>>> From: Gabriel Krisman Bertazi <krisman@collabora.com>
>>>
>>> @@ -403,11 +451,12 @@ static int duplicate_search_and_fix(e2fsck_t ctx, ext2_filsys fs,
>>>  		ent = fd->harray + i;
>>>  		prev = ent - 1;
>>>  		if (!ent->dir->inode ||
>>> -		    (ext2fs_dirent_name_len(ent->dir) !=
>>> -		     ext2fs_dirent_name_len(prev->dir)) ||
>>> -		    memcmp(ent->dir->name, prev->dir->name,
>>> -			     ext2fs_dirent_name_len(ent->dir)))
>>> +		    !same_name(cmp_ctx, ent->dir->name,
>>> +			       ext2fs_dirent_name_len(ent->dir),
>>> +			       prev->dir->name,
>>> +			       ext2fs_dirent_name_len(prev->dir)))
>>>  			continue;
>>> +
   ^^^^^^^

>> 
>> noise.
>
> Could you please be more specific?

the patch is adding an empty line for no reason.

>
> Arnaud
>
>> 
>> Other than that, I think this is still good.
>> 
>>>  		pctx.dirent = ent->dir;
>>>  		if ((ent->dir->inode == prev->dir->inode) &&
>>>  		    fix_problem(ctx, PR_2_DUPLICATE_DIRENT, &pctx)) {
>>> @@ -426,10 +475,11 @@ static int duplicate_search_and_fix(e2fsck_t ctx, ext2_filsys fs,
>>>  		mutate_name(new_name, &new_len);
>>>  		for (j=0; j < fd->num_array; j++) {
>>>  			if ((i==j) ||
>>> -			    (new_len !=
>>> -			     (unsigned) ext2fs_dirent_name_len(fd->harray[j].dir)) ||
>>> -			    memcmp(new_name, fd->harray[j].dir->name, new_len))
>>> +			    !same_name(cmp_ctx, new_name, new_len,
>>> +				       fd->harray[j].dir->name,
>>> +				       ext2fs_dirent_name_len(fd->harray[j].dir))) {
>>>  				continue;
>>> +			}
>>>  			mutate_name(new_name, &new_len);
>>>  
>>>  			j = -1;
>>> @@ -894,6 +944,7 @@ errcode_t e2fsck_rehash_dir(e2fsck_t ctx, ext2_ino_t ino,
>>>  	struct fill_dir_struct	fd = { NULL, NULL, 0, 0, 0, NULL,
>>>  				       0, 0, 0, 0, 0, 0 };
>>>  	struct out_dir		outdir = { 0, 0, 0, 0 };
>>> +	struct name_cmp_ctx name_cmp_ctx = {0, NULL};
>>>  
>>>  	e2fsck_read_inode(ctx, ino, &inode, "rehash_dir");
>>>  
>>> @@ -921,6 +972,11 @@ errcode_t e2fsck_rehash_dir(e2fsck_t ctx, ext2_ino_t ino,
>>>  		fd.compress = 1;
>>>  	fd.parent = 0;
>>>  
>>> +	if (fs->encoding && (inode.i_flags & EXT4_CASEFOLD_FL)) {
>>> +		name_cmp_ctx.casefold = 1;
>>> +		name_cmp_ctx.tbl = fs->encoding;
>>> +	}
>>> +
>>>  retry_nohash:
>>>  	/* Read in the entire directory into memory */
>>>  	retval = ext2fs_block_iterate3(fs, ino, 0, 0,
>>> @@ -949,16 +1005,16 @@ retry_nohash:
>>>  	/* Sort the list */
>>>  resort:
>>>  	if (fd.compress && fd.num_array > 1)
>>> -		qsort(fd.harray+2, fd.num_array-2, sizeof(struct hash_entry),
>>> -		      hash_cmp);
>>> +		qsort_r(fd.harray+2, fd.num_array-2, sizeof(struct hash_entry),
>>> +			hash_cmp, &name_cmp_ctx);
>>>  	else
>>> -		qsort(fd.harray, fd.num_array, sizeof(struct hash_entry),
>>> -		      hash_cmp);
>>> +		qsort_r(fd.harray, fd.num_array, sizeof(struct hash_entry),
>>> +			hash_cmp, &name_cmp_ctx);
>>>  
>>>  	/*
>>>  	 * Look for duplicates
>>>  	 */
>>> -	if (duplicate_search_and_fix(ctx, fs, ino, &fd))
>>> +	if (duplicate_search_and_fix(ctx, fs, ino, &fd, &name_cmp_ctx))
>>>  		goto resort;
>>>  
>>>  	if (ctx->options & E2F_OPT_NO) {
>> 

-- 
Gabriel Krisman Bertazi