svn commit: r247300 - in head: sys/sys usr.bin/truss

[Bruce Evans]

On Tue, 26 Feb 2013, Davide Italiano wrote:

> On Tue, Feb 26, 2013 at 7:35 PM, John Baldwin <jhb at freebsd.org> wrote:
>> On Tuesday, February 26, 2013 12:09:42 pm Davide Italiano wrote:
>>> On Tue, Feb 26, 2013 at 3:41 PM, John Baldwin <jhb at freebsd.org> wrote:
>>>> On Monday, February 25, 2013 9:13:02 pm Xin LI wrote:
>>>>> Author: delphij
>>>>> Date: Tue Feb 26 02:13:02 2013
>>>>> New Revision: 247300
>>>>> URL: http://svnweb.freebsd.org/changeset/base/247300
>>>>>
>>>>> Log:
>>>>>   Expose timespec and timeval macros when __BSD_VISIBLE is defined.  This
>>>>>   allows userland application to use the following macros:
>>>>>
>>>>>       timespecclear, timespecisset, timespeccmp, timespecadd,
>>>>>       timespecsub;
>>>>>
>>>>>       timevalclear, timevalisset, timevalcmp.

These were intentionally left out.  Unfortunately the API/namespace police
were not vigilant when the even worse APIs timerclear(), etc., were imported
from NetBSD/OpenBSD.

Apart from namespace pollution, bugs in these APIs start with them being
unsafe macros spelled as safe ones.  This bug is unimportant for
relatively limited use in the kernel, but is unsuitable for a public
API.

Not leaving the above out gives a reasonable but nonstandard set of APIs
for timespecs (the 5 listed above), but a weird sesqui-duplicated set for
timevals (the 3 listed above, plus the 5 misnamed timer*() from NetBSD/
OpenBSD).  Not leaving out the 3 duplicates the 3 least useful ones.

>>>> Why not fix truss to use the stock functions instead of keeping private
>>>> "unusual" versions?

They are 3-operand variants of the 2-operand kernel ones that escaped.
3 operands would be better (more general, and at no cost except in
calling complexity if the target operand is one of the source operands),
but the order of the 3 operands is as badly designed as possible:

     kernel API: operands (v, u); result v op= u;
     truss API:  operands (t, u, v); result should be t = u op v
                                     result is        v = t op u
         			    (just reversed; t is not the target...)

The timeval APIs are not actually sesqui-duplicated like I said above.
The NetBSD/OpenBSD ones follow the truss order, with t not being the
target (it just means generic timespec/timeval, while u and v mean
the second and third ones, and now it is the kernel API that is
confusing since there is no t and the order of the others is reversed
in the operands), while the 3 recently exposed kernel ones aren't of
the form target = source1 op source2.

truss's timespecadd() also has or had an off by 1 error in its tv_nsec
wrap check.

>>> time.h is already a mess in terms of namespace pollution, and this
>>> exposure might not help thing.
>>> Other details here:
>>> http://permalink.gmane.org/gmane.os.freebsd.architechture/15518

The bintime macros add further designed and implementation errors.  And
now there are sbintimes...

>> I think that is orthogonal.  Even if this is reverted I think truss should
>> be changed to use the "normal" timespecsubt() macro rather than using a custom
>> one with a different argument order.
>
> When I talked about "exposure" I referred about timeval/timespec
> macros(). I wasn't arguing about your proposed change.
> Sorry if it wasn't clear.

In fact, conversion would further unimprove truss, since it wants a
different target in all cases, so the 3-operand variants are ideal for
it.  Converting to the 2-operand variants would require a temporary
variable in all callers or in the macros.  The macros could be based
on the standard ones, but would hardly be improved by that since the
operations are so simple that no one would get them wrong with off by
1 errors etc.

Bruce