From owner-freebsd-hackers@FreeBSD.ORG Sat Mar 16 22:35:26 2013 Return-Path: Delivered-To: freebsd-hackers@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by hub.freebsd.org (Postfix) with ESMTP id D024389F; Sat, 16 Mar 2013 22:35:26 +0000 (UTC) (envelope-from to.my.trociny@gmail.com) Received: from mail-ea0-x22b.google.com (mail-ea0-x22b.google.com [IPv6:2a00:1450:4013:c01::22b]) by mx1.freebsd.org (Postfix) with ESMTP id 5B3AE6AE; Sat, 16 Mar 2013 22:35:25 +0000 (UTC) Received: by mail-ea0-f171.google.com with SMTP id b15so1991783eae.30 for ; Sat, 16 Mar 2013 15:35:24 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=x-received:sender:date:from:to:cc:subject:message-id:references :mime-version:content-type:content-disposition:in-reply-to :user-agent; bh=KksQC9biQ8w33sDheBv5Tu0s8ZIPkH8KbzGKvHH1gzI=; b=PwRPQQAEJ2H5no9JscVcF/lmrQ2kjxpHZmFsrBb1hHbS1Dt4n7MSkTlUacu91nsi+b 3ejiC68p0iIfyfbkF/7FIDPQ5R9PZBawMTBxQJ/numDkAN55jsQeMPkVeomMNWTAbaNV /9mWow5kk/8jU4PxWQm4zfi83ticmpDflvm7lf4BoIJgTON9OhYyKgxCwhSkuwZjihit SnRgxh6D/GjW/JlYTPjKOjxTbYEEsgQj5VVnSRIpLHkmC+wQ4YDBzyGxPPMGRZj/1Ify h/GYKSZJG+bWDfra0YT3dUK3rH5C2j8jqAOcyZm8d88mgUNDQO8gkpfeNPcSJYTJ/Qnh 0FiQ== X-Received: by 10.14.207.200 with SMTP id n48mr32940943eeo.4.1363473324359; Sat, 16 Mar 2013 15:35:24 -0700 (PDT) Received: from localhost ([178.150.115.244]) by mx.google.com with ESMTPS id t4sm18365855eel.0.2013.03.16.15.35.22 (version=TLSv1.2 cipher=RC4-SHA bits=128/128); Sat, 16 Mar 2013 15:35:23 -0700 (PDT) Sender: Mikolaj Golub Date: Sun, 17 Mar 2013 00:35:20 +0200 From: Mikolaj Golub To: Konstantin Belousov Subject: Re: libprocstat(3): retrieve process command line args and environment Message-ID: <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> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20130316191605.GJ3794@kib.kiev.ua> User-Agent: Mutt/1.5.21 (2010-09-15) Cc: Stanislav Sedov , Attilio Rao , "Robert N. M. Watson" , freebsd-hackers@freebsd.org X-BeenThere: freebsd-hackers@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: Technical Discussions relating to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 16 Mar 2013 22:35:26 -0000 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