On 04/30/2015 07:49 AM, José Bollo wrote: > Le jeudi 30 avril 2015 à 00:15 -0500, Rob Landley a écrit : >> >> I mentioned the redundant path creation and switching it out for >> openat(O_PATH) >> and fgetxattr(). (If smack vetoes this open in ls, smack is probably going to >> have problems with other programs using modern apis.) > > Hello Rob, > > Thanks a lot for all of your patience and good work. I checked your > patch (despite that I didn't seen the starting point). That is great. > Bravo. > > Of course I changed things but very little. > > There is however something bad. I wanted to use O_NOATIME but found that > it is not defined.
According to git annotate include/uapi/asm-generic/fcntl.h it's been there since at least 2005 (and that was consolidating it from the platform-specific headers)... Ah: https://git.kernel.org/cgit/linux/kernel/git/tglx/history.git/commit/?id=5a86174c17ab9d62 June 20, 2004. That said, Posix doesn't seem to have caught up yet: http://pubs.opengroup.org/onlinepubs/9699919799//functions/open.html > The issue can be solved by defining _GNU_SOURCE at > the beginning of ls.c To quote the 11th Doctor (in his case with regards to just walking past a Fez), "Never gonna happen." It looks like I should add a code.html explanation of my objection to GNU_SOURCE. Many moons ago we had a discussion about this: http://lists.landley.net/pipermail/toybox-landley.net/2012-March/002002.html And I tried #defining a constellation of feature test macros to placate the compiler into defining the symbols that we #included headers to get, and it basically didn't work: http://lists.landley.net/pipermail/toybox-landley.net/2012-March/002064.html And I came to the conclusion that the proper way to fix this nonsense is to stick it in lib/portability.h: https://github.com/landley/toybox/blob/master/lib/portability.h#L219 I could alternately #include <asm-generic/fcntl.h> but that's not an improvement. > Halas it fails because of basename redefinition. > So I added the defining of O_NOATIME in portability. Indeed. My pending patch added O_PATH there too. (Oddly that's in my asm-generic/fcntl.h in ubuntu 2.12, but not in /usr/include/fcntl.h.) I was going to use musl as an example of how to to do this sanely, but for some inexplicable reason musl sticks O_NOATIME in each platform's bits/fcntl.h despite the fact it's the same value on every single target _and_ the kernel headers have had it in asm-generic since 2005. O_PATH is in there too (with the same value on all targets). *shrug*. We have portability.h. >> The first -Z hunk used lgetxattr() but the second used getxattr(). So we >> measured the length of the symlink attribute, then printed the file >> attribute? >> I'm consistently using openat(O_PATH|O_NOFOLLOW) which should give me a >> filehandle to the symlink... >> >> We're testing whether getxattr() returns a length > SMACK_LABEL_LEN. If that >> can happen, did it stomp stack data outside the buffer? If it can't happen, >> why are we testing for it? > > Ooops that is my fault. IIRC, I saw that getxattr was used where > lgetxattr is to be used and began to change it. But I finished not. And > in fact the behaviour has to follow the -L option. But doing that is > producing unexpected result that I should investigate. openat(O_PATH) seems to follow symlinks by default, and not do so when you feed it the O_NOFOLLOW bit. So that's easy enough to control... >> Query: we feed sizeof(zlen) to getxattr(), I.E. SMACK_LABEL_LEN+1. But we >> write a nul terminator, so the getxattr() call isn't terminating the buffer, >> so why are we feeding it a buffer size _including_ space for a null >> terminator? >> >> The third -Z hunk is identical to the second -Z hunk except it's left >> justified instead of right justified. Also your length is zlen-1 in hunk 3 >> and zlen without the -1 in hunk 2? Let's see... what you want is always one >> space on the right, only a space on the left for -long, and yes the >> left/right >> justification changes between -C and -l for no apparent reason but that's >> what you expect so... (Note: sprintf %*s will left justify if fed a negative >> length. Posix 2008 says so.) > > Yes true. We are all expecting that "if (CFG_LS_SMACK)" fails then the > code is removed by the compiler. So creating a function that is never > called is ... Well it's ok We compile with -ffunction-sections -fdata-sections and then feed the linker --gc-sections. This means that each function winds up in its own elf section and the unused elf sections are garbage collected at link time. (Presumably newer compilers have a less awkward way of doing this, but it's worked for 10 years now, so...) This used to trigger a glibc bug back when Ulrich DrPepper was maintainer, which he blamed on the linker people. Denys Vlasenko (currnt busybox maintainer) gave a presentation about this topic in 2010: http://free-electrons.com/blog/elc-2010-videos/ >> Why are you typecasting zlen to (int)? Doesn't subtracting a ssize_t work? > > On 64 bits arch, ssize_t is 64 bits and int is 32 bits. Does it explain? So it does the math in a 64 bit register and then converts it down automatically when the resulting argument is of type int? No, I still don't understand what the typecast accomplishes. >> The query is setting a length of 0 if getxattr() doesn't find any data, >> but you're printing "?" and a space, meaning the column sizing logic circa >> line 550 needs adjusting too. (totpad += totals[7]+!!totals[7]) > >>From what I sew of your new implementation, the way how the spaces > between fields is counted is mysterious. From my tests, it is not > working because it is not counted. Indeed, that would be the "I couldn't test this" part. My goal was that the format field is " %*s" and we add 1 to that if we don't want that space on the left, then the * is an integer argument which is size to pad the field to, ala: #include <stdio.h> int main(int argc, char *argv[]) { printf("%*s|\n", 10, "hello"); printf("%*s|\n", -10, "hello"); } Produces: hello| hello | (In this case two space indentation of examples confuses matters slightly, but hopefully not too much. :) >> Alright, here's a wild guess at a patch, which I can't test because >> the tizen image I installed following the instructions on >> https://wiki.tizen.org/wiki/Tizen_Standalone_Emulator hasn't got a native >> toolchain. > > (snip) > > I'm working on your proposal and will soon produce a submission. The > current difficulty is that calling "dirtree_parentfd(dt)" is currently > not reliable. If so that's a bug and needs fixing. It should always produce either the directory file descriptor or AT_FDCWD if we're at the top of the tree (dt->parent is NULL). So you should always be able to feed that into the first argument of openat(). > I'm still working on... Result in few minutes. Ah, I have more mail. Rob _______________________________________________ Toybox mailing list [email protected] http://lists.landley.net/listinfo.cgi/toybox-landley.net
