libprocstat(3): retrieve process command line args and environment

[Mikolaj Golub]

On Sat, Mar 16, 2013 at 09:16:05PM +0200, Konstantin Belousov wrote:

> IMO sbuf_pad() should be moved to subr_sbuf.c. I find the KPI of
> the sbuf_pad() wrong. You have two separate number, the amount to
> pad to, and the current amount. Natural interface would take the
> two numbers separate instead of the difference. Also, could the
> sbuf_pad() deduce the current amount on its own, this would be the
> best ?

Hm, I am not sure about this.

So are you proposing instead of something like this

  sbuf_pad(sb, roundup(x, y) - x, 0);

to have

  sbuf_pad(sb, x, roundup(x, y), 0)?

Although it is a little easier to write, it looks less intuitive for
me.  E.g. I have troubles how to document this and explain. I can't
reffer x as a current position in sbuf, because it might not be. It is
just a some position, roundup(x,y) is another one, and only their
difference makes sence for sbuf_pad, so why not just to provide this
difference? So

  sbuf_pad(sb, from, to, c);

looks for me less intutive than

  sbuf_pad(sb, len, c);

A KPI that would be natural for my case: 

  /* start a section that is going to be aligned to sizeof(Elf_Size),
     using byte '0' for padding */
  sbuf_padded_start(sb, sizeof(Elf_Size), 0);
  /* write something to sbuf */
  sbuf_bcat(sb, data, len);
  /* align the sbuf section */
  sbuf_pad(sb);

This might look a little awkward and would add some overhead for the normal
case though...

> In register_note(), put spaces around '|' for the malloc line.
> 
> It seems that you did not moved the 'ABI hack' for ENOMEM situation for
> sysctl_kern_proc_filedesc() into the rewritten function.
>

Ah, this is a thing I wanted to discuss but forgot! As I understand
the idea of the 'ABI hack' is: if the output buffer is less than the
size of data we have, truncate our data to the last successfully
written kinfo_file structure and return without error.

In my code I do reset ENOMEM to 0 (see sysctl_kern_proc_filedesc), but
I don't see a way how using sbuf interface I can truncate req->oldidx
to the last successfully written file: sbuf_bcat() (to internal
buffer) may be successful and the error might appear only later, when
draining. I suspect it will break things if I return with a partial
kinfo_file, but haven't come with a solution yet...

> Please commit the changes to use pget() in the sysctl handlers separately.
> 
> Indents after the else clauses in kern_proc_out() are wrong.

Do you mean indents after '#ifdef COMPAT_FREEBSD32' block? I did it
that way so if COMPAT_FREEBSD32 sections were removed from the code
the indentation would be correct. I saw this practice through the code
and used it myself before.

> Since you are moving the KERN_PROC_ZOMBMASK out of kern_proc.c,
> a comment is needed to describe its usage. I would find it very
> confusing if grep reveals no like of code that sets the flags.
> 
> In the comment for sbuf_drain_core_output(), s/drainig/draining/,
> s/sefely/safely/ and s/hold/call us with the process lock held/.
> 
> I do not see much sense in the kstack note. The core is dumped when
> the threads are moved into the safe state in the kernel, so you
> cannot catch 'living' stack backtraces for the kernel side of
> things.

I was afraid of it after I had tried it on real dumps :-( Ok, will
remove the kstack note.

> On the other hand, things like umask, resources and osrel might be
> of the interest for post-morted analysis.

This is in my TODO list.

> I did not looked at the usermode changes.

Thanks for your suggestions! Will do them.

-- 
Mikolaj Golub