Page MenuHomeFreeBSD

Convert procstat to libxo
ClosedPublic

Authored by allanjude on May 5 2015, 2:14 AM.

Details

Summary

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

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

allanjude retitled this revision from to Convert procstat to libxo.
allanjude updated this object.
allanjude edited the test plan for this revision. (Show Details)
eadler removed a reviewer: eadler.

I am the wrong reviewer for this - sorry :\

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.

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.

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"
    }
  }
}

}

Update with feedback from rodrigc

Fixed the issue with the usage message in json mode

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 edited edge metadata.

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

This revision is now accepted and ready to land.Aug 28 2015, 1:26 AM
allanjude edited edge metadata.

Update manpage

This revision now requires review to proceed.Aug 28 2015, 6:27 AM
usr.bin/procstat/procstat.c
64 ↗(On Diff #8279)

There should be a new line here before asprintf

161 ↗(On Diff #8279)

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.

259 ↗(On Diff #8279)

Waht is allocation fails?

usr.bin/procstat/procstat.c
161 ↗(On Diff #8279)

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

usr.bin/procstat/procstat.c
161 ↗(On Diff #8279)

Couldn't a simple:

const char *xocontainer = NULL;

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

Do the trick in a simpler fashion?

usr.bin/procstat/procstat.c
161 ↗(On Diff #8279)

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.

Overall: nicely done!

usr.bin/procstat/Makefile
20 ↗(On Diff #8279)

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 ↗(On Diff #8279)

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);
82 ↗(On Diff #8279)

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
63 ↗(On Diff #8279)

Long line. Lots in this function.

usr.bin/procstat/procstat_basic.c
56 ↗(On Diff #8279)

Long line.

allanjude edited edge metadata.

Updated with feedback from bapt and marcel

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

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

allanjude edited edge metadata.

Rewrap long lines

usr.bin/procstat/procstat.c
263 ↗(On Diff #8413)

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 edited edge metadata.

Address feedback from bapt@

procstat.c:263

bapt edited edge metadata.
This revision is now accepted and ready to land.Sep 2 2015, 3:28 PM
marcel edited edge metadata.

Sorry. Looks good!

This revision was automatically updated to reflect the committed changes.