kdump improvements, v2

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

kdump improvements, v2

Cédric Berger-2
Hi Mickey,

Here is version 2 of the patch which implements -X in a tcpdump-like fashion.
Actually, I think it is better because the output tracks the screen width, but I made
sure that for a 80-column display, it generates exactly 16-bytes per line ;)

Cedric

diff -u kdump.orig/kdump.1 kdump/kdump.1
--- kdump.orig/kdump.1 Fri May  5 10:28:12 2006
+++ kdump/kdump.1 Wed May 10 13:41:05 2006
@@ -104,6 +104,10 @@
 option of
 .Xr ktrace 1
 for the definitions of the flags.
+.It Fl x
+Display I/O data in hexadecimal.
+.It Fl X
+Display I/O data with hexadecimal data and printable ASCII characters side by side.
 .El
 .Sh FILES
 .Bl -tag -width ktrace.out -compact
diff -u kdump.orig/kdump.c kdump/kdump.c
--- kdump.orig/kdump.c Fri May  5 10:29:31 2006
+++ kdump/kdump.c Wed May 10 13:39:28 2006
@@ -65,7 +65,7 @@
 #include "kdump.h"
 #include "extern.h"
 
-int timestamp, decimal, fancy = 1, tail, maxdata;
+int timestamp, decimal, iohex, fancy = 1, tail, maxdata;
 char *tracefile = DEF_TRACEFILE;
 struct ktr_header ktr_header;
 pid_t pid = -1;
@@ -173,7 +173,7 @@
 
  current = &emulations[0]; /* native */
 
- while ((ch = getopt(argc, argv, "e:f:dlm:nRp:Tt:")) != -1)
+ while ((ch = getopt(argc, argv, "e:f:dlm:nRp:Tt:xX")) != -1)
  switch (ch) {
  case 'e':
  setemul(optarg);
@@ -207,6 +207,12 @@
  if (trpoints < 0)
  errx(1, "unknown trace point in %s", optarg);
  break;
+ case 'x':
+ iohex = 1;
+ break;
+ case 'X':
+ iohex = 2;
+ break;
  default:
  usage();
  }
@@ -512,10 +518,10 @@
 ktrgenio(struct ktr_genio *ktr, int len)
 {
  char *dp = (char *)ktr + sizeof (struct ktr_genio);
- int datalen = len - sizeof (struct ktr_genio);
+ int i, j, datalen = len - sizeof (struct ktr_genio);
  static int screenwidth = 0;
- int col = 0, width;
- char visbuf[5], *cp;
+ int col = 0, width, bpl;
+ char visbuf[5], *cp, c;
 
  if (screenwidth == 0) {
  struct winsize ws;
@@ -530,6 +536,50 @@
  ktr->ktr_rw == UIO_READ ? "read" : "wrote", datalen);
  if (maxdata && datalen > maxdata)
  datalen = maxdata;
+ if (iohex && !datalen)
+ return;
+ if (iohex == 1) {
+ putchar('\t');
+ col = 8;
+ for (i = 0; i < datalen; i++) {
+ printf("%02x", dp[i] & 0xff);
+ col += 3;
+ if (i < datalen - 1) {
+ if (col + 3 > screenwidth) {
+ printf("\n\t");
+ col = 8;
+ } else
+ putchar(' ');
+ }
+ }
+ putchar('\n');
+ return;
+ }
+ if (iohex == 2) {
+ bpl = (screenwidth - 13)/4;
+ if (bpl <= 0)
+ bpl = 1;
+ for (i = 0; i < datalen; i += bpl) {
+ printf("   %04x:  ", i);
+ for (j = 0; j < bpl; j++) {
+ if (i+j >= datalen)
+ printf("   ");
+ else
+ printf("%02x ", dp[i+j] & 0xff);
+ }
+ putchar(' ');
+ for (j = 0; j < bpl; j++) {
+ if (i+j >= datalen)
+ break;
+ c = dp[i+j];
+ if (!isprint(c) || c == ' ')
+ c = '.';
+ putchar(c);
+ }
+ putchar('\n');
+ }
+ return;
+ }
  (void)printf("       \"");
  col = 8;
  for (; datalen > 0; datalen--, dp++) {
@@ -605,7 +655,7 @@
 
  extern char *__progname;
  fprintf(stderr, "usage: %s "
-    "[-dnlRT] [-e emulation] [-p pid] [-f trfile] [-m maxdata] "
+    "[-dnlRTxX] [-e emulation] [-p pid] [-f trfile] [-m maxdata] "
     "[-t [ceinsw]]\n", __progname);
  exit(1);
 }

Reply | Threaded
Open this post in threaded view
|

Re: kdump improvements, v2

Ted Unangst-2
On 5/10/06, [hidden email] <[hidden email]> wrote:

> @@ -512,10 +518,10 @@
>  ktrgenio(struct ktr_genio *ktr, int len)
>  {
>        char *dp = (char *)ktr + sizeof (struct ktr_genio);
> -       int datalen = len - sizeof (struct ktr_genio);
> +       int i, j, datalen = len - sizeof (struct ktr_genio);
>        static int screenwidth = 0;
> -       int col = 0, width;
> -       char visbuf[5], *cp;
> +       int col = 0, width, bpl;
> +       char visbuf[5], *cp, c;

can you change these to unsigned chars?  passing signed char to isprint is bad.

> +                               c = dp[i+j];
> +                               if (!isprint(c) || c == ' ')
> +                                       c = '.';

why print period instead of space?

Reply | Threaded
Open this post in threaded view
|

Re: kdump improvements, v2

Cédric Berger-2
On May 10, 2006, at 6:36 PM, Ted Unangst wrote:

> On 5/10/06, [hidden email] <[hidden email]> wrote:
>> @@ -512,10 +518,10 @@
>>  ktrgenio(struct ktr_genio *ktr, int len)
>>  {
>>        char *dp = (char *)ktr + sizeof (struct ktr_genio);
>> -       int datalen = len - sizeof (struct ktr_genio);
>> +       int i, j, datalen = len - sizeof (struct ktr_genio);
>>        static int screenwidth = 0;
>> -       int col = 0, width;
>> -       char visbuf[5], *cp;
>> +       int col = 0, width, bpl;
>> +       char visbuf[5], *cp, c;
>
> can you change these to unsigned chars?  passing signed char to  
> isprint is bad.

Yes, I can try to change those. I'll wait a bit for other
comments first before sending final v3 version.

>
>> +                               c = dp[i+j];
>> +                               if (!isprint(c) || c == ' ')
>> +                                       c = '.';
>
> why print period instead of space?

First I did it with just isprint, but the output looked very bad when
there was a lot of space characters. What I mean is you cannot count
the character easily. So, for example, if you've:

   20 20 20 XX YY ZZ YY XX 20 20 20     Hello
   20 XX XX XX XX XX XX XX XX XX XX   World

I found that the following is easier to parse, easier to match left
and right blocks and count characters:

   20 20 20 XX YY ZZ YY XX 20 20 20  ...Hello...
   20 XX XX XX XX XX XX              .World!

Cedric

Reply | Threaded
Open this post in threaded view
|

Re: kdump improvements, v2

Michael Shalayeff-2
On Wed, May 10, 2006 at 07:51:40PM +0200, C?dric Berger wrote:

> On May 10, 2006, at 6:36 PM, Ted Unangst wrote:
>
> >On 5/10/06, [hidden email] <[hidden email]> wrote:
> >>@@ -512,10 +518,10 @@
> >> ktrgenio(struct ktr_genio *ktr, int len)
> >> {
> >>       char *dp = (char *)ktr + sizeof (struct ktr_genio);
> >>-       int datalen = len - sizeof (struct ktr_genio);
> >>+       int i, j, datalen = len - sizeof (struct ktr_genio);
> >>       static int screenwidth = 0;
> >>-       int col = 0, width;
> >>-       char visbuf[5], *cp;
> >>+       int col = 0, width, bpl;
> >>+       char visbuf[5], *cp, c;
> >
> >can you change these to unsigned chars?  passing signed char to  
> >isprint is bad.
>
> Yes, I can try to change those. I'll wait a bit for other
> comments first before sending final v3 version.
>
> >
> >>+                               c = dp[i+j];
> >>+                               if (!isprint(c) || c == ' ')
> >>+                                       c = '.';
> >
> >why print period instead of space?
>
> First I did it with just isprint, but the output looked very bad when
> there was a lot of space characters. What I mean is you cannot count
> the character easily. So, for example, if you've:
>
>   20 20 20 XX YY ZZ YY XX 20 20 20     Hello
>   20 XX XX XX XX XX XX XX XX XX XX   World
>
> I found that the following is easier to parse, easier to match left
> and right blocks and count characters:
>
>   20 20 20 XX YY ZZ YY XX 20 20 20  ...Hello...
>   20 XX XX XX XX XX XX              .World!

tcpdump prints space. i think we must be consistant.

cu
--
    paranoic mickey       (my employers have changed but, the name has remained)

Reply | Threaded
Open this post in threaded view
|

Re: kdump improvements, v2

Cédric Berger-2
mickey wrote:

> On Wed, May 10, 2006 at 07:51:40PM +0200, C?dric Berger wrote:
>> On May 10, 2006, at 6:36 PM, Ted Unangst wrote:
>>
>>> On 5/10/06, [hidden email] <[hidden email]> wrote:
>>>> @@ -512,10 +518,10 @@
>>>> ktrgenio(struct ktr_genio *ktr, int len)
>>>> {
>>>>       char *dp = (char *)ktr + sizeof (struct ktr_genio);
>>>> -       int datalen = len - sizeof (struct ktr_genio);
>>>> +       int i, j, datalen = len - sizeof (struct ktr_genio);
>>>>       static int screenwidth = 0;
>>>> -       int col = 0, width;
>>>> -       char visbuf[5], *cp;
>>>> +       int col = 0, width, bpl;
>>>> +       char visbuf[5], *cp, c;
>>> can you change these to unsigned chars?  passing signed char to  
>>> isprint is bad.
>> Yes, I can try to change those. I'll wait a bit for other
>> comments first before sending final v3 version.
>>
>>>> +                               c = dp[i+j];
>>>> +                               if (!isprint(c) || c == ' ')
>>>> +                                       c = '.';
>>> why print period instead of space?
>> First I did it with just isprint, but the output looked very bad when
>> there was a lot of space characters. What I mean is you cannot count
>> the character easily. So, for example, if you've:
>>
>>   20 20 20 XX YY ZZ YY XX 20 20 20     Hello
>>   20 XX XX XX XX XX XX XX XX XX XX   World
>>
>> I found that the following is easier to parse, easier to match left
>> and right blocks and count characters:
>>
>>   20 20 20 XX YY ZZ YY XX 20 20 20  ...Hello...
>>   20 XX XX XX XX XX XX              .World!
>
> tcpdump prints space. i think we must be consistant.

I prefer the dots, but if you guys like it better with spaces,
that's fine with me too. In any case it's a big improvement.

(One other reason I don't like spaces in columns like that is
that it breaks grepability... ok, I will shut up now :)

Do you need me to send a new version, or can you commit it
yourself with 'c' defined as uchar and without the c == ' '
part?

Cedric

Reply | Threaded
Open this post in threaded view
|

Re: kdump improvements, v2

Sean Kennedy-5
>Subject: Re: kdump improvements, v2
>>>>why print period instead of space?
>>>I found that the following is easier to parse, easier to match left
>>>and right blocks and count characters:
>>>
>>>   20 20 20 XX YY ZZ YY XX 20 20 20  ...Hello...
>>>   20 XX XX XX XX XX XX              .World!
>>
>>tcpdump prints space. i think we must be consistant.
>I prefer the dots, but if you guys like it better with spaces,
>that's fine with me too. In any case it's a big improvement.
>Cedric

For what its worth..
I concur with Ted on this one.

Hexdump and HexDisplay type modifiers show spaces. But if it can be easily
command-line switched, to 'period-pad' spaces for read/grep-ability I'd
recommend adding it as a feature, its a feature that has uses if one is
looking for a byte-code sequence of some sort.
But that too is what the HEX line numbers are for too. <shrug>
Note that if the command line switch is used, Nulls would need an indicator
like '`' <backtick> or something, to keep from being confused.

-sean