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@xxxxxxx>
> >
> > 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
--
Crash-utility mailing list
Crash-utility@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/crash-utility
