[Toybox] [PATCH] fix ps.c build

[enh]

On Tue, May 5, 2015 at 8:53 AM, Rob Landley <rob at landley.net> wrote:

> On 05/02/2015 02:47 PM, enh wrote:
> >> 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).
>
> If you're building against an x86 version of bionic on the host, it'll
> consider it android. If you _don't_ have that, it's a build break to
> enable this stuff anyway...?


no, i'm building against glibc with your "make" because (a) it's a useful
way to double-check behavior and (b) i'm relying on the generated/ files
created by that. for now i've put the #if defined(__ANDROID__)s back.

i do want to move to where i'm generating the generated files at
build-time, but i need to be able to tell all that stuff where "generated/"
is (it needs to be in a directory of the build system's choosing, in the
"out" tree), and i need to supply the mkflags that the build system has
already built.

even when that's done, i'll still have a checked-in .config file, and i
won't want to have to maintain a .config.glibc that should be identical
except for toys that include <cutils/properties.h>.


> >      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);
> >    }
>
> Oops. Implemented a fix, checked in the library file, did not check in
> the change to the file that triggered it. (Sigh. The git status/diff
> stuff vs mercurial status/diff stuff are just different enough to
> repeatedly throw me off here...)
>
> >> 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.
>
> Applied.
>
> >> 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.
>
> String constants being in the read-only data segment should help catch
> careless mistakes. I've yet to see const do that. (It _can't_ for stack
> data.)
>
>  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*.
>
> It's from me trying to be consistent within toybox, where nothing uses
> const.
>
> I broke down and put the const on qstrcmp() because every single use of
> it would have to be typecast otherwise. (I've been tempted to switch the
> dlist stuff to void * for the same reason.) But xstrcmp() shouldn't
> throw warnings whether or not you feed it a const?
>
> >> (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!
>
> Or if property_list() didn't have a meaningless qualifier that exists
> solely to be matched with other meaningless qualifiers.
>
> (I experimented with "#define const" as the first include early in
> toybox' history. It didn't help. I forget why.)
>
> >> 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.
>
> But that logic the i in for loops should be size_t.
>
> > 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.
>
> For me it was the other way. I had to stop and think about whether there
> was something subtle going on.
>
> I like to know what types I'm actually using. This is why toys.optflags
> is an int rather than a long: on 32 bit platforms you get 32 bits (thus
> 32 possible flags). On 64 bit platforms, you get: 32 bits. You don't
> have something that works on 64 bits but _doesn't_ work on 32 bits.
>
> But size_t varies with "long file support" (archaic but still possible
> to forget to enable because the glibc default is still crazy:
> https://sourceware.org/ml/libc-alpha/2014-03/msg00092.html ). This is
> the _reason_ for size_t. Because it might _not_ be 64 bits.
>
> The above "i in for loops" example has a built-in reason _not_ to use
> it: manipulating 64 bit numbers on 32 bit systems is awkward so you
> _don't_ want to have a 64 bit index counter in a performance critical
> inner loop in that context (and you _really_ want to avoid 64 bit
> divides there). That isn't the case here but also isn't something you
> want to forget and do it wrong where it does matter. (I've seen people
> complain that large file support slowed down their programs and it was
> generally them pulling that sort of thing.)
>
> This isn't a big enough deal for me to bother to change it, but the "no
> thinking required" above? Not a compelling argument for me in this
> instance. :)
>
> >> Rob
> >>
> >> P.S. GIANT_RANTY_FOOTNOTE:
> ...
> > (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.)
>
> And when they collapse under the weight of their endlessly increasing
> complexity, C will presumably still be here. :)
>
> (The saving grace of ISO-WG14 and the Austin group is neither actually
> does very much, and when they do they're mostly* documenting existing
> practice. I'm still ignoring the 2011 C standard, because I can. I read
> the summaries when it came out, and other than the new thread
> synchronization stuff (NNPTL? NITL?) I think it was all fluff. The musl
> guys do care, and I cheer them on from the sidelines with popcorn.)
>
> (Don't get me started on Python. Honestly, python 3 is not the same
> language as python 2. One obvious way to do it indeed. They have
> _more_than_one_ conference panel at pycon about the lack of uptake of
> their rewrite (ala https://lwn.net/Articles/640181/ and
> https://lwn.net/Articles/640179/) six and a half years after its
> release, and they don't think this is damaging the language? A c89
> program will generally compile and run under a c99 compiler. Python 3
> breaks python 2. This is a problem.)
>
> * Yeah, I know.
>
> >> 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 :-)
>
> I totally should have, yes. Should be fixed now...
>
> Rob
>



-- 
Elliott Hughes - http://who/enh - http://jessies.org/~enh/
Android native code/tools questions? Mail me/drop by/add me as a reviewer.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.landley.net/pipermail/toybox-landley.net/attachments/20150505/ce7e9f8a/attachment-0002.htm>