[Crash-utility] [PATCH 2/2] Fix ZERO_FILL flag to mkstring()

[Petr Tesarik]

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 at 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