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&gt; > > > > 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.