From owner-svn-src-all@FreeBSD.ORG Thu Jun 5 10:11:54 2014 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from ptrcrt.ch (freefall.freebsd.org [IPv6:2001:1900:2254:206c::16:87]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 90C6923E; Thu, 5 Jun 2014 10:11:52 +0000 (UTC) Date: Thu, 5 Jun 2014 12:11:48 +0200 From: Pietro Cerutti To: Bruce Evans Subject: Re: svn commit: r267027 - head/usr.bin/users Message-ID: <20140605101148.GJ37870@ptrcrt.ch> Reply-To: gahr@FreeBSD.org References: <201406032059.s53KxQAp081243@svn.freebsd.org> <20140605013835.M1934@besplex.bde.org> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="zOcTNEe3AzgCmdo9" Content-Disposition: inline In-Reply-To: <20140605013835.M1934@besplex.bde.org> X-PGP-Key: fp="DA6D E106 A5B8 54B8 5DD8 6D49 ADD0 D38E A192 089E"; id="0xA192089E"; get=; get=; User-Agent: Mutt/1.5.23 (2014-03-12) Cc: bapt@FreeBSD.org, src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org, Julian Elischer , cognet@FreeBSD.org X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 05 Jun 2014 10:11:54 -0000 --zOcTNEe3AzgCmdo9 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Bruce, your comments do make sense. I semi-seriously suggest that we get rid of the current implementation and replace it with this. Comments? /* * Copyright (c) 2014 Pietro Cerutti * All rights reserved. * * Redistribution and use in source and binary forms, with or without * modification, are permitted provided that the following conditions * are met: * 1. Redistributions of source code must retain the above copyright * notice, this list of conditions and the following disclaimer. * 2. Redistributions in binary form must reproduce the above copyright * notice, this list of conditions and the following disclaimer in the * documentation and/or other materials provided with the distribution. * 4. Neither the name of the University nor the names of its contributors * may be used to endorse or promote products derived from this software * without specific prior written permission. * * THIS SOFTWARE IS PROVIDED BY THE REGENTS AND CONTRIBUTORS ``AS IS'' AND * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPO= SE * ARE DISCLAIMED. IN NO EVENT SHALL THE REGENTS OR CONTRIBUTORS BE LIABLE * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTI= AL * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRI= CT * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF * SUCH DAMAGE. */ #include __FBSDID("$FreeBSD: head/usr.bin/users/users.c 267027 2014-06-03 20:59:26Z = gahr $"); #include #include #include #include #include #include #include using namespace std; int main(int argc, char **) { struct utmpx *ut; vector names; if (argc > 1) { cerr << "usage: users" << endl; return (1); } setutxent(); while ((ut =3D getutxent()) !=3D NULL) { if (ut->ut_type !=3D USER_PROCESS) continue; names.push_back(ut->ut_user); } endutxent(); if (names.size() =3D=3D 0) { return (0); } sort(begin(names), end(names)); vector::iterator last(unique(begin(names), end(names))); copy(begin(names), last-1, ostream_iterator(cout, " ")); cout << *(last-1) << endl; } On 2014-Jun-05, 02:20, Bruce Evans wrote: > On Tue, 3 Jun 2014, Pietro Cerutti wrote: >=20 > > Log: > > - Avoid calling a wrapper function around strcmp >=20 > This changes correct code to give undefined behaviour. >=20 > > - Use sizeof(*array) instead of sizeof(element) everywhere >=20 > This also allows removal of a typedef obfuscation. >=20 > > Modified: head/usr.bin/users/users.c > > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D > > --- head/usr.bin/users/users.c Tue Jun 3 20:58:11 2014 (r267026) > > +++ head/usr.bin/users/users.c Tue Jun 3 20:59:26 2014 (r267027) > > @@ -50,9 +50,9 @@ static const char rcsid[] =3D > > #include > > #include > > > > -typedef char namebuf[sizeof(((struct utmpx *)0)->ut_user) + 1]; > > +typedef char namebuf[sizeof(((struct utmpx *)0)->ut_user) + 1]; > > +typedef int (*scmp)(const void *, const void *); >=20 > Most typedefs shouldn't exist, and these 2 are no exceptions. >=20 > 'namebuf' only existed to reduce 2 copies of a type declaration to 1. > It obfuscated both. Now it is only used once. >=20 > 'scmp' only exists to help implement undefined behaviour. >=20 > The change also de-KNFizes the formatting (from tab to space after the > first part of the type). >=20 > > @@ -91,7 +91,7 @@ main(int argc, char **argv) > > } > > endutxent(); > > if (ncnt > 0) { > > - qsort(names, ncnt, sizeof(namebuf), scmp); > > + qsort(names, ncnt, sizeof(*names), (scmp)strcmp); >=20 > qsort()'s function pointer must point to a function like scmp, not > a different type of function with the type mismatch hidden by a bogus > cast. >=20 > The type puns fail as follow: > - strcmp is initially a function > - dereferencing it gives a pointer to a function of the type of strcmp > - casting this pointer to (scmp) gives a pointer to a different type > of function. The behaviour is defined. The representation may > change. > - use of such a cast function pointer to call the function is undefined > unless the pointer is converted back to the original (pointer) type > before making the call. Since qsort() cannot possibly know the origin= al > type, it cannot convert back. >=20 > It is a feature that qsort()'s API doesn't have a function pointer typede= f. > There are a few legitimate uses for function pointer typedefs, but in > practices most uses are for bogus casts to hide undefined behaviour, as > above. >=20 > The qsort() call also uses the other type obfuscation. 'namebuf' is > the typedef-name. This typedef name was only used twice, first to > declare 'names' and also here. Now with the better spelling of the > sizeof(), the typedef name is only used once. It just obfuscates > the declaration of 'names'. >=20 > > ... > > @@ -107,10 +107,3 @@ usage(void) > > fprintf(stderr, "usage: users\n"); > > exit(1); > > } > > - > > -int > > -scmp(const void *p, const void *q) > > -{ > > - > > - return (strcmp(p, q)); > > -} >=20 > Most qsort() functions have to look like this, or a bit more complicated, > to avoid the undefined behaviour. This one looks simpler than it is. > It has to convert the arg types to those of strcmp(), but this happens > automatically due to strcmp()s prototype. The arg types started as > strings, but had to be converted to const void *'s to match qsort()'s > API, then have to be converted back to strings here. >=20 > The undefined behaviour is usually to work in practice because only > Vax hardware is supported. strings are so similar to void * that it > is hard to tell if the behaviour can ever be undefined in practice > for type puns between them. But the behaviour is more clearly undefined > for function pointers because compatibility of void * and char * as > args doesn't extend to the function pointers. The implementation could > reasonably check all types at runtime and is only constrained from > generating a fatal error for type mismatches for certain mismatches > that are specified to work for compatibility reasons. I think that > if the bogus conversions get as far as calling the function, then > the behaviour is defined by the compatibility kludges: > - string args became const void * in qsort()'s internals > - they are not converted back when they are passed to strcmp() > - however, the compatibility kludges now apply. const void * looks > enough like const char * to work. (I think it may have a different > representation, but only with bits that aren't really used, for > example extra type bits that might be used for error checking > but must not be used here due to the compatibility kludges.) >=20 > Bruce --=20 Pietro Cerutti The FreeBSD Project gahr@FreeBSD.org PGP Public Key: http://gahr.ch/pgp --zOcTNEe3AzgCmdo9 Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQJ8BAEBCgBmBQJTkEJkXxSAAAAAAC4AKGlzc3Vlci1mcHJAbm90YXRpb25zLm9w ZW5wZ3AuZmlmdGhob3JzZW1hbi5uZXREQTZERTEwNkE1Qjg1NEI4NUREODZENDlB REQwRDM4RUExOTIwODlFAAoJEK3Q046hkgieQF4P/AmNjl1Bp7L1dex3jcmhntm6 VefBMgUNyhIvmXs/yu5BqYeshlHrHNpcQtwqQ8oLWVlmRn6DQpiOLvvdpvgvOe8A 8cGkTH9l9tPqwghV9fn/tN0p04w8lbvhNZCshaSZ0oE8X+jECwc94ilfxkfQDv9Q 8J7sRiyfp2D8kAM8/zsKHM//QoD5bbvi1eHiAPnKt3SSAwCQ6EGA8qU3Ou42WAFQ QjRyOZ9CBkRRwik/f60rvBDqRq5Gpxokzd9m4Hsl25CjH/RdlzQiaON+out7KWEK zuHvFrWUr5tFs566cQV/ETCyHucHBq6APDS+BOd3VChka9uUQUk+PuXzqrYQHWP/ i1dgthO8b8P0eX+XnCgFog3t9BPD25khM5Nfd7YYKwM/dcbg+cN09JPU/Onmi2FP H0mEvHWdshUTPl4y9QL5XbsecIIjLZhdEM4zW1Ij3+bb3NecTKGNtIJY1Q5Hk6tK 1ewgi1jRybq+oX67eGGpQdfy+LJKHYmoF8+XLJ39Ltn977NPIcJzFyvy0jV7arlZ DxUuRagUCl72G0iXpSuaTU3iddjCBRn0ZgA7n0Q3GJjbBRj0Jm78uV9JeljJ7/Vd ukDREHXung18HxmHJTMXBZEDriX20BDVs5pBCzon3YhvAFa+FFBbMZ13nTPBwgEH hFQeUVeRiJMYvy90Wc2y =LX0s -----END PGP SIGNATURE----- --zOcTNEe3AzgCmdo9--