2023-05-09 06:35:42

by Srivathsa Dara

[permalink] [raw]
Subject: [RESEND PATCH] debugfs/htree.c: In do_dx_hash() read hash_seed, hash_version directly from superblock

debugfs hash command computes the hash for the given filename. It takes
hash_seed and hash_version (i.e hash algorithm) as arguments. User has
to refer to the superblock to get these values used by the filesystem.
If the arguments are not given then debugfs computes hash assuming both
hash_seed and hash_version are zeros. In most of the cases this assumption
will be different from the actual hash_seed and hash_version used by the
filesystem. In general user will be in need of hash computed from
hash_seed and hash_version of the filesystem. So, instead of assuming
hash_seed and hash_version as zero when the arguments are not provided,
read these directly from the superblock to simplify the task of user.

Example:
Before:-
debugfs: hash -s 524e5394-e2a3-43fa-b192-79720b1fe3e1 -h half_md4 file1
Hash of file1 is 0x4a8d8c94 (minor 0x17a37f43)

After improvement:-
debugfs: hash file1
Hash of file1 is 0x4a8d8c94 (minor 0x17a37f43)

Signed-off-by: Srivathsa Dara <[email protected]>
---
debugfs/htree.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/debugfs/htree.c b/debugfs/htree.c
index 7fae7f11..2d881c74 100644
--- a/debugfs/htree.c
+++ b/debugfs/htree.c
@@ -316,7 +316,12 @@ void do_dx_hash(int argc, char *argv[], int sci_idx EXT2FS_ATTR((unused)),
int hash_flags = 0;
const struct ext2fs_nls_table *encoding = NULL;

- hash_seed[0] = hash_seed[1] = hash_seed[2] = hash_seed[3] = 0;
+ hash_seed[0] = current_fs->super->s_hash_seed[0];
+ hash_seed[1] = current_fs->super->s_hash_seed[1];
+ hash_seed[2] = current_fs->super->s_hash_seed[2];
+ hash_seed[3] = current_fs->super->s_hash_seed[3];
+
+ hash_version = current_fs->super->s_def_hash_version;

reset_getopt();
while ((c = getopt(argc, argv, "h:s:ce:")) != EOF) {
--
2.31.1


2023-12-03 05:15:18

by Theodore Ts'o

[permalink] [raw]
Subject: Re: [RESEND PATCH] debugfs/htree.c: In do_dx_hash() read hash_seed, hash_version directly from superblock

On Thu, Aug 24, 2023 at 06:56:34AM +0000, Srivathsa Dara wrote:
> diff --git a/debugfs/htree.c b/debugfs/htree.c
> index 7fae7f11..2d881c74 100644
> --- a/debugfs/htree.c
> +++ b/debugfs/htree.c
> @@ -316,7 +316,12 @@ void do_dx_hash(int argc, char *argv[], int sci_idx EXT2FS_ATTR((unused)),
> int hash_flags = 0;
> const struct ext2fs_nls_table *encoding = NULL;
>
> - hash_seed[0] = hash_seed[1] = hash_seed[2] = hash_seed[3] = 0;
> + hash_seed[0] = current_fs->super->s_hash_seed[0];
> + hash_seed[1] = current_fs->super->s_hash_seed[1];
> + hash_seed[2] = current_fs->super->s_hash_seed[2];
> + hash_seed[3] = current_fs->super->s_hash_seed[3];
> +
> + hash_version = current_fs->super->s_def_hash_version;
>
> reset_getopt();
> while ((c = getopt(argc, argv, "h:s:ce:")) != EOF) {

The problem with this patch is that if a file system is not opened,
then current_fs is NULL. As a result:

% gdb -q debugfs
Reading symbols from debugfs...
(gdb) run
Starting program: /build/e2fsprogs/debugfs/debugfs
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
debugfs 1.47.0 (5-Feb-2023)
debugfs: dx_hash test1

Program received signal SIGSEGV, Segmentation fault.
0x000055555556f73d in do_dx_hash (argc=2, argv=0x5555555d38d0, sci_idx=1, infop=0x0)
at /usr/projects/e2fsprogs/e2fsprogs/debugfs/htree.c:343
343 hash_seed[0] = current_fs->super->s_hash_seed[0];


To address this, I've fixed up your patch slightly. (See below for
the fix up, as well as the final patch.)

Also, in the future, please make sure that the first line of the
commit is a summary of the patch, no longer than 75 characters, and
that the text is wrapped to no more than 75 characters. (I personally
use 72 characters, but 75 is what suggested in the Linux Kernel's
submitting patches documentation[1] in the "The Canonical Patch
Format" section.)

[1] https://docs.kernel.org/process/submitting-patches.html

- Ted



Attachments:
(No filename) (2.00 kB)
fixup (1.04 kB)
final-patch (1.71 kB)
Download all attachments

2023-12-04 11:59:09

by Srivathsa Dara

[permalink] [raw]
Subject: RE: [External] : Re: [RESEND PATCH] debugfs/htree.c: In do_dx_hash() read hash_seed, hash_version directly from superblock

Hi Ted,
Thank you for considering the patch and improving it.
I'll make sure that I follow the proper patch formatting for
my future patches.

Thanks,
Srivathsa Dara

-----Original Message-----
From: Theodore Ts'o <[email protected]>
Sent: Sunday, December 3, 2023 10:45 AM
To: Srivathsa Dara <[email protected]>
Cc: [email protected]; [email protected]; Rajesh Sivaramasubramaniom <[email protected]>; Junxiao Bi <[email protected]>
Subject: [External] : Re: [RESEND PATCH] debugfs/htree.c: In do_dx_hash() read hash_seed, hash_version directly from superblock

On Thu, Aug 24, 2023 at 06:56:34AM +0000, Srivathsa Dara wrote:
> diff --git a/debugfs/htree.c b/debugfs/htree.c index
> 7fae7f11..2d881c74 100644
> --- a/debugfs/htree.c
> +++ b/debugfs/htree.c
> @@ -316,7 +316,12 @@ void do_dx_hash(int argc, char *argv[], int sci_idx EXT2FS_ATTR((unused)),
> int hash_flags = 0;
> const struct ext2fs_nls_table *encoding = NULL;
>
> - hash_seed[0] = hash_seed[1] = hash_seed[2] = hash_seed[3] = 0;
> + hash_seed[0] = current_fs->super->s_hash_seed[0];
> + hash_seed[1] = current_fs->super->s_hash_seed[1];
> + hash_seed[2] = current_fs->super->s_hash_seed[2];
> + hash_seed[3] = current_fs->super->s_hash_seed[3];
> +
> + hash_version = current_fs->super->s_def_hash_version;
>
> reset_getopt();
> while ((c = getopt(argc, argv, "h:s:ce:")) != EOF) {

The problem with this patch is that if a file system is not opened, then current_fs is NULL. As a result:

% gdb -q debugfs
Reading symbols from debugfs...
(gdb) run
Starting program: /build/e2fsprogs/debugfs/debugfs [Thread debugging using libthread_db enabled] Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
debugfs 1.47.0 (5-Feb-2023)
debugfs: dx_hash test1

Program received signal SIGSEGV, Segmentation fault.
0x000055555556f73d in do_dx_hash (argc=2, argv=0x5555555d38d0, sci_idx=1, infop=0x0)
at /usr/projects/e2fsprogs/e2fsprogs/debugfs/htree.c:343
343 hash_seed[0] = current_fs->super->s_hash_seed[0];


To address this, I've fixed up your patch slightly. (See below for the fix up, as well as the final patch.)

Also, in the future, please make sure that the first line of the commit is a summary of the patch, no longer than 75 characters, and that the text is wrapped to no more than 75 characters. (I personally use 72 characters, but 75 is what suggested in the Linux Kernel's submitting patches documentation[1] in the "The Canonical Patch Format" section.)

[1] https://urldefense.com/v3/__https://docs.kernel.org/process/submitting-patches.html__;!!ACWV5N9M2RV99hQ!Pfb7MqoeE6NAhDDwRgjeGOuOZJ5NozX9970IvoER0R4oDpPXfKVMC3Mmq_TzfqTesrPbvRy6gI5sxFlEQo4$

- Ted