Date: Sun, 17 Mar 2013 00:35:20 +0200 From: Mikolaj Golub <trociny@FreeBSD.org> To: Konstantin Belousov <kostikbel@gmail.com> Cc: Stanislav Sedov <stas@freebsd.org>, Attilio Rao <attilio@freebsd.org>, "Robert N. M. Watson" <rwatson@freebsd.org>, freebsd-hackers@freebsd.org Subject: Re: libprocstat(3): retrieve process command line args and environment Message-ID: <20130316223339.GA3534@gmail.com> In-Reply-To: <20130316191605.GJ3794@kib.kiev.ua> References: <20130119151253.GB88025@gmail.com> <201301251531.43540.jhb@freebsd.org> <20130212215054.GA9839@gmail.com> <201302200904.15324.jhb@freebsd.org> <20130220195801.GA8679@gmail.com> <20130316180915.GA91146@gmail.com> <20130316191605.GJ3794@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20130316223339.GA3534>