Date: Sat, 7 Jun 2014 07:13:13 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Pietro Cerutti <gahr@freebsd.org> Cc: bapt@freebsd.org, src-committers@freebsd.org, svn-src-all@freebsd.org, Bruce Evans <brde@optusnet.com.au>, svn-src-head@freebsd.org, Julian Elischer <julian@freebsd.org>, cognet@freebsd.org Subject: Re: svn commit: r267027 - head/usr.bin/users Message-ID: <20140607061954.S9562@besplex.bde.org> In-Reply-To: <20140605101148.GJ37870@ptrcrt.ch> References: <201406032059.s53KxQAp081243@svn.freebsd.org> <20140605013835.M1934@besplex.bde.org> <20140605101148.GJ37870@ptrcrt.ch>
next in thread | previous in thread | raw e-mail | index | archive | help
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<string> 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<string>::iterator last(unique(begin(names), end(names)));
> copy(begin(names), last-1, ostream_iterator<string>(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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20140607061954.S9562>