[Toybox] [PATCH] fix ps.c build
enh
enh at google.com
Sat May 2 12:47:52 PDT 2015
On Sat, Apr 18, 2015 at 10:36 PM, Rob Landley <rob at landley.net> wrote:
> 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.]
this breaks things for me. my goal is to have one .config file but
build for both the host and Android, so i can run tests on the host
(where i have stuff like awk).
> 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 results are no longer sorted.
Fix getprop sorting.
Use qstrcmp instead of alphasort (which expects struct dirent arguments).
diff --git a/toys/android/getprop.c b/toys/android/getprop.c
index 400d80e..71289f0 100644
--- a/toys/android/getprop.c
+++ b/toys/android/getprop.c
@@ -41,7 +41,7 @@ void getprop_main(void)
size_t i;
if (property_list((void *)add_property, 0)) perror_exit("property_list");
- qsort(TT.nv, TT.size, 2*sizeof(char *), alphasort);
+ qsort(TT.nv, TT.size, 2*sizeof(char *), qstrcmp);
for (i = 0; i<TT.size; i++) printf("[%s]: [%s]\n",
TT.nv[i*2],TT.nv[1+i*2]);
if (CFG_TOYBOX_FREE) free(TT.nv);
}
> (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.)
no, it doesn't. since you won't have applied the previous patch by the
time you get here...
Fix getprop sorting and error reporting.
Use qstrcmp instead of alphasort (which expects struct dirent arguments).
Don't use perror_exit because property_list doesn't set errno.
diff --git a/toys/android/getprop.c b/toys/android/getprop.c
index 400d80e..09bb0f0 100644
--- a/toys/android/getprop.c
+++ b/toys/android/getprop.c
@@ -40,8 +40,8 @@ void getprop_main(void)
} else {
size_t i;
- if (property_list((void *)add_property, 0)) perror_exit("property_list");
- qsort(TT.nv, TT.size, 2*sizeof(char *), alphasort);
+ if (property_list((void *)add_property, 0)) error_exit("property_list");
+ qsort(TT.nv, TT.size, 2*sizeof(char *), qstrcmp);
for (i = 0; i<TT.size; i++) printf("[%s]: [%s]\n",
TT.nv[i*2],TT.nv[1+i*2]);
if (CFG_TOYBOX_FREE) free(TT.nv);
}
> 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
it helps catch careless mistakes. plus, here, it's weird to have a
wrapper around a function that takes const char* where the wrapper
takes the more restrictive (and misleading) char*.
> (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.)
but you'd have zero typecasts if xstrdup didn't have an incorrect prototype!
> 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...)
size_t is the type of an index/size. if you use something _other_ than
size_t, your reader has to stop and wonder whether something subtle is
going on. if you use size_t, no thinking required.
> 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.)
(anyone who cares has already moved on to C++ anyway. what the POSIX
folks do to qsort matters little to the people of 2015 passing lambdas
to std::sort or just using a sorted container. or, you know, Java or
Python.)
> 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.
you should have waited until you were using your new function before
walking away :-)
--
Elliott Hughes - http://who/enh - http://jessies.org/~enh/
Android native code/tools questions? Mail me/drop by/add me as a reviewer.
1430596072.0
More information about the Toybox
mailing list