Re: [PATCH v2] ext4: Enable code path when DX_DEBUG is set

2021-02-02 Thread Christoph Hellwig
On Mon, Feb 01, 2021 at 12:13:52PM -0500, Theodore Ts'o wrote: > However, if there *is* a bug, having an early detection that the > representation invariant of the data structure has been violated can > be useful in root causing a bug. This would probably be clearer if > the code was pulled out in

Re: [PATCH v2] ext4: Enable code path when DX_DEBUG is set

2021-02-01 Thread Nick Desaulniers
On Mon, Feb 1, 2021 at 2:48 PM Theodore Ts'o wrote: > > On Mon, Feb 01, 2021 at 07:05:11PM -0300, Vinicius Tinti wrote: > > > > The goal is to try to detect real bugs. In this instance specifically I > > suggested to remove the "if (0) {...}" because it sounded like an > > unused code. > > > > If

Re: [PATCH v2] ext4: Enable code path when DX_DEBUG is set

2021-02-01 Thread Theodore Ts'o
On Mon, Feb 01, 2021 at 07:05:11PM -0300, Vinicius Tinti wrote: > > The goal is to try to detect real bugs. In this instance specifically I > suggested to remove the "if (0) {...}" because it sounded like an > unused code. > > If it is useful it is fine to keep. The trick was that it was unused

Re: [PATCH v2] ext4: Enable code path when DX_DEBUG is set

2021-02-01 Thread Vinicius Tinti
On Mon, Feb 1, 2021 at 6:41 PM Nick Desaulniers wrote: > > On Mon, Feb 1, 2021 at 1:38 PM Theodore Ts'o wrote: > > > > On Mon, Feb 01, 2021 at 01:16:19PM -0800, Nick Desaulniers wrote: > > > I agree; Vinicius, my recommendation for -Wunreachable-* with Clang > > > was to see whether dead code ide

Re: [PATCH v2] ext4: Enable code path when DX_DEBUG is set

2021-02-01 Thread Nick Desaulniers
On Mon, Feb 1, 2021 at 1:38 PM Theodore Ts'o wrote: > > On Mon, Feb 01, 2021 at 01:16:19PM -0800, Nick Desaulniers wrote: > > I agree; Vinicius, my recommendation for -Wunreachable-* with Clang > > was to see whether dead code identified by this more aggressive > > diagnostic (than -Wunused-functi

Re: [PATCH v2] ext4: Enable code path when DX_DEBUG is set

2021-02-01 Thread Theodore Ts'o
On Mon, Feb 01, 2021 at 01:16:19PM -0800, Nick Desaulniers wrote: > I agree; Vinicius, my recommendation for -Wunreachable-* with Clang > was to see whether dead code identified by this more aggressive > diagnostic (than -Wunused-function) was to ask maintainers whether > code identified by it was

Re: [PATCH v2] ext4: Enable code path when DX_DEBUG is set

2021-02-01 Thread Nick Desaulniers
On Mon, Feb 1, 2021 at 1:09 PM Theodore Ts'o wrote: > > On Mon, Feb 01, 2021 at 03:41:50PM -0300, Vinicius Tinti wrote: > > > > My goal is to avoid having a dead code. Three options come to mind. > > > > The first would be to add another #ifdef SOMETHING (suggest a name). > > But this doesn't remo

Re: [PATCH v2] ext4: Enable code path when DX_DEBUG is set

2021-02-01 Thread Theodore Ts'o
On Mon, Feb 01, 2021 at 03:41:50PM -0300, Vinicius Tinti wrote: > > My goal is to avoid having a dead code. Three options come to mind. > > The first would be to add another #ifdef SOMETHING (suggest a name). > But this doesn't remove the code and someone could enable it by accident. I *really*

Re: [PATCH v2] ext4: Enable code path when DX_DEBUG is set

2021-02-01 Thread Andreas Dilger
On Feb 1, 2021, at 11:41 AM, Vinicius Tinti wrote: > > On Mon, Feb 1, 2021 at 2:13 PM Theodore Ts'o wrote: >> >> On Mon, Feb 01, 2021 at 01:15:29PM -0300, Vinicius Tinti wrote: >>> On Mon, Feb 1, 2021 at 9:49 AM Christoph Hellwig wrote: DX_DEBUG is completely dead code, so either ki

Re: [PATCH v2] ext4: Enable code path when DX_DEBUG is set

2021-02-01 Thread Vinicius Tinti
On Mon, Feb 1, 2021 at 2:13 PM Theodore Ts'o wrote: > > On Mon, Feb 01, 2021 at 01:15:29PM -0300, Vinicius Tinti wrote: > > On Mon, Feb 1, 2021 at 9:49 AM Christoph Hellwig wrote: > > > > > > DX_DEBUG is completely dead code, so either kill it off or make it an > > > actual CONFIG_* symbol throug

Re: [PATCH v2] ext4: Enable code path when DX_DEBUG is set

2021-02-01 Thread Theodore Ts'o
On Mon, Feb 01, 2021 at 01:15:29PM -0300, Vinicius Tinti wrote: > On Mon, Feb 1, 2021 at 9:49 AM Christoph Hellwig wrote: > > > > DX_DEBUG is completely dead code, so either kill it off or make it an > > actual CONFIG_* symbol through Kconfig if it seems useful. > > About the unreachable code in

Re: [PATCH v2] ext4: Enable code path when DX_DEBUG is set

2021-02-01 Thread Theodore Ts'o
On Mon, Feb 01, 2021 at 12:49:24PM +, Christoph Hellwig wrote: > DX_DEBUG is completely dead code, so either kill it off or make it an > actual CONFIG_* symbol through Kconfig if it seems useful. I wouldn't call it completely dead code. If you manually add "#define DX_DEBUG" fs/ext4/namei.c c

Re: [PATCH v2] ext4: Enable code path when DX_DEBUG is set

2021-02-01 Thread Vinicius Tinti
On Mon, Feb 1, 2021 at 9:49 AM Christoph Hellwig wrote: > > DX_DEBUG is completely dead code, so either kill it off or make it an > actual CONFIG_* symbol through Kconfig if it seems useful. About the unreachable code in "if (0)" I think it could be removed. It seems to be doing an extra check.

Re: [PATCH v2] ext4: Enable code path when DX_DEBUG is set

2021-02-01 Thread Christoph Hellwig
DX_DEBUG is completely dead code, so either kill it off or make it an actual CONFIG_* symbol through Kconfig if it seems useful.

Re: [PATCH v2] ext4: Enable code path when DX_DEBUG is set

2021-01-31 Thread Nathan Chancellor
On Mon, Feb 01, 2021 at 12:31:25AM +, Vinicius Tinti wrote: > By enabling -Wunreachable-code-aggressive on Clang the following code > paths are unreachable. > > This has been present since commit ac27a0ec112a ("[PATCH] ext4: initial > copy of files from ext3") and fs/ext3 had it present at the

[PATCH v2] ext4: Enable code path when DX_DEBUG is set

2021-01-31 Thread Vinicius Tinti
By enabling -Wunreachable-code-aggressive on Clang the following code paths are unreachable. This has been present since commit ac27a0ec112a ("[PATCH] ext4: initial copy of files from ext3") and fs/ext3 had it present at the beginning of git history. It has not been changed since. Clang warns: f