FAQ Search Today's Posts Mark Forums Read
» Video Reviews

» Linux Archive

Linux-archive is a website aiming to archive linux email lists and to make them easily accessible for linux users/developers.


» Sponsor

» Partners

» Sponsor

Go Back   Linux Archive > Redhat > Crash Utility

 
 
LinkBack Thread Tools
 
Old 02-02-2011, 09:43 AM
Petr Tesarik
 
Default Fix ZERO_FILL flag to mkstring()

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@suse.cz>
--
Crash-utility mailing list
Crash-utility@redhat.com
https://www.redhat.com/mailman/listinfo/crash-utility
 
Old 02-02-2011, 09:55 AM
Petr Tesarik
 
Default Fix ZERO_FILL flag to mkstring()

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@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().

Fixed version attached.

Petr Tesarik
--
Crash-utility mailing list
Crash-utility@redhat.com
https://www.redhat.com/mailman/listinfo/crash-utility
 
Old 02-02-2011, 02:47 PM
Dave Anderson
 
Default Fix ZERO_FILL flag to mkstring()

----- 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@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
", 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>

Again, I get really nervous when you start fixing things
without there being a bug there to begin with...

Dave




--
Crash-utility mailing list
Crash-utility@redhat.com
https://www.redhat.com/mailman/listinfo/crash-utility
 
Old 02-03-2011, 06:46 AM
Petr Tesarik
 
Default 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@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
", 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<<
", mkstring(buf, 2*VADDR_PRLEN,
CENTER|LONG_HEX|ZERO_FILL, MKSTR(inode)));
fprintf(fp, ">>%s<<
", mkstring(buf, 2*VADDR_PRLEN,
LJUST|LONG_HEX|ZERO_FILL, MKSTR(inode)));
fprintf(fp, ">>%s<<
", 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@redhat.com
https://www.redhat.com/mailman/listinfo/crash-utility
 
Old 02-03-2011, 01:18 PM
Dave Anderson
 
Default Fix ZERO_FILL flag to mkstring()

----- 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@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
", 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<<
", mkstring(buf, 2*VADDR_PRLEN,
> CENTER|LONG_HEX|ZERO_FILL, MKSTR(inode)));
> fprintf(fp, ">>%s<<
", mkstring(buf, 2*VADDR_PRLEN,
> LJUST|LONG_HEX|ZERO_FILL, MKSTR(inode)));
> fprintf(fp, ">>%s<<
", 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


--
Crash-utility mailing list
Crash-utility@redhat.com
https://www.redhat.com/mailman/listinfo/crash-utility
 

Thread Tools




All times are GMT. The time now is 01:30 PM.

VBulletin, Copyright ©2000 - 2014, Jelsoft Enterprises Ltd.
Content Relevant URLs by vBSEO ©2007, Crawlability, Inc.
Copyright 2007 - 2008, www.linux-archive.org