Date: Sun, 17 Mar 2013 08:30:33 +0200 From: Konstantin Belousov <kostikbel@gmail.com> To: Mikolaj Golub <trociny@FreeBSD.org> 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: <20130317063033.GL3794@kib.kiev.ua> In-Reply-To: <20130316223339.GA3534@gmail.com> 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> <20130316223339.GA3534@gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
--OjaoFelNtturnNY9 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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: >=20 > > 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 ? >=20 > Hm, I am not sure about this. >=20 > So are you proposing instead of something like this >=20 > sbuf_pad(sb, roundup(x, y) - x, 0); >=20 > to have >=20 > sbuf_pad(sb, x, roundup(x, y), 0)? >=20 > 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 >=20 > sbuf_pad(sb, from, to, c); >=20 > looks for me less intutive than >=20 > sbuf_pad(sb, len, c); >=20 > A KPI that would be natural for my case:=20 >=20 > /* 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); >=20 > This might look a little awkward and would add some overhead for the norm= al > case though... This looks fine, in fact. You might want to call it sbuf_start_section() and sbuf_end_section() ? >=20 > > In register_note(), put spaces around '|' for the malloc line. > >=20 > > It seems that you did not moved the 'ABI hack' for ENOMEM situation for > > sysctl_kern_proc_filedesc() into the rewritten function. > > >=20 > 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. >=20 > 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. >=20 > > Please commit the changes to use pget() in the sysctl handlers separate= ly. > >=20 > > Indents after the else clauses in kern_proc_out() are wrong. >=20 > 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 ? >=20 > > 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. > >=20 > > In the comment for sbuf_drain_core_output(), s/drainig/draining/, > > s/sefely/safely/ and s/hold/call us with the process lock held/. > >=20 > > 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. >=20 > I was afraid of it after I had tried it on real dumps :-( Ok, will > remove the kstack note. >=20 > > On the other hand, things like umask, resources and osrel might be > > of the interest for post-morted analysis. >=20 > This is in my TODO list. >=20 > > I did not looked at the usermode changes. >=20 > Thanks for your suggestions! Will do them. >=20 > --=20 > Mikolaj Golub --OjaoFelNtturnNY9 Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.19 (FreeBSD) iQIcBAEBAgAGBQJRRWMJAAoJEJDCuSvBvK1Bob4P/iy35q2eLdVJF0sg2TQqzB4q vN2wgPLZ4vLIgjulViGTILG0g8jG8pENecLCaPx3q0F4+s+qHVX26kALe5a1pv/P gsCr0Z30cUxmtgeXCPLFrpSSNwHgvr2dstWMx+NmVa6VISPLTgMDvxnm49XilE5Q q0wG9oyIzHHGGsTkcx4uMWMYILxc0bv+POs1gee6p0/DU3P8dGpljd7I7govZh/6 vhFgyhAgEgAH/puNtvJ23UJ5MWzzZTbhtAVPVSGXN9WZMBPkJ1rtb/TMEfGEUNEB qEHS2xylJtlPXBY06S/dCElxSHC1FmjOp0ezybk5EA6OlkPLu5A4ffoUXfj7gRux DnnafVpMjhEbaQb2qdNevKxtiVksRWQactm+uD7UWNUnJkOnCJTEXBiaiQvlC615 o0TmGIl4P5IjxeeCXTHOtCh62wNx28IjQJQcn+8yOugpRYh13hJxmACh1a8Hoysd OMPwCqclkHANgwIWYf5e3nz3eRzhg25vpnSjtrIGfrIkQ8DN2aWq7VvFLQTMYL+U jeje/ld3Tt0gaQON1qDv6ib8OjMVdHrf52b9ETRa3bZEEBiF7KyN1OkFX1vnebAV JTxo30qp8SCY15JXQq29LEI2ui2Z1BsxVnzlXrjs/gOTIaNpSxjl2V7B07RE0lWs tH8ISXMEReaIemaypkUW =vbxZ -----END PGP SIGNATURE----- --OjaoFelNtturnNY9--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20130317063033.GL3794>