Convert procstat to libxo

[allanjude]

Summary

Structured output makes it easier to get the specific details one needs.

[eadler]

I am the wrong reviewer for this - sorry :\

[allanjude]

Update to use xo_version()

[• rodrigc]

Nice stuff, you put a lot of work into this. I would like to see this go into the tree.

I tested out your patch, and it looks good.

A few comments:

(1) If I do this:

procstat --libxo json -b -c 1

I do not see the usage() information printed out, like I do if I type:

procstat -b -c 1

(2) If I do this:

procstat --libxo json,pretty -w -b  1

I never get a terminating tag.

(3) If I do this:

procstat --libxo json,pretty -w -b  1

I get:

{
  "procstat": {
    "procstat_binary": {
      "1": {
        "process_id": 1,
        "command": "init",
        "osrel": 1100072,
        "pathname": "/sbin/init"
      }
    }
  }
}

Using "procstat_binary" is redundant. The outer tag is "procstat", so no need to have that for the inner tag.
This would be a bit cleaner:

{
  "procstat": {
    "binary": {
      "1" : {
        "process_id": 1,
        "command": "init",
        "osrel": 1100072,
        "pathname": "/sbin/init"
      }
    }
  }
}

Same comment applies for all the "xocontainer" tags like procstat_cs, procstat_arguments, etc.

Nice stuff though. I was able to do:

procstat --libxo json,pretty -b 1 3 7 8

and it did the right thing.

I also ran the arguments with the libxo flag, and they did the right thing.

If you can fix these minor issues, I would be happy to approve this patch, and see
it committed.

[allanjude]

In D2446#65665, @rodrigc wrote:

Nice stuff, you put a lot of work into this. I would like to see this go into the tree.

I tested out your patch, and it looks good.

A few comments:

(1) If I do this:

procstat --libxo json -b -c 1

I do not see the usage() information printed out, like I do if I type:

This seems to be a libxo issue. Not sure if it is actually solved upstream or not.

https://github.com/Juniper/libxo/issues/21

procstat -b -c 1

(2) If I do this:

procstat --libxo json,pretty -w -b  1

I never get a terminating tag.

I think you mean -w 1 -b 1

I'll need to change how this is handled.

How are you ending output to expect a terminator?

(3) If I do this:

procstat --libxo json,pretty -w -b  1

You didn't mean -w here, correct?

I get:

{
  "procstat": {
    "procstat_binary": {
      "1": {
        "process_id": 1,
        "command": "init",
        "osrel": 1100072,
        "pathname": "/sbin/init"
      }
    }
  }
}

Using "procstat_binary" is redundant. The outer tag is "procstat", so no need to have that for the inner tag.

removed redundancy

This would be a bit cleaner:

{
  "procstat": {
    "binary": {
      "1" : {
        "process_id": 1,
        "command": "init",
        "osrel": 1100072,
        "pathname": "/sbin/init"
      }
    }
  }
}

Same comment applies for all the "xocontainer" tags like procstat_cs, procstat_arguments, etc.

Nice stuff though. I was able to do:

procstat --libxo json,pretty -b 1 3 7 8

and it did the right thing.

I also ran the arguments with the libxo flag, and they did the right thing.

If you can fix these minor issues, I would be happy to approve this patch, and see
it committed.

[allanjude]

I have fixed the bug with -w, but there seems to be a bug in libxo that is causing an errant , near the beginning of the subsequent output.

{

"__version": "1",
"procstat": {
  "binary": {
    "1": {
      "process_id": 1,
      "command": "init",
      "osrel": 1100073,
      "pathname": "/sbin/init.bak"
    }
  }
}

}
{

"__version": "1",

,

"procstat": {
  "binary": {
    "1": {
      "process_id": 1,
      "command": "init",
      "osrel": 1100073,
      "pathname": "/sbin/init.bak"
    }
  }
}

}
{

"__version": "1",

,

"procstat": {
  "binary": {
    "1": {
      "process_id": 1,
      "command": "init",
      "osrel": 1100073,
      "pathname": "/sbin/init.bak"
    }
  }
}

}

[allanjude]

Update with feedback from rodrigc

[allanjude]

Fixed the issue with the usage message in json mode

[• rodrigc]

Please update the man page as part of this commit. I think if you follow the ls or wc
man pages, which just have [--libxo] and some cross-references to libxo, that is good enough for starters.

I'll test out the reset of the patch today.

[• rodrigc]

Beautiful!!

I tried a bunch of test cases, and it worked great, much better than the initial revision.
For example, I did:

procstat --libxo json,pretty -k -k 1

and got:

{
  "__version": "1", 
  "procstat": {
    "kstack": {
      "1": {
        "process_id": 1,
        "thread_id": 100002,
        "command": "init",
        "thread_name": "-",
        "trace": [
          "#0 0xffffffff80a27a79 at mi_switch+0x179",
          "#1 0xffffffff80a68192 at sleepq_switch+0x152",
          "#2 0xffffffff80a685dc at sleepq_catch_signals+0x2cc",
          "#3 0xffffffff80a6826f at sleepq_wait_sig+0xf",
          "#4 0xffffffff80a2732f at _sleep+0x32f",
          "#5 0xffffffff809e11f3 at kern_wait6+0x413",
          "#6 0xffffffff809e0bf3 at sys_wait4+0x73",
          "#7 0xffffffff80e6a3c2 at amd64_syscall+0x282",
          "#8 0xffffffff80e49f6b at Xfast_syscall+0xfb"
        ]
      }
    }
  }
}

Please fix up the man page, and commit this!!

[allanjude]

Update manpage

[bapt]

usr.bin/procstat/procstat.c

64

There should be a new line here before asprintf

165

so nothing technically prevents from using -S and -b at the same time here and you will result in 2 run of asprintf meaning the first allocation will just become a leak

also
each time you do use asprintf you should also check the allocated variable is not NULL or complain about it.

263

Waht is allocation fails?

[• rodrigc]

Thanks for the man page fix!

[allanjude]

usr.bin/procstat/procstat.c

165

Would I be better off just making a single fixed size buffer, and snprint'ing into it? In this case an overwrite won't cause a problem, and there is no change of an allocation failure.

Otherwise, I basically need to replicate this entire case ladder again after the mutually exclusive flags check on line 262

[bapt]

usr.bin/procstat/procstat.c

165

Couldn't a simple:

const char *xocontainer = NULL;

xocontainer = "binary";
break
xocontainter = "arguments";

Do the trick in a simpler fashion?

[marcel]

usr.bin/procstat/procstat.c

165

Indeed. There's nothing wrong with xocontainer being a character pointer that gets assigned to for each case. If the 'S' flag is given, assign "cs" to xocontainer. Similarly for all the other cases. All it does is set xocontainer to the address of some constant string.

[marcel]

Overall: nicely done!

usr.bin/procstat/Makefile

20

After the last import of libxo, libutil must be after libxo because elibxo uses humanize_number() from libutil. Thus, change the LIBADD line to the following:
LIBADD+= procstat xo util sbuf

usr.bin/procstat/procstat_args.c

56–57

This line is longer than 80 characters. You may want to break it, like:

xo_emit("{k:process_id/%5d/%d} {:command/%-16s/%s}", kipp->ki_pid,
    kipp->ki_comm);

83

This line is longer than 80 characters. You may want to break it, like:

xo_emit("{k:process_id/%5d/%d} {:command/%-16s/%s}", kipp->ki_pid,
    kipp->ki_comm);

usr.bin/procstat/procstat_auxv.c

68

Long line. Lots in this function.

usr.bin/procstat/procstat_basic.c

56

Long line.

[allanjude]

Updated with feedback from bapt and marcel

Removed many asprintf's and added allocation failure checks to the remaining instances

[• rodrigc]

@marcel , @bapt : I believe that @allanjude has addressed all the issues you raised. Is it OK to commit this now?

[allanjude]

Rewrap long lines

[bapt]

usr.bin/procstat/procstat.c

263

I think it would be cleaner to initialize xocontainer to "basic" and let it be overwritten based on flags.
this test will then become unneeded

[allanjude]

Address feedback from bapt@

procstat.c:263

[• rodrigc]

@marcel does this look OK?

[marcel]

Sorry. Looks good!