[1/3] e2fsck: Clarify overflow link count error message
diff mbox series

Message ID 20200205100138.30053-2-jack@suse.cz
State New
Headers show
Series
  • e2fsprogs: Better handling of indexed directories
Related show

Commit Message

Jan Kara Feb. 5, 2020, 10:01 a.m. UTC
When directory link count is set to overflow value (1) but during pass 4
we find out the exact link count would fit, we either silently fix this
(which is not great because e2fsck then reports the fs was modified but
output doesn't indicate why in any way), or we report that link count is
wrong and ask whether we should fix it (in case -n option was
specified). The second case is even more misleading because it suggests
non-trivial fs corruption which then gets silently fixed on the next
run. Similarly to how we fix up other non-problems, just create a new
error message for the case directory link count is not overflown anymore
and always report it to clarify what is going on.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 e2fsck/pass4.c   | 20 ++++++++++++++++----
 e2fsck/problem.c |  5 +++++
 e2fsck/problem.h |  3 +++
 3 files changed, 24 insertions(+), 4 deletions(-)

Comments

Andreas Dilger Feb. 5, 2020, 5:38 p.m. UTC | #1
On Feb 5, 2020, at 3:01 AM, Jan Kara <jack@suse.cz> wrote:
> 
> When directory link count is set to overflow value (1) but during pass 4
> we find out the exact link count would fit, we either silently fix this
> (which is not great because e2fsck then reports the fs was modified but
> output doesn't indicate why in any way), or we report that link count is
> wrong and ask whether we should fix it (in case -n option was
> specified). The second case is even more misleading because it suggests
> non-trivial fs corruption which then gets silently fixed on the next
> run. Similarly to how we fix up other non-problems, just create a new
> error message for the case directory link count is not overflown anymore
> and always report it to clarify what is going on.
> 
> 
> diff --git a/e2fsck/problem.c b/e2fsck/problem.c
> index c7c0ba986006..cde369d03034 100644
> --- a/e2fsck/problem.c
> +++ b/e2fsck/problem.c
> @@ -2035,6 +2035,11 @@ static struct e2fsck_problem problem_table[] = {
> 	  N_("@d exceeds max links, but no DIR_NLINK feature in @S.\n"),
> 	  PROMPT_FIX, 0, 0, 0, 0 },
> 
> +	/* Directory ref count set to overflow but it doesn't have to be */

> +	{ PR_4_DIR_OVERFLOW_REF_COUNT,
> +	  N_("@d @i %i ref count set to overflow value %Il but could be exact value %N.  "),

IMHO, you don't need to print "value %Il" since that will always be "1"?
That would shorten the message to fit on a single line.

Also, lease keep the comment and the actual error message identical.
Otherwise, it is harder to find the PR_* number and the related code in
e2fsck when trying to debug a problem.  So the comment should be:

	/* Directory inode ref count set to overflow but could be exact value */

To be honest, I don't see the benefit of the @d, @i, etc. abbreviations
in the messages anymore.  The few bytes of space savings is IMHO outweighed
by the added complexity in understanding and finding the messages in the
code, and added complexity in e2fsck itself when printing the messages.


Cheers, Andreas
Jan Kara Feb. 6, 2020, 9:59 a.m. UTC | #2
On Wed 05-02-20 10:38:04, Andreas Dilger wrote:
> On Feb 5, 2020, at 3:01 AM, Jan Kara <jack@suse.cz> wrote:
> > 
> > When directory link count is set to overflow value (1) but during pass 4
> > we find out the exact link count would fit, we either silently fix this
> > (which is not great because e2fsck then reports the fs was modified but
> > output doesn't indicate why in any way), or we report that link count is
> > wrong and ask whether we should fix it (in case -n option was
> > specified). The second case is even more misleading because it suggests
> > non-trivial fs corruption which then gets silently fixed on the next
> > run. Similarly to how we fix up other non-problems, just create a new
> > error message for the case directory link count is not overflown anymore
> > and always report it to clarify what is going on.
> > 
> > 
> > diff --git a/e2fsck/problem.c b/e2fsck/problem.c
> > index c7c0ba986006..cde369d03034 100644
> > --- a/e2fsck/problem.c
> > +++ b/e2fsck/problem.c
> > @@ -2035,6 +2035,11 @@ static struct e2fsck_problem problem_table[] = {
> > 	  N_("@d exceeds max links, but no DIR_NLINK feature in @S.\n"),
> > 	  PROMPT_FIX, 0, 0, 0, 0 },
> > 
> > +	/* Directory ref count set to overflow but it doesn't have to be */
> 
> > +	{ PR_4_DIR_OVERFLOW_REF_COUNT,
> > +	  N_("@d @i %i ref count set to overflow value %Il but could be exact value %N.  "),
> 
> IMHO, you don't need to print "value %Il" since that will always be "1"?
> That would shorten the message to fit on a single line.

Yeah, will change.

> Also, lease keep the comment and the actual error message identical.
> Otherwise, it is harder to find the PR_* number and the related code in
> e2fsck when trying to debug a problem.  So the comment should be:
> 
> 	/* Directory inode ref count set to overflow but could be exact value */

Sure, thanks for catching this.

> To be honest, I don't see the benefit of the @d, @i, etc. abbreviations
> in the messages anymore.  The few bytes of space savings is IMHO outweighed
> by the added complexity in understanding and finding the messages in the
> code, and added complexity in e2fsck itself when printing the messages.

I tend to agree but I was never bothered enough to try to change that.

									Honza

Patch
diff mbox series

diff --git a/e2fsck/pass4.c b/e2fsck/pass4.c
index 10be7f87180d..8c2d2f1fca12 100644
--- a/e2fsck/pass4.c
+++ b/e2fsck/pass4.c
@@ -237,6 +237,8 @@  void e2fsck_pass4(e2fsck_t ctx)
 			link_counted = 1;
 		}
 		if (link_counted != link_count) {
+			int fix_nlink = 0;
+
 			e2fsck_read_inode_full(ctx, i, EXT2_INODE(inode),
 					       inode_size, "pass4");
 			pctx.ino = i;
@@ -250,10 +252,20 @@  void e2fsck_pass4(e2fsck_t ctx)
 			pctx.num = link_counted;
 			/* i_link_count was previously exceeded, but no longer
 			 * is, fix this but don't consider it an error */
-			if ((isdir && link_counted > 1 &&
-			     (inode->i_flags & EXT2_INDEX_FL) &&
-			     link_count == 1 && !(ctx->options & E2F_OPT_NO)) ||
-			    fix_problem(ctx, PR_4_BAD_REF_COUNT, &pctx)) {
+			if (isdir && link_counted > 1 &&
+			    (inode->i_flags & EXT2_INDEX_FL) &&
+			    link_count == 1) {
+				if ((ctx->options & E2F_OPT_READONLY) == 0) {
+					fix_nlink =
+						fix_problem(ctx,
+							PR_4_DIR_OVERFLOW_REF_COUNT,
+							&pctx);
+				}
+			} else {
+				fix_nlink = fix_problem(ctx, PR_4_BAD_REF_COUNT,
+						&pctx);
+			}
+			if (fix_nlink) {
 				inode->i_links_count = link_counted;
 				e2fsck_write_inode_full(ctx, i,
 							EXT2_INODE(inode),
diff --git a/e2fsck/problem.c b/e2fsck/problem.c
index c7c0ba986006..cde369d03034 100644
--- a/e2fsck/problem.c
+++ b/e2fsck/problem.c
@@ -2035,6 +2035,11 @@  static struct e2fsck_problem problem_table[] = {
 	  N_("@d exceeds max links, but no DIR_NLINK feature in @S.\n"),
 	  PROMPT_FIX, 0, 0, 0, 0 },
 
+	/* Directory ref count set to overflow but it doesn't have to be */
+	{ PR_4_DIR_OVERFLOW_REF_COUNT,
+	  N_("@d @i %i ref count set to overflow value %Il but could be exact value %N.  "),
+	  PROMPT_FIX, PR_PREEN_OK, 0, 0, 0 },
+
 	/* Pass 5 errors */
 
 	/* Pass 5: Checking group summary information */
diff --git a/e2fsck/problem.h b/e2fsck/problem.h
index c7f65f6dee0f..4185e5175cab 100644
--- a/e2fsck/problem.h
+++ b/e2fsck/problem.h
@@ -1164,6 +1164,9 @@  struct problem_context {
 /* directory exceeds max links, but no DIR_NLINK feature in superblock */
 #define PR_4_DIR_NLINK_FEATURE		0x040006
 
+/* Directory ref count set to overflow but it doesn't have to be */
+#define PR_4_DIR_OVERFLOW_REF_COUNT	0x040007
+
 /*
  * Pass 5 errors
  */