[Toybox] [PATCH] roadmap update: Android switched to toybox ls today.

Tue Jun 23 12:08:20 PDT 2015

On Sat, May 16, 2015 at 8:32 PM, enh <enh at google.com> wrote:

>
>
> On Sat, May 16, 2015 at 3:23 PM, Rob Landley <rob at landley.net> wrote:
>
>> On 05/16/2015 03:43 PM, enh wrote:
>> > actually, this is the one outstanding patch you _shouldn't_ apply --- i
>> > had to revert the switch to ls late last night after it broke a
>> > surprising amount of infrastructure.
>>
>> Yeah, I was kinda surprised. I applied your first "add selinux" patch
>> verbatim, but it builds with a warning under glibc.
>
>
> that's not "under glibc" so much as "without selinux". this seems to fix
> that:
>
> diff --git a/lib/portability.h b/lib/portability.h
> index aa1ee48..4c4a6e7 100644
> --- a/lib/portability.h
> +++ b/lib/portability.h
> @@ -251,7 +251,8 @@ pid_t xfork(void);
>  #include <selinux/selinux.h>
>  #else
>  #define is_selinux_enabled() 0
> -int getcon(void* con);
> +int getcon(void *con);
> +int lgetfilecon(const char *path, char **con);
>  #endif
>
>  #if CFG_TOYBOX_SMACK
>
>
>
>> I have pending stuff
>> to fix in it, I posted my notes from my first look at it...
>>
>> > (the toolbox ls didn't columnate,
>>
>> I think I fixed that one here:
>>
>>
>> https://github.com/landley/toybox/commit/b18c7e8a59e61b8ec49e2d0f8cb2dda19b8f6a82
>>
>
> no, *toolbox* didn't. toybox ls was actually more compatible when -C
> didn't work properly :-)
>
>
>> > and "adb shell ..." creates a pty on
>> > the remote side, so toybox ls sees that and defaults to -C rather than
>> > -1. callers can't use -1 to say what they want, because toolbox ls
>> > didn't support that, and even if we add it today they still need to deal
>> > with a mix of old and new devices.
>>
>> In theory it should do the ANSI size probe to talk to your xterm. You
>> could also have "adb shell" export COLUMNS=1 to force -1 mode?
>>
>> (I'm not sure what the desired behavior is here in "given x, toybox
>> should do y" terms. Is "adb shell" being used to run scripts? Provide an
>> interactive terminal? Dunno the use case.)
>
>
> "adb shell" is interactive. "adb shell ..." isn't.
>
>
>> > similarly, the pty creation is on the
>> > adbd side, so although i'll fix that to be like ssh(1), that will only
>> > help new devices.
>>
>> I'm confused: you get toybox when you upgrade to android m, what
>> infrastructure is _not_ upgraded when you do that, causing the version
>> skew?
>
>
> test infrastructure. IDEs. that kind of thing.
>
>
>>
>> > i suspect we'll have to disable the isatty test in
>> > ls_main for the next ten years.
>>
>> In theory isatty() can also be taught "if the terminal size is 0 by 0
>> when you query it, it's not real".
>>
>> It's easy enough to make a kconfig switch so the behavior defaults to -1
>> instead of -C even when you have a terminal. (Or the test can be if
>> !CFG_TOYBOX_ANDROID...) I'd just like to understand th problem space a
>> little better first?
>
>
> oh, me too. i almost didn't write anything but thought that would just
> lead to questions, so i wrote the parenthetical explanation of why i'd had
> to revert in the hopes that would waste _less_ time. oops.
>
> short version: toybox is doing nothing wrong, i'm going to see how much
> infrastructure i can fix (if i can fix ddmlib, IDEs and the CTS stuff both
> just use that), and i'll come back with a specific patch if there's
> something i can't fix.
>
>
>>
>> > there's also code that parses the
>> > toolbox ls -l format, which doesn't have the "nlink" column, so i'll
>> > need to find and fix those regular expressions. but first i need to talk
>> > to the ddmlib folks to see why they're parsing ls output at all, when
>> > using the adb sync protocol queries would seem like a better idea...)
>>
>> We can add a flag to switch off that column, but given that posix is the
>> one that said to have that column:
>>
>> http://pubs.opengroup.org/onlinepubs/9699919799/utilities/ls.html
>>
>> That may not be the ideal solution...
>
>
> indeed, and i suspect this is the easiest thing to fix. something like
> (?:\s+\d+)? should suffice.
>
>
>>
>> > but there's plenty of other stuff like the hwclock fix and the find
>> > -inum addition and the xxd for pending to keep you busy,
>>
>> I'm trying to catch up, but I get on a plane back to japan tomorrow
>> afternoon.
>>
>> (Also, I'm supposed to port $DAYJOB's kernel to smp for sh2 (which is
>> fun because sh2 hasn't got an an atomic test-and-set so we may need to
>> change the vhdl again), and add dma to our drivers now the dma engine
>> can actually generate a completion interrupt, and finish forward porting
>> the the kernel to 4.0 (the ethernet driver uses obsolete infrastructure
>> to fake hardware timestamps and it broke for the third time between 3.19
>> and 4.0, I need to understand it so I can rip it out. Plus I spent half
>> the week unsuccessfully trying to make the h8 port's updated elf2flt
>> repo work with my last gplv2 release binutils 2.17, and I'm _still_
>> learning how bits of linking work...)
>>
>> > plus -- as you
>> > said -- there's smack for the flight :-) (rather than make things more
>> > confusing and risk slowing things down even further, i'm waiting until
>> > the smack changes are in before i worry about SELinux. what i've seen
>> > looks plausible though so i'm not anticipating problems.)
>>
>> There's a first pass review of that already posted, working on some of
>> that today...
>>
>> > btw, on the subject of testing --- i noticed that a command that SEGVs
>> > gets a "PASS". i've been meaning to fix that and send a patch, but since
>> > it's been two weeks since i noticed already, i should at least report
>> it...
>>
>> Huh. It shouldn't. (Unless it was expected to produce no output? Output
>> to stderr can get ignored, and the return value's only meaningful if you
>> explicitly test for it ala "command && echo yes"...)
>>
>> Where did you notice this? (Context?)
>
>
> in my new tests for xxd. at one point xxd was segfaulting but the test
> would say PASS.
>
> checking again, it looks like it's just this test:
>
> testing "xxd file2" "xxd file2" "" "" ""
>
> (file2 is an empty file, and the test is to check that we don't output an
> address until we have some data.)
>
> something like this removes the gotcha:
>
> diff --git a/scripts/runtest.sh b/scripts/runtest.sh
> index 8da1089..b9302b0 100644
> --- a/scripts/runtest.sh
> +++ b/scripts/runtest.sh
> @@ -86,6 +86,11 @@ testing()
>    echo -ne "$5" | eval "$2" > actual
>    RETVAL=$?
>
> +  if [ $RETVAL -gt 128 ] && [ $RETVAL -lt 255 ]
> +  then
> +    echo "exited with signal (or returned $RETVAL)" >> actual
> +  fi
> +
>    cmp expected actual > /dev/null 2>&1
>    if [ $? -ne 0 ]
>    then
>

ping on this test infrastructure problem since it prevented anyone from
adding a test for the ls segfault...

>
>> Rob
>>
>
>
>
> --
> Elliott Hughes - http://who/enh - http://jessies.org/~enh/
> Android native code/tools questions? Mail me/drop by/add me as a reviewer.
>

-- 
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/20150623/be35f77d/attachment-0002.htm>