From owner-svn-src-head@FreeBSD.ORG Mon Nov 3 17:07:03 2014 Return-Path: Delivered-To: svn-src-head@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 29F2A3E3; Mon, 3 Nov 2014 17:07:03 +0000 (UTC) Received: from mail107.syd.optusnet.com.au (mail107.syd.optusnet.com.au [211.29.132.53]) by mx1.freebsd.org (Postfix) with ESMTP id C8995C63; Mon, 3 Nov 2014 17:07:02 +0000 (UTC) Received: from c122-106-147-133.carlnfd1.nsw.optusnet.com.au (c122-106-147-133.carlnfd1.nsw.optusnet.com.au [122.106.147.133]) by mail107.syd.optusnet.com.au (Postfix) with ESMTPS id 87030D4A1BD; Tue, 4 Nov 2014 04:06:52 +1100 (AEDT) Date: Tue, 4 Nov 2014 04:06:51 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: d@delphij.net Subject: Re: svn commit: r273958 - head/sys/dev/random In-Reply-To: <54573E07.5040505@delphij.net> Message-ID: <20141104025944.F1038@besplex.bde.org> References: <201411020201.sA221unt091493@svn.freebsd.org> <720EB74E-094A-43F3-8B1C-47BC7F6FECC3@grondar.org> <1414934579.17308.248.camel@revolution.hippie.lan> <6FB65828-6A79-4BDE-A9F7-BC472BA538CE@grondar.org> <20141102192057.GB53947@kib.kiev.ua> <29A795E1-19E2-49E4-9653-143D3F6F3F12@grondar.org> <20141102194625.GC53947@kib.kiev.ua> <751CD860-95B9-4F68-AE69-976B42823AD0@grondar.org> <54568E41.8030305@delphij.net> <20141102201331.GE53947@kib.kiev.ua> <545693B4.8030602@delphij.net> <1414961583.1200.27.camel@revolution.hippie.lan> <5456A47E.2030405@delphij.net> <5456A69C.6040902@delphij.net> <20141103115402.H3149@besplex.bde.org> <54573E07.5040505@delphij.net> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.1 cv=BdjhjNd2 c=1 sm=1 tr=0 a=7NqvjVvQucbO2RlWB8PEog==:117 a=PO7r1zJSAAAA:8 a=kj9zAlcOel0A:10 a=JzwRw_2MAAAA:8 a=dFdFSw2EkNAFRa9SwC4A:9 a=CjuIK1q_8ugA:10 Cc: "src-committers@freebsd.org" , Bruce Evans , Ian Lepore , "svn-src-all@freebsd.org" , "svn-src-head@freebsd.org" , Mark R V Murray , Konstantin Belousov X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 03 Nov 2014 17:07:03 -0000 On Mon, 3 Nov 2014, Xin Li wrote: > -----BEGIN PGP SIGNED MESSAGE----- > Hash: SHA512 > > On 11/2/14 5:13 PM, Bruce Evans wrote: >> % - % - KASSERT(random_adaptor != NULL, ("No active random >> adaptor in %s", __func__)); % } % % void >> >> Lots of style bugs (long lines, use of the __func__ obfuscation, >> and general verboseness). > > What would you recommend doing for cases like this? Do we just do > KASSERT without the __func__ and rely on ddb to provide the > information in backtrace or something else? The implementation of KASSERT() should have been that. ddb, addr2line, and similar programs can produce better results at negative costs to the source and object code sizes. See an old flamew^Wdiscussion. Similarly for lock and mutex macros. Inlining complicates things, so that full debugging info is required to recover function names and context. __func__ sometimes gives the name of the function that you want, but not always. It is not useful to know the name of a deeply nested inline function -- instead the context is usually needed. Outisde of macros, I consider use of __func__ to be an obfuscation. It is easier to write but harder to read and useless for grepping. It is useful for macros like mtx_lock_spin() for the debugging case implemented with no ddb/addr2line support, since unlike for KASSERT() the API doesn't support passing in the caller's name. (mtx_lock_spin() actually uses the weaker __FILE__ and stronger __LINE__, and never __func__.) It is also rarely useful for inline functions, since there it gives the name of the inline function but usually you want the name of the immediate caller. If you want the name of the caller several steps up, then there is nothing better than a backtrace that has not been broken by inlining. assert(3) gets this wrong in the opposite direction. With assert(), the message is generated automatically from the assertion and there is no way to keep __FILE__, __LINE__ or __func__ out of it, though the implementation could produce these virtually to minimize the runtime bloat. The only way to add some extra info is to use some hack like the comma operator to add a string literal. So the above would be written as: assert(random_adaptor != NULL); which says almost as much in 1/3 the space and 1/10 of the mess in the source code, or with a different mess: assert(("X != NULL means no X (duh)", random_adaptor != NULL)); > Speaking for long lines -- I usually use the MTA's default so the diff > won't wrap. style(9) is a little bit vague here, what should new code > appear like? > >> % @@ -256,11 +258,15 @@ random_adaptor_read(struct cdev *dev >> __unused, str % /* Let the entropy source do any post-read >> cleanup. */ % (random_adaptor->ra_read)(NULL, 1); % % - >> free(random_buf, M_ENTROPY); % + if (nbytes != >> uio->uio_resid && (error == ERESTART || % + error == >> EINTR) ) % + error = 0; /* Return partial read, not >> error. */ I consider MTA wrapping a bug. It destroyed the diff after a couple of rounds of quoting. Some MTAs don't like my "% " quoting for diffs. style(9) just has an 80- or 79-column limit, stated by example of not having any lines longer than that. FreeBSD broke this by mangling the example C code into a man page. man adds a 5-space left margin and also normally formats for 80 columns (or just the terminal's width?), so it isn't clear what the example is. It looks more like a 75- or 74-column limit. That is good too since it leaves space for quoting. >> Extra blank lines are sprinkled randomly. Mostly added, but one >> was removed. There is an extra blank line after the sx_slock() that >> separates it from the blocks(s) of code that it locks; the removed >> one used the same style. > > Will change the code to consistently use no blank line after > sx_slock() and before sx_sunlock() if that's correct, can you confirm? I would probably put these blank lines back, to conform to the original (non-KNF) style. Bruce