[Toybox] [PATCH] fix ps.c build

[Rob Landley]

On Sat, Apr 18, 2015 at 11:15 PM, enh <enh at google.com> wrote:
> On Sat, Apr 18, 2015 at 8:21 PM, Rob Landley <rob at landley.net> wrote:
>> I intend to finish and promote it this month, for what that's worth.
>> (I've got another 2 weeks...)
>
> yeah, for now i've taken ps out of the Android .config. we didn't have
> a symbolic link to it anyway, and although it's making progress, it's
> not currently very likely that anyone would want to "toybox ps".
>
> here's the current list of things from pending that get built into the
> AOSP toybox binary. ones marked with * get a symbolic link too.
>
>     toys/pending/dd.c
>     toys/pending/expr.c *
>     toys/pending/hwclock.c *
>     toys/pending/more.c *
>     toys/pending/pgrep.c *
>     toys/pending/netstat.c *
>     toys/pending/route.c *
>     toys/pending/tar.c *
>     toys/pending/top.c
>     toys/pending/tr.c *
>     toys/pending/traceroute.c
>
> those with links are either demonstrably better than what we had, or
> things we just didn't have before. so although rob worries, it's not
> like we're shipping all of pending and relying on it :-)

I'm cleaning up as fast as I can!

Speaking of which, here's a proposed getprop.c cleanup that I haven't
got a test environment for. The attached patch is infrastructure
changes, then the changed getprop.c file, and here's the notes from
cleanup:

---------

Proposed changes! (I'm not checking this in because I need your review
before doing that, I don't have your build environment or domain
expertise here.)

Instead of an #ifdef, let's add a "depends on TOYBOX_ON_ANDROID" so
you can only select the sucker when we're building on an android
system. (Meaning I can't test this, but I couldn't anyway because I
didn't have the header. I need to chop a build environment out of the
AOSP build...) [todo: teach "make change" about probed symbols.]

Globals go in GLOBALS(), this includes "properties" here.

Google says PROPERTY_VALUE_MAX is 92, sizeof(toybuf) is 4096, so let's
just use that.

Since properties is an array of pointers to 2 entry structures, just
put the structs inline in the array. Since the name is the first entry
and you can typecast a struct pointer to a pointer to its first entry
(C99 says so), we can use strcmp() directly in qsort, so that function
goes away.

(Except we have to wrap it anyway to do an extra dereference. [See
GIANT_RANTY_FOOTNOTE at the bottom] So I added a qstrcmp() function to
lib/lib.c.)

Also the free() function is just free() the array at the end so that function
goes away.

Also in add_property we realloc() with 32 more slots each time size is
a multiple of 32, so we don't need a separate capacity counter. (Since 0
is an even multiple of 32 we alloc the initial data the same way.)

I'm assuming that property_list() will set errno on a failure, and thus used
perror_exit(). (This is a questionable assumption I can't test, hence the shout
out here.)

About changing the xstrdup() prototype: Toybox doesn't use "const" because
I've never encountered a compiler that does anything intelligent with it
(what's it gonna do, put stack variables in a read-only segment?) and we
otherwise spend a huge amount of time getting it all to line up with no
actual benefit. There's a cleanup.html entry to this effect:
  http://landley.net/toybox/cleanup.html#advice

(I'm aware property_list() wants const in its prototype, and am hitting that
with (void *) which is ugly but still fewer typecasts than before.)

And looking at it: we don't really need the struct at all because it's just
pairs of char * with alternating meanings, that's simple enough to just do
inline. (This is a "six of one, half dozen of another" thing.)

Query: why size_t here? Is more than 2 billion of them likely to come up?
(Left it alone, but...)

Rob

P.S. GIANT_RANTY_FOOTNOTE:

qsort() really should have standard sort functions for things like
strcmp() on an array of pointers (or structs whose first member is a
pointer, which work the same way as far as qsort() is concerned). The
man page even has EXAMPLE CODE showing how to do this, and yet it's
not in libc and everybody needs to do their own gratuitous wrapper
function.

It _does_ say we can use alphasort(), which is wrong. That says it
wraps strcoll() which is equivalent to strcmp() in the "C" locale
(which we're in if we didn't feed TOYFLAG_LOCALE to the flags up top,
but I dunno if bionic actually implements that, even though it's in
C99. But that's not the wrong part, the wrong part is that when I
actually _try_ it I get incompatible pointer type because the man page
says it's alphasort(const void *a, const void *b) and the compiler is
saying "oh no, those arguments are struct dirent **"...

Ah, it's because the Posix Committee has its head up its ass:
  http://austingroupbugs.net/view.php?id=142

So posix-2008 is forcing people to use a typecast to get historical
behavior _in_the_name_of_type_safety_. That's just hilarious... (Ok,
looking deeper the Solaris guys broke this, then Jorg Schilling pushed
it upstream into posix, and it's up to _me_ to email Michael Kerrisk
to fix the man page _7_years_ later. And no, the solaris guys _really_
broke this so it doesn't work anymore even if you typecast. So the man
page needs fixing and the posix guys need somebody to mail them a box
of live stinging insects with a card that plays "seasons in the sun"
when you open it and doesn't stop when you close it again until the
battery runs down. Use a lithium battery.)

Anyway, I added a lib.c function for this. I was TEMPTED to add a
portability.c function and just declare posix's actions on this too
dumb to live, but somebody who shall remain nameless but used to
maintain cdrecord seems to ahve hijacked the posix committee into
doing stupid and I haven't got time to try to engage with the stupid
and move an elephant out of my way. So, lib.c function qstrcmp() and I
walk away from the keyboard for a bit.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: meep.patch
Type: text/x-patch
Size: 1289 bytes
Desc: not available
URL: <http://lists.landley.net/pipermail/toybox-landley.net/attachments/20150419/5bd2ee6d/attachment-0005.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: getprop.c
Type: text/x-csrc
Size: 1153 bytes
Desc: not available
URL: <http://lists.landley.net/pipermail/toybox-landley.net/attachments/20150419/5bd2ee6d/attachment-0005.c>