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

[Dave Anderson]

----- Original Message -----
> 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.

Right -- and I'm sure you can find dozens of places where you would 
do things differently.  But I'm also sure you can understand why I'm not
over-enthused about re-working fundamental functions that haven't changed
for over 10 years just for the sake of changing them.  It forces me to
take the time to test the code to absolutely make sure there are no 
regressions introduced.  And if the change has architecture-dependencies,
the time is multiplied by having to do it for all of the supported architectures.  

At this point in the life-time of this crash utility, I'm far more 
interested in bug fixes and/or new functionality.
 
> 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.

OK fine, that part is queued for crash-5.1.2.

Thanks,
  Dave