Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
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
Organization: Collabora
References: <20201210150353.91843-1-arnaud.ferraris@collabora.com>
        <20201210150353.91843-8-arnaud.ferraris@collabora.com>
        <87y2i51ixm.fsf@collabora.com>
        <40566e74-abd8-13df-45b9-2cf26f89ad54@collabora.com>
Date:   Tue, 15 Dec 2020 14:34:45 -0300
In-Reply-To: <40566e74-abd8-13df-45b9-2cf26f89ad54@collabora.com> (Arnaud
        Ferraris's message of "Tue, 15 Dec 2020 18:17:19 +0100")
Message-ID: <87r1nrt1l6.fsf@collabora.com>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8BIT
Precedence: bulk

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