From owner-svn-src-all@FreeBSD.ORG Fri Jun 6 21:13:29 2014 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 3C2335EF; Fri, 6 Jun 2014 21:13:29 +0000 (UTC) Received: from mail109.syd.optusnet.com.au (mail109.syd.optusnet.com.au [211.29.132.80]) by mx1.freebsd.org (Postfix) with ESMTP id 13C262FC1; Fri, 6 Jun 2014 21:13:28 +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 mail109.syd.optusnet.com.au (Postfix) with ESMTPS id A92D0D617B9; Sat, 7 Jun 2014 07:13:16 +1000 (EST) Date: Sat, 7 Jun 2014 07:13:13 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Pietro Cerutti Subject: Re: svn commit: r267027 - head/usr.bin/users In-Reply-To: <20140605101148.GJ37870@ptrcrt.ch> Message-ID: <20140607061954.S9562@besplex.bde.org> References: <201406032059.s53KxQAp081243@svn.freebsd.org> <20140605013835.M1934@besplex.bde.org> <20140605101148.GJ37870@ptrcrt.ch> 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=eojmkOZX c=1 sm=1 tr=0 a=7NqvjVvQucbO2RlWB8PEog==:117 a=PO7r1zJSAAAA:8 a=M9zwebxRaw4A:10 a=kj9zAlcOel0A:10 a=JzwRw_2MAAAA:8 a=2NTOfg1Ub98rnNZzmX8A:9 a=4nYZKbiEO7K0d1u-:21 a=sZPDjW1OswgcD5fj:21 a=CjuIK1q_8ugA:10 Cc: bapt@freebsd.org, src-committers@freebsd.org, svn-src-all@freebsd.org, Bruce Evans , 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: Fri, 06 Jun 2014 21:13:29 -0000 On Thu, 5 Jun 2014, Pietro Cerutti wrote: > your comments do make sense. I semi-seriously suggest that we get rid of > the current implementation and replace it with this. Comments? Not for me, thanks. > ... > int > main(int argc, char **) > { > struct utmpx *ut; > vector names; > > if (argc > 1) { > cerr << "usage: users" << endl; > return (1); > } This doesn't give quite the same error handling as the getopt() call, but the C version could use the same simplification. The C version even has dead code for argc and argc. It has almost as many style bugs as the above. > > setutxent(); > while ((ut = getutxent()) != NULL) { > if (ut->ut_type != USER_PROCESS) > continue; > names.push_back(ut->ut_user); > } This is simpler, but the C version does much more than needed too. ut_user is now guaranteed to be NUL terminated. The above depends on this, but the current version still uses old code that adds a NUL terminator. This gives minor complications. It should be possible to build an array of pointers and just sort that. However, the getutxent() API is nasty, and is under-documented in FreeBSD. It returns a pointer to static storage (perhaps thread-local), as is common for bad old APIs. This is documented in POSIX, but doesn't seem to be documented in FreeBSD. It is obviously not expected that there be a large number of users (else the serial API would be worse than it is), so a small amount of statically allocated storage should be enough, especially if the application only has to hold the pointers and the database holds the strings. The 4.4BSD-Lite version of user(1) had a fixed limit of 200 users. Memory is free-er than it used to be, especially virtually, so no one would notice the bloat for statically allocating enough for a measly few million users, but in practice a few thousand should be enough. The unformatted output is bad for just 200 users. The above has only C++ default error handling. I know little about C++, but doubt that you it matches the errx() reporting exactly. Of course, realloc() can't fail, especially with only a few million users, so no error handling would work too. If there is a memory shortage, then getutxent() might fail too and its API doesn't even allow determining if there was an error -- the output would be truncated; I prefer a core dump. > endutxent(); > > if (names.size() == 0) { > return (0); > } > The C version can use the same simplifcation (to reduce indentation). > 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; > } This is not simpler. I prefer to iterate with a for loop (that is not obfuscated with a macro). The C code is simple and was mostly correct here. Except it has the usual null error checking for printf() failure, and bogus void'ing of the return value. The above seems to duplicate the null error handling. Bruce