Re: [Crash-utility] [PATCH 2/2] Fix ZERO_FILL flag to mkstring()
Dne středa 02 Únor 2011 16:47:51 Dave Anderson napsal(a):
----- Original Message -----
> Dne středa 02 Únor 2011 11:43:18 Petr Tesarik napsal(a):
> > The ZERO_FILL flag should in fact be honoured during justification,
> > not for the formatting. This patch makes it possible to use
> > the ZERO_FILL flag for any type, not just LONG_DEC.
> >
> > Signed-off-by: Petr Tesarik <ptesarik(a)suse.cz>
>
> Argh, scrath this. I only re-compiled tools.c to see if it works, but
> there are more callers of shift_string_right().
Any function that is exported in defs.h must remain there -- with the
same prototype -- because they could be used by a pre-existing extension
module.
> Fixed version attached.
Quickly looking at the patch I didn't quite understand how/why this
part could be removed:
@@ -1562,12 +1573,6 @@ mkstring(char *s, int size, ulong flags,
case LONG_HEX:
sprintf(s, "%lx", (ulong)opt);
break;
- case (LONG_HEX|ZERO_FILL):
- if (VADDR_PRLEN == 8)
- sprintf(s, "%08lx", (ulong)opt);
- else if (VADDR_PRLEN == 16)
- sprintf(s, "%016lx", (ulong)opt);
- break;
case INT_DEC:
sprintf(s, "%u", (uint)((ulong)opt));
break;
so I applied this patch to test.c to check out an example of
LONG_HEX|ZERO_FILL:
--- test.c.orig 2011-02-02 10:30:31.000000000 -0500
+++ test.c 2011-02-02 10:27:11.000000000 -0500
@@ -42,6 +42,15 @@
;
optind++;
}
+
+{
+ char buf[BUFSIZE];
+ ulong inode;
+ inode = 1;
+
+ fprintf(fp, "%s\n", mkstring(buf, VADDR_PRLEN,
+ CENTER|LONG_HEX|ZERO_FILL, MKSTR(inode)));
+}
}
It should simply zero-fill the long hexadecimal value,
and should show this on a 64-bit machine:
crash> test
0000000000000001
crash>
But with your patch applied, it fails like this:
crash> test
0000000100000000
crash>
Ah, I can see the intended behaviour now. ZERO_FILL is different from filling
the remaining space, because it shouldn't fill to the target size, but to the
maximum width of the type. So, a better test case would be:
fprintf(fp, ">>%s<<\n", mkstring(buf,
2*VADDR_PRLEN,
CENTER|LONG_HEX|ZERO_FILL, MKSTR(inode)));
fprintf(fp, ">>%s<<\n", mkstring(buf,
2*VADDR_PRLEN,
LJUST|LONG_HEX|ZERO_FILL, MKSTR(inode)));
fprintf(fp, ">>%s<<\n", mkstring(buf,
2*VADDR_PRLEN,
RJUST|LONG_HEX|ZERO_FILL, MKSTR(inode)));
Note that I'm now aligning the buffer inside _twice_ the size of LONG.
Again, I get really nervous when you start fixing things
without there being a bug there to begin with...
While I can understand this position, there _is_ a bug IMO: code which is hard
to understand correctly and thus hard to maintain. Of course, this is a matter
of taste, and you're the long-term maintainer here, so I'll accept your
opinion.
Anyway, what about the first patch from this series? It gets rid of a local
fixed-size buffer and of repeated calls to strcat(). It can be applied without
this second patch.
Petr Tesarik