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

[Konstantin Belousov]

On Sun, Mar 17, 2013 at 12:35:20AM +0200, Mikolaj Golub wrote:
> 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...
This looks fine, in fact. You might want to call it sbuf_start_section()
and sbuf_end_section() ?

> 
> > 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...
All you need is to reset req->oldidx. This could be done outside the
sbuf interface, in the top level function implementing the sysctl ?

What you propose in the follow-up message should work too, I do not
have any preference.
> 
> > 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.
The sections are not going to be removed. IMHO code should be formatted
as if the preprocessor directive lines are not present. Could you point
out an example of existing code consistent with your indentation ?

> 
> > 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 834 bytes
Desc: not available
URL: <http://lists.freebsd.org/pipermail/freebsd-hackers/attachments/20130317/e243beb3/attachment.sig>