Re: [Toybox] patch to disable scripts/make.sh GITHASH=...

[Felix Janda]

Thomas wrote:
> > Wait, your git --describe is finding a random tarball in some other
> > directory?
> >
> > That's really broken.
> ...
> > That's a pretty clear yocto bug (one I can't reproduce), but if you're
> > adding a workaround for this yocto bug the easy thing to do is keep
> > GITHASH if it's already be set, not add a second special case variable.
> 
> Replace "yocto" with "project using git + building toybox"
> 
> make.sh sets GITHASH because the "." directory is "under" git.
> /proj/whatever/ ... was checked out of git and contains:
> /proj/whatever/things/toybox/download-build-toybox.sh
> ... "git describe --tags --abbrev=12" prints out the tag/hash
> for parent project whatever ... not for toybox.tgz.
> 
> GITHASH is technically correct, but annoyingly unhelpful.
> 
> How shall I force "toybox --version" to print "toybox 0.7.4"
> instead of "toybox whatever-16.0.0-222-ge7bb9e918200"
> just because the toybox build dir is "under" a git dir?

Have you looked at the GIT_CEILING_DIRECTORIES variable?

Felix
___
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net

Re: [Toybox] FreeBSD support patch.

[Felix Janda]

Just commenting on this:

Rob Landley wrote:
> As for the lib/portability.h patch: FreeBSD doesn't have features.h, but
> Linux and MacOS X agree on that one? Odd. Speaking of which, we have
> __FreeBSD__ without #including features.h?

The compiler might define it similarly to how it defines __linux__.

> As for this bit:
> 
> -#if !defined(__GLIBC__) && !defined(__BIONIC__)
> +#if !defined(__GLIBC__) && !defined(__BIONIC__) && !defined(__FreeBSD__)
> 
> If we didn't include features.h, we shouldn't have __GLIBC__ or
> __BIONIC__ #defined because that's the header that defines them. I'd
> also assume that if we're building on freebsd, we don't have those
> #defined either because it's got its own libc? So why add a __FreeBSD__
> guard symbol here?

For glibc it seems to me, that almost any header will include
. So including  is presumably enough to get
the definition of _GLIBC_. It might also work for the other systems
targetted by portability.h.

Felix

PS: Just to mention, there is also something called kfreebsd [1], which
will define both _GLIBC_ and __FreeBSD__ (but not __linux__).

[1]: http://www.debian.org/ports/kfreebsd-gnu
___
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net

Re: [Toybox] [PATCH] include sys/sysmacros.h

[Felix Janda]

Rich Felker wrote:
...
> > glibc is the process of deprecating, and musl/uClibc would follow suit,
> > if not just drop the include altogether.
> 
> musl has never had the issue. :-) because we don't implicitly include
> sys/sysmacros.h at all. The set of programs that have any use for
> these macros is basically ls, mknod, and archivers (tar, cpio, etc.)
> that support archiving device nodes, and these programs can just
> include the appropriate header to get them.

musl's sys/types.h currently does include sys/sysmacros.h in a
_BSD_SOURCE/_GNU_SOURCE block. (sys/types.h is not implictly included
by stdlib.h, though.)

Felix
___
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net

Re: [Toybox] Implement wget

[Felix Janda]

Lipi C. H. Lee wrote:
> implement simple 'wget' and port name can be specified in URL if default
> port 80 is not used.
> It may be added to toys/pending directory.

Thanks for your submission!

Some comments below.

In general, try simplifying the error messages:

http://lists.landley.net/pipermail/toybox-landley.net/2013-April/000850.html

Also, generally functions are only added when they can be called at
least twice. See

http://lists.landley.net/pipermail/toybox-landley.net/2013-April/000919.html

(URLs taken from http://landley.net/toybox/cleanup.html)

> /* wget.c - Simple downloader to get the resource file in HTTP server
>  *
>  * Copyright 2016 Lipi C.H. Lee 
>  *
> 
> USE_WGET(NEWTOY(wget, "f:", TOYFLAG_USR|TOYFLAG_BIN))
> 
> config WGET
>   bool "wget"
>   default n
>   help
> usage: wget -f filename URL
> -f filename: specify the filename to be saved
> URL: HTTP uniform resource location and only HTTP, not HTTPS
> 
> examples:
>   wget -f index.html http://www.example.com
>   wget -f sample.jpg http://www.example.com:8080/sample.jpg
> */
> 
> #define FOR_wget
> #include "toys.h"
> 
> GLOBALS(
>   char *filename;
> )
> 
> #define HN_LEN 128 // HOSTNAME MAX LENGTH 
> #define PATH_LEN 256 // PATH MAX LENGTH
> 
> struct httpinfo {
>   char hostname[HN_LEN];
>   char port[6];  // MAX port value: 65535
>   char path[PATH_LEN];
> };

In the code base, global defines are usually avoided. If the functions
were inlined, this struct would actually be unecessary and HN_LEN
could be sizeof hostname.

> 
> // extract hostname from url
> static unsigned int get_hn(char *url, char *hn) {
>   unsigned int i;
> 
>   for (i = 0; url[i] != '\0' && url[i] != ':' && url[i] != '/'; i++) {
> if(i >= HN_LEN)
>   error_exit("The hostname's length is lower than %d.", HN_LEN);

You mean "larger"

> hn[i] = url[i];
>   }
>   hn[i] = '\0';
> 
>   return i;
> }
> 
> // extract port number
> static void get_port(char *url, char *port, unsigned int *url_i) {
>   unsigned int i;
> 
>   for (i = 0; url[i] != '\0' && url[i] != '/'; i++, (*url_i)++) {
> if('0' <= url[i] && url[i] <= '9') port[i] = url[i];
> else error_exit("Port is invalid");
>   }
>   if(i <= 6) port[i] = '\0';
>   else error_exit("Port number is too long");
> }
> 
> // get http infos in URL
> static void get_info(struct httpinfo *hi, char *url) {
>   unsigned int i = 7, len;
> 
>   if (strncmp(url, "http://;, i)) error_exit("Only HTTP can be supported.");
>   len = get_hn(url+i, hi->hostname);
>   i += len;
> 
>   // get port if exists
>   if (url[i] == ':') {
> i++;
> get_port(url+i, hi->port, );
>   } else strcpy(hi->port, "80");
> 
>   // get uri in URL
>   if (url[i] == '\0') strcpy(hi->path, "/");
>   else if (url[i] == '/') {
> if (strlen(url+i) < PATH_LEN) strcpy(hi->path, url+i);
> else error_exit("The URL path's length is less than %d.", PATH_LEN);
>   } else error_exit("The URL is NOT valid.");
> }
> 
> // connect to any IPv4 or IPv6 server
> static int conn_svr(const char *hostname, const char *port) {
>   struct addrinfo hints, *result, *rp;
>   int sock;
> 
>   memset(, 0, sizeof(struct addrinfo));
>   hints.ai_family = AF_UNSPEC;
>   hints.ai_socktype = SOCK_STREAM;
>   hints.ai_flags = 0;
>   hints.ai_protocol = 0;
> 
>   if ((errno = getaddrinfo(hostname, port, , )))
> error_exit("getaddrinfo: %s", gai_strerror(errno));
> 
>   // try all address list(IPv4 or IPv6) until success
>   for (rp = result; rp; rp = rp->ai_next) {
> if ((sock = socket(rp->ai_family, rp->ai_socktype, rp->ai_protocol))
> == -1) {
>   perror_msg("Socket Error");
>   continue;
> }
> if (connect(sock, rp->ai_addr, rp->ai_addrlen) != -1)
>   break; // succeed in connecting to any server IP 
> else perror_msg("Connect Error");
> close(sock);
>   }
>   freeaddrinfo(result);
>   if(!rp) error_exit("Can not connect to HTTP server");
> 
>   return sock;
> }
> 
> // make HTTP request header field
> static void mk_fld(char *name, char *value) {
>   strcat(toybuf, name);
>   strcat(toybuf, ": ");
>   strcat(toybuf, value);
>   strcat(toybuf, "\r\n");
> }
> 
> // make http request
> static void mk_rq(char *path) {
>   strcpy(toybuf, "GET ");
>   strcat(toybuf, path);
>   strcat(toybuf, " HTTP/1.1\r\n");
> }

Why not sprintf() for these?

> 
> // get http response body starting address and its length
> static char *get_body(size_t len, size_t *body_len) {
>   unsigned int i;
> 
>   for (i = 0; i < len-4; i++)
> if (!strncmp(toybuf+i, "\r\n\r\n", 4)) break;
> 
>   *body_len = len-i-4;
>   return toybuf+i+4;
> }

Why not strstr()?

> 
> void wget_main(void)
> {
>   int sock;
>   struct httpinfo hi;
>   FILE *fp;
>   size_t len, body_len;
>   char *body, *result, *rc, *r_str, ua[18] = "toybox wget/", ver[6];
> 
>   // TODO extract filename to be saved from URL
>   if (!(toys.optflags & FLAG_f))
> help_exit("The filename to be saved should be needed.");
> 

Re: [Toybox] make menuconfig

[Felix Janda]

Rob Landley wrote:
 On 05/02/2015 02:54 PM, enh wrote:
  anyone else having trouble with make menuconfig? getprop and setprop
  aren't showing up for me, and i don't see where to set
  CONFIG_TOYBOX_ON_ANDROID and generated/mkflags segfaults if i have ls
  configured and...
  
  actually, downloading a clean toybox git repo, make defconfig 
  make reproduces the segfault, so at least it's not Android-specific.
  posting early in case anyone's already debugged this...
 
 FYI I had a test wrong in the config parser. It was triggered by
 configuring out a command line option that was the last (leftmost) short
 option in a config that also had bare longopts.

I just wanted to say that not everything is yet fixed. For example
'ls --color' does not work at all. (Seems to be related to the option
parsing.)

Also even on a terminal ls now only shows one directory per line. With the
following hack, the previous behavior is restored in my case.

diff --git a/toys/posix/ls.c b/toys/posix/ls.c
index 9c4d6d3..d783eca 100644
--- a/toys/posix/ls.c
+++ b/toys/posix/ls.c
@@ -344,7 +344,7 @@ static void listfiles(int dirfd, struct dirtree *indir)
 if (len[width]totals[width]) totals[width] = len[width];
   blocks += sort[ul]-st.st_blocks;
 }
-totpad = totals[1]+!!totals[1]+totals[6]+!!totals[6]+totals[7]+!!totals[7];
+totpad = 
totals[1]+!!totals[1]+totals[6]+!!totals[6];//+totals[7]+!!totals[7];
 if (flags  (FLAG_l|FLAG_o|FLAG_n|FLAG_g|FLAG_s)  indir-parent)
   xprintf(total %llu\n, blocks);
   }

Thanks,
Felix
___
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net

Re: [Toybox] make menuconfig

[Felix Janda]

enh wrote:
 On Mon, May 4, 2015 at 10:57 AM, Felix Janda felix.ja...@posteo.de wrote:
[..]
  I just wanted to say that not everything is yet fixed. For example
  'ls --color' does not work at all. (Seems to be related to the option
  parsing.)
 
 works for me. are you sure you have LS_COLOR configured?

CONFIG_LS=y
# CONFIG_LS_Z is not set
CONFIG_LS_COLOR=y

 i did notice one bug though --- the -Z help gets output part way
 through the --color output:
 
 --color  device=yellow  symlink=turquoise/red  dir=blue  socket=purple
 -Z security context
  files: exe=green  suid=red  suidfile=redback  stickydir=greenback
  =auto means detect if output is a tty.
 
 that's weird.

For me the corresponding output for ls --help is (notice the end)

--color  device=yellow  symlink=turquoise/red  dir=blue  socket=purple
 files: exe=green  suid=red  suidfile=redback  stickydir=greenback
 =auto means detect if output is a tty.

ls: Unknown option help

  Also even on a terminal ls now only shows one directory per line. With the
  following hack, the previous behavior is restored in my case.
 
 heh. i thought this was because adb doesn't pass through window size
 information but you're right --- i see this on the desktop too.

Thanks for reproducing the issue.

Felix
___
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net

Re: [Toybox] [PATCH] head: Fix not -123 options

[Felix Janda]

Rob Landley wrote:
 On 03/28/2015 04:31 PM, Felix Janda wrote:
  ---
   toys/posix/head.c | 3 ++-
   1 file changed, 2 insertions(+), 1 deletion(-)
  
  diff --git a/toys/posix/head.c b/toys/posix/head.c
  index 42f945b..8e77b01 100644
  --- a/toys/posix/head.c
  +++ b/toys/posix/head.c
  @@ -56,6 +56,7 @@ void head_main(void)
 if (arg  *arg == '-'  arg[1]) {
   TT.lines = atolx(arg+1);
   toys.optc--;
  +toys.optargs++;
 }
  -  loopfiles(toys.optargs+!!arg, do_head);
  +  loopfiles(toys.optargs, do_head);
   }
 
 Actually I try to avoid modifying toys.optargs in case I expand nofork()
 stuff in future and want to free it on return.

ok, then I don't see how to do is without using another variable.

 When you say fix, could you give me a test case showing the failure?

The question is rather what still works.

The current code ignores the first non-option argument (where -123 don't
count as options). So something like head file will hang.

Sorry for not giving more details before. I viewed it as fixing a very
obvious bug.

Felix
___
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net

[Toybox] grep -x segfault

[Felix Janda]

$ answer=42
$ var=$(for i in $(seq $answer); do printf a; done)
$ printf %s\n\n $var  file
$ toybox grep -x $var file
aa
Segmentation fault


I haven't yet investigated the cause.

42 is the minimal value for the segfault to trigger.

Possibly relevant: I'm using musl libc.


Felix
___
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net

Re: [Toybox] grep -x segfault

[Felix Janda]

This seems to be a bug in musl which will be fixed in musl-1.1.8.

Felix
___
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net

Re: [Toybox] [PATCH] head: Fix not -123 options

[Felix Janda]

Rob Landley wrote:
 On 03/28/2015 04:31 PM, Felix Janda wrote:
  ---
   toys/posix/head.c | 3 ++-
   1 file changed, 2 insertions(+), 1 deletion(-)
  
  diff --git a/toys/posix/head.c b/toys/posix/head.c
  index 42f945b..8e77b01 100644
  --- a/toys/posix/head.c
  +++ b/toys/posix/head.c
  @@ -56,6 +56,7 @@ void head_main(void)
 if (arg  *arg == '-'  arg[1]) {
   TT.lines = atolx(arg+1);
   toys.optc--;
  +toys.optargs++;
 }
  -  loopfiles(toys.optargs+!!arg, do_head);
  +  loopfiles(toys.optargs, do_head);
   }
 
 Actually I try to avoid modifying toys.optargs in case I expand nofork()
 stuff in future and want to free it on return.
 
 When you say fix, could you give me a test case showing the failure?

head -n 1 file

Felix
___
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net

[Toybox] [PATCH] inotifyd: don't ignore first mask

[Felix Janda]

---
 toys/other/inotifyd.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/toys/other/inotifyd.c b/toys/other/inotifyd.c
index f2e11ca..86f1a67 100644
--- a/toys/other/inotifyd.c
+++ b/toys/other/inotifyd.c
@@ -55,7 +55,6 @@ void inotifyd_main(void)
 
 if (!masks) mask = 0xfff; // default to all
 else{
-  *masks++ = 0;
   for (*masks++ = 0; *masks; masks++) {
 i = stridx(masklist, *masks);;
 if (i == -1) error_exit(bad mask '%c', *masks);
-- 
2.0.5
___
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net

Re: [Toybox] sed s/pattern/\newline/

[Felix Janda]

Rob Landley wrote:
 On 01/08/2015 03:38 PM, Felix Janda wrote:
  sed doesn't allow embedded newlines in the replacement for s///, e.g.
  
  s/walrus/wal\
  rus/
 
 Indeed, that's the next sed bug I'm wrestling with right now.
 
  It is not obvious to me how to fix this. There is some code for dealing
  with line continuations but I don't see how to use it.
 
 I've blogged about this but haven't swapped my blog to the new year yet.
 Lemme go do that...
 
 http://landley.net/notes-2015.html#04-01-2015

Thanks.

As to why would anyone do that? That's supported?, POSIX says that
A line can be split by substituting a newline into it.. On the other
hand, '\n' in the replacement text is not specified by POSIX. So scripts
trying to be portable use escaped newlines for splitting lines with sed.
(I hit the bug when trying to build musl.)

 P.S. I hit a bug after that one too, but don't remember what it was at
 the moment. I promoted sed too early. Fixing it before the next release...

Thanks,
Felix
___
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net

Re: [Toybox] sed {N;N;} cmd test failure

[Felix Janda]

Rob Landley wrote:
[..]
 I believe the toybox behavior is right?
 
 We can simplify the above test by removing the p; entries that don't add
 a line to the output:
 
   echo -ne '1\n2\n3\n4\n' | sed -n '1{N;N;d};2,3p;4p'
 
 Toybox just prints one 4, and gnu prints two of them. The difference is
 that 2,3p is triggering on line 4 in gnu, and isn't in toybox.
[..]

Also note that gnu is inconsistent. Both

echo -ne '1\n2\n3\n4\n' | sed -n '1{N;N;d};2p;4p'

and

echo -ne '1\n2\n3\n4\n' | sed -n '1{N;N;d};3p;4p'

only give one 4.

Felix
___
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net

Re: [Toybox] expr horizontal lines in extended description?

[Felix Janda]

Rob Landley wrote:
 Clarification received from The Austin Group.
 
 (Unfortunately the PDF he mentions costs $2500, and is thus completely
 irrelevant in 2014.)

It also seems to be rendered correctly on the POSIX man page.

https://www.kernel.org/pub/linux/docs/man-pages/man-pages-posix/man-pages-posix-2013-a.tar.xz

Felix
___
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net

Re: [Toybox] rm -rf doesn't chmod high enough

[Felix Janda]

Isaac Dunham wrote:
[..]
 A trickier bug is that rm -r dir will skip dir, saying 
 rm: dir: is a directory
 I guess the fix involves rm -r calling rmdir instead of unlink.

unlinkat() also removes directories when given AT_REMOVEDIR.
The following patch makes rm pass the tests and it still asks
me when I try to remove a readonly dir.

-Felix

diff -r 434c4ae19f05 toys/posix/rm.c
--- a/toys/posix/rm.c   Thu Sep 18 18:07:58 2014 -0500
+++ b/toys/posix/rm.c   Sat Sep 20 15:19:09 2014 +0200
@@ -50,7 +50,6 @@
 // Handle chmod 000 directories when -f
 if (faccessat(fd, try-name, R_OK, AT_SYMLINK_NOFOLLOW)) {
   if (toys.optflags  FLAG_f) wfchmodat(fd, try-name, 0700);
-  else goto skip;
 }
 if (!try-again) return DIRTREE_COMEAGAIN;
 using = AT_REMOVEDIR;
___
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net

[Toybox] [PATCH] Fix du tests

[Felix Janda]

See the attached patch. Now all du tests are passed.

Felix
# HG changeset patch
# User Felix Janda felix.ja...@posteo.de
# Date 1411230095 -7200
#  Sat Sep 20 18:21:35 2014 +0200
# Node ID edb1ee770843fb2528e42a77d40fbbdbc96e56ca
# Parent  434c4ae19f0559ab3ba6cd3d0a288b44c3c23949
Fix du test: du symlink - 0\tsymlink

diff -r 434c4ae19f05 -r edb1ee770843 scripts/test/du.test
--- a/scripts/test/du.test	Thu Sep 18 18:07:58 2014 -0500
+++ b/scripts/test/du.test	Sat Sep 20 18:21:35 2014 +0200
@@ -13,13 +13,15 @@
 testing du -s du -k -s du_test 8\tdu_test\n  
 ln -s ../du_2 du_test/xyz
 # du shall count the size of the symbolic link
-# I assume this means the space used to store the link name
-testing du counts symlinks without following du -ks du_test 12\tdu_test\n  
+# The tests assume that like for most POSIX systems symbolic
+# links are stored directly in the inode so that the
+# allocated file space is zero.
+testing du counts symlinks without following du -ks du_test 8\tdu_test\n  
 testing du -L follows symlinks du -ksL du_test 16\tdu_test\n  
 # if -H and -L are specified, the last takes priority
 testing du -HL follows symlinks du -ksHL du_test 16\tdu_test\n  
-testing du -H does not follow unspecified symlinks du -ksH du_test 12\tdu_test\n  
-testing du -LH does not follow unspecified symlinks du -ksLH du_test 12\tdu_test\n  
+testing du -H does not follow unspecified symlinks du -ksH du_test 8\tdu_test\n  
+testing du -LH does not follow unspecified symlinks du -ksLH du_test 8\tdu_test\n  
 testing du -H follows specified symlinks du -ksH du_test/xyz 8\tdu_test/xyz\n  
 
 rm -rf du_test du_2
___
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net

Re: [Toybox] Patches for toybox-0.4.8

[Felix Janda]

luck...@vp.pl wrote:
[..]
 getty.c requires sys/ttydefaults.h. Error of my compilation:
 
 toys/pending/getty.c:168:29: error: 'CERASE' undeclared (first use in 
 this function)
 
 The CERASE macro and more similar macros just are defined at 
 sys/ttydefaults.h in
 musl libc. This header file isn't included any header file in musl libc. 
 I use musl-1.0.3.

For that one constant it might be simpler to replace its only
occurence by 0177 or 127 and leave a comment that that's CERASE.

Felix
___
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net

Re: [Toybox] [NEW TOY] iconv

[Felix Janda]

Rob Landley wrote:
 On 04/13/14 17:18, Felix Janda wrote:
  Rob Landley wrote:
[..]
  Really the only interesting errno case from iconv is illegal sequence.
  The rest just say ran out of input or ran out of output which is
  what you expect from a conversion that's not at the end of the file yet.
  (Ok, truncated sequence is a synonym for illegal sequence if we're not
  at the end of the buffer, which we can special case as at the _start_ of
  the buffer with the memmove logic.)
  
  You mean if we're at the end of the buffer?
 
 No, if we are at the end of the buffer, truncated sequence isn't an
 error. It means the buffer ran out before the sequence did. But if we're
 _not_ at the end of the buffer, it means the

Ah, I was confusing end of buffer and end of file.

 However, if we just zap the parts we handled, do the memmove to the
 front, refill the buffer, and then have the error _again_ that means the
 truncated sequence is invalid, not a problem with running out of data.
 
 (And that means we don't have to care how long the truncated sequence
 is, so we don't care how far from the end of the buffer still counts as
 retrying instead of skipping.)
 
  Hmmm... we should probably pass illegal sequence bytes through. (Pass
  'em through.) Except check if output buffer is full before doing that.
  (Don't have to check inleft nonzero because if inconv() returns illegal
  sequence but used up all the input buffer, that's a libc bug.)
  
  Right...
 
 I think the -c flag controls whether or not to pass them through,
 although posix is going and we refuse to specify the behavior here at
 all because Microsoft paid us money not to.
 
 http://pubs.opengroup.org/onlinepubs/9699919799/utilities/iconv.html

  Where would I get a test file to convert? I just ran a text file through
  it and confirmed it's not making any changes to it, but that doesn't
  mean much. :)
  
  More interesting would be roundtrip encoding some files.
 
 Except that means cat would pass. Not really a test that instills a
 lot of confidence in me...
 
  For testing, I just used an uim tarball[1] with some eucjp encoded files.
  The cleaned up version still seems to work properly.
 
 We can echo -e some snippets. Basically if we convert between utf-8 and
 whatever it is windows uses (latin pi) for like japan or korean or
 something, we'll have shown it Did A Thing. We're not trying to test the
 libc implementation of iconv, just show that we're feeding data into it.

  Even more interesting would be a file with some illegal sequences. I didn't
  test that at all.
 
 The failure paths are always the most interesting thing to test. And the
 most often overlooked...
 
 We'd also want to test retry across 2k boundaries on both input and
 output if we were being serious. _and_ test a file that exactly filled
 up the input and another that exactly filled the output buffer when the
 file ended.
 
 But again, since I dunno what success looks like, I'll wait for somebody
 who does to complain. :)

I think the simplest thing would be to translate between iso-8859-1 and
utf-8. Attached a simple test.

  Error handling looks more sensible. Have you considered that iconv_open()
  might also fail because of insufficient memory.
 
 I looked at doing perror_exit(0) but EINVAL is Invalid argument which
 isn't necessarily enough to figure out what went wrong. As for other
 failure causes:
[..]

Ok thanks, I see why you changed the error message.

  P.S. Posix iconv has several more command line options. -c is easy and
  -s is NOP for us, but I dunno how to do -l.
  
  glibc's doesn't have them. So I guessed that they are not much used.
  Now I see that libiconv has them.
 
 When glibc and posix disagree, posix can potentially win. I'll probably
 do the extra 2 posix options on general principles, and fluff out the
 help text before promoting it.

Yeah, -c and -s look sensible.

Felix
#!/bin/bash

[ -f testing.sh ]  . testing.sh

#testing name command result infile stdin

iso=$(printf '\357')
utf=$(printf '\303\257') # ï
printf a  iso
printf a  utf
for i in $(seq 4096)
do
  printf $iso  iso
  printf $utf  utf
done

testing iconv iconv -f iso-8859-1 iso $(cat utf)  
testing iconv -c iconv -c -f utf-8 iso a  

rm iso utf
___
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net

Re: [Toybox] [PATCH] Factor out dhcpd's and syslogd's main loops into a library function

[Felix Janda]

Rob Landley wrote:
 Taking the remaining holiday time to close old reply windows and such.  
 I've ahd this one open a while... :)
 
 On 09/06/2013 05:35:27 PM, Felix Janda wrote:
[...]
   Are we normally blocking a lot of signals in other commands? Using  
  the
   6 argument version instead of the 5 argument version addresses an  
  issue
   for which existing command?
  
  If a signal occurs during the block it will still be delivered in the  
  next
  pselect loop. pselect implements exactly the signal unblocking I am  
  talking
  about above. We block all signals. But during the signal call some are
  unblocked. When it returns they are again blocked and we can safely  
  check
  what has happened. See the article
  
  https://lwn.net/Articles/176911/
  
  for some discussion on the system call.
 
 My problem with the approach was that the loop really shouldn't mess  
 with signal handlers at all. If we want to fiddle with something fancy  
 there's signalfd.

Ok, I see that in the case that in the case that no signal handling is
needed it shouldn't touch the sigprocmask at all.

In the case signal handling is needed the blocking tames the
asynchronic signals. signalfd would be another (linux specific) option.

 I have an aversion to infrastructure in search of a user. When common  
 code goes into the library, I like to be able to point to at least one  
 existing user, preferably two.

For me the whole point was factoring out the similar code in dhcpd and
syslogd (now there are more possible users in pending), replacing the
self-pipes by something of similar functionality but better suited to
be put into a function.

[...]
  select() adjusts its timespecs on linux. But that's only one possible
  behavior specified by POSIX.
 
 Some not-linuxes don't work like linux, posix is vague enough to allow  
 Windows NT to be certified...
 
 I'm pretty happy sticking with a documented Linux semantic if it's a  
 documented Linux semantic.

Ok. (But it'd be good to note somewhere that a Linux semantic is used.)

  Since we are already doing signals SIGALRM
  seemed more elegant then constantly adjusting the timeout to me.
 
 Having a library I/O function modify global signal handler state is  
 elegant?

more elegant. (Oh, s/then/than/) daemon_main_loop() says how much I
thought of it as a library I/O function at the start. It is meant to
take total control of all signals. I can't think of any application
(in toybox) of both signals and a poll/select loop but a
wait-for-fd-or-signal-loop.

[...] (Discussion on fork and exec.)

Since xexec()
doesn't change the pid the pidfile needs to be unlinked() before
terminating so that xpidfile() doesn't complain.
  
   Um... I agree this is a symptom of a problem. I'm not sure that's  
  the
   right fix. The restart is maintaining state and then getting  
  confused
   by the state it's maintaining...
  
   (I can just see some horrible systemd thing hitting a race window  
  where
   the PID file wasn't there and trying to restart the process...)
  
  Does systemd still need PID files?
 
 I have no idea. It's a horrible thing and I'm trying to avoid it. My  
 point was not assuming behavior on the part of other programs, and  
 leaving a window where something doing inotify() wakes up because an  
 indicator file went away when the process it represents didn't and  
 isn't going to... I try to avoid that sort of halfway state where there  
 can be conflicting assumptions.

So build in some extra logic so that the PID file is only deleted if the
daemon actually exits?

[...]
  So how about making the two helper functions for daemon_main_loop()
  return a value and possibly returning from daemon_main_loop() based on
  that value? Then in syslogd_main() add a infinite loop around (or  
  almost)
  everything.
 
 I need to go clean up dhcpd and systemd before figuring out what the  
 infrastructure should look like. If we're going to do anything fancy  
 with signals I lean towards teaching this loop about signalfd, but  
 probably since Linux can adjust the timeout we should just let Linux  
 adjust the timeout.

Apart from the portability I'd also be very much happy with signalfd.
Again pselect is a replacement for the __self-pipes__. The timeout
is just a side show.

 Lemme finish my current set of cleanups, then get back to this. :)

Take your time.

 I'm finally sort of resurfacing from A) catching up on domestic life  
 after 6 months out of state, B) starting a new job after the  
 kickstarter idea got zero replies.

I have no valuable input on the proposed text for the campaign
(like it as it is) but would really like something like it to happen
(and succeed). The current state is kind of sad. (There is no way
to blame you for it.)

Felix
___
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net

Re: [Toybox] [patch] watch cleanup

[Felix Janda]

Rob Landley wrote:
[...]
 D) Full of unicode characters where multiple input bytes become one  
 output character. (I am assuming fixed width font even for unicode; any  
 non-english speaker think that's a really bad assumption?)

A unicode character can consume 0, 1 or 2 columns on the terminal. You
can use wcwidth() to detect that. 0-width characters are usually combining
characters (In unicode you can write ê as a single character, or as two
characters, one for the hat and one for e.), double-width characters are
for example CJK ideographs, hangul, ...

Felix
___
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net

Re: [Toybox] [New Toy] - Add tftpd

[Felix Janda]

Ashwini Sharma wrote:
 Hi Rob, All,
 
 Attached is the TFTPD implementation.
 Add it to hg, if you find it fine.
 
 Let me know for any comments/improvements.
 
 -Ashwini

Thanks for your submission. Some comments sprinkled into the code below.

Felix

 /* tftpd.c - TFTP server.
  *
  * Copyright 2013 Ranjan Kumar ranjankumar@gmail.com
  * Copyright 2013 Kyungwan Han asura...@gmail.com
  *
  * No Standard.
 
 USE_TFTPD(NEWTOY(tftpd, rcu:, TOYFLAG_BIN))
 
 config TFTPD
   bool tftpd
   default y
   help
 usage: tftpd [-cr] [-u USER] [DIR]
 
 Transfer file from/to tftp server.
 
 -rProhibit upload
 -cAllow file creation via upload
 -uAccess files as USER
 */
 #define FOR_tftpd
 #include toys.h
 #include toynet.h
 
 GLOBALS(
   char *user;
   long sfd;
   struct passwd *pw;
 )
 
 #define TFTPD_BLKSIZE 512  // as per RFC 1350.
 
 // opcodes
 #define TFTPD_OP_RRQ  1  // Read Request  RFC 1350, RFC 2090
 #define TFTPD_OP_WRQ  2  // Write Request RFC 1350
 #define TFTPD_OP_DATA 3  // Data chunkRFC 1350
 #define TFTPD_OP_ACK  4  // Acknowledgement   RFC 1350
 #define TFTPD_OP_ERR  5  // Error Message RFC 1350
 #define TFTPD_OP_OACK 6  // Option acknowledgment RFC 2347
 
 // Error Codes:
 #define TFTPD_ER_NOSUCHFILE  1 // File not found
 #define TFTPD_ER_ACCESS  2 // Access violation
 #define TFTPD_ER_FULL3 // Disk full or allocation exceeded
 #define TFTPD_ER_ILLEGALOP   4 // Illegal TFTP operation
 #define TFTPD_ER_UNKID   5 // Unknown transfer ID
 #define TFTPD_ER_EXISTS  6 // File already exists
 #define TFTPD_ER_UNKUSER 7 // No such user
 #define TFTPD_ER_NEGOTIATE   8 // Terminate transfer due to option negotiation
 
 /* TFTP Packet Formats
  *  Type   Op # Format without header
  * 2 bytesstring1 bytestring1 byte
  * ---
  *  RRQ/  | 01/02 |  Filename  |   0  |Mode|   0  |
  *  WRQ---
  * 2 bytes2 bytes  n bytes
  * -
  *  DATA  | 03|   Block #  |Data|
  * -
  * 2 bytes2 bytes
  * ---
  *  ACK   | 04|   Block #  |
  * 
  * 2 bytes  2 bytes   string 1 byte
  * 
  *  ERROR | 05|  ErrorCode |   ErrMsg   |   0  |
  * 
  */

Thanks for the useful comments.

 static char g_buff[TFTPD_BLKSIZE];
 static char g_errpkt[TFTPD_BLKSIZE];

How about using toybuf and toybuf + TFTP_BLKSIZE for this?

 static void bind_and_connect(struct sockaddr *srcaddr,
 struct sockaddr *dstaddr, socklen_t socklen)
 {
   int set = 1;
 
   TT.sfd = xsocket(dstaddr-sa_family, SOCK_DGRAM, 0);
   if (setsockopt(TT.sfd, SOL_SOCKET, SO_REUSEADDR, (const void *)set,
 sizeof(set))  0) perror_exit(setsockopt failed);
   if (bind(TT.sfd, srcaddr, socklen)) perror_exit(bind);
   if (connect(TT.sfd, dstaddr, socklen)  0)
 perror_exit(can't connect to remote host);
 }

This can be inlined into tftpd_main().

 // Create and send error packet.
 static void send_errpkt(struct sockaddr *dstaddr,
 socklen_t socklen, char *errmsg)
 {
   error_msg(errmsg);
   g_errpkt[1] = TFTPD_OP_ERR;
   strcpy(g_errpkt + 4, errmsg);
   if (sendto(TT.sfd, g_errpkt, strlen(errmsg)+5, 0, dstaddr, socklen)  0)
 perror_exit(sendto failed);
 }

Why does this function not set g_errpkt[3] (the error code)? In some cases
before send_errpkt is called it is set up, in some cases not. In the second
type of case I guess the error code of the previous packet would be used.
Is that intentional?

 // Used to send / receive packets.
 static void do_action(struct sockaddr *srcaddr, struct sockaddr *dstaddr,
 socklen_t socklen, char *file, int opcode, int tsize, int blksize)

This is also only used once in tftpd_main() and can be inlined. That would
however require quite some work.

 {
   int fd, done = 0, retry_count = 12, timeout = 100, len;
   uint16_t blockno = 1, pktopcode, rblockno;
   char *ptr, *spkt, *rpkt;
   struct pollfd pollfds[1];
 
   spkt = xzalloc(blksize + 4);
   rpkt = xzalloc(blksize + 4);
   ptr = spkt+2; //point after opcode.
 
   pollfds[0].fd = TT.sfd;
   // initialize groups, setgid and setuid
   if (TT.pw) {
 if (change_identity(TT.pw)) perror_exit(Failed to change identity);
 endgrent();
   }
 
   if (opcode == TFTPD_OP_RRQ) fd = open(file, O_RDONLY, 0666);
   else fd = open(file, ((toys.optflags  FLAG_c) ?
 (O_WRONLY|O_TRUNC|O_CREAT) : (O_WRONLY|O_TRUNC)) , 0666);
   if (fd  0) {
 g_errpkt[3] = TFTPD_ER_NOSUCHFILE;
 send_errpkt(dstaddr, socklen, can't open file);
 goto CLEAN_APP;
   }
   // For download - blockno will be 1.
   // 1st ACK will be from dst,which will have 

Re: [Toybox] [PATCH] Factor out dhcpd's and syslogd's main loops into a library function

[Felix Janda]

Rob Landley wrote:
 On 09/10/2013 04:22:41 PM, Felix Janda wrote:
  Rob Landley wrote:
   And yet a call to pselect needs its own sigprocmask() call as setup?
   So... why are we using pselect again?
  
  Didn't I say so? Before the call to pselect we block everything.
 
 But why do we do that? (So that only kill -9 will kill daemons?)

So that we can precisely control when signals come in. If a signal occurs
during the time signals are blocked, it is pending to be delivered after
signals are unblocked, that's here during the pselect call. You don't need
kill -9 unless the program decides to go into an infinite loop or read()
from an fd with no data or something like that.

  Then
  pselect atomically unblocks everything, does the select and afterwards
  blocks everything again. So the only thing that can be interrupted by
  signals is pselect. No need to think about race conditions. The  
  relation
  between pselect and select is the same as between ppoll and poll.
 
 There was no need to think about signal handling race conditions in  
 netcat. I haven't read through the dhcpd or syslogd implementation very  
 closely yet, but I don't know what problem you're trying to solve here.

From what I see netcat only uses SIGALRM. syslogd has SIGHUP, SIGTERM,
SIGINT and SIGQUIT - these can occur at any time, during a signal handler,
during a system call... (ok, SIGALRM can also interrupt a system call,
but in netcat it terminates the toy anyway.)

I was trying to replace the self-pipe logic in these toys by something
equivalent. Race conditions occur when you try to make your sighandlers
reentrant by making the sighandlers stubs and doing the real work in the
main loop.

I can't say that I've understood all of syslogd, yet... For dhcpd I've
only tested the signal handling.

  The main reason for not using ppoll is that it is linux only. pselect
  is in POSIX. OpenBSD however doesn't seem to have it... The portable
  way would be to use the self-pipe as before. But then there is even
  more global state...
 
 I don't understand what problem this addresses. I wrote a dnsd that  
 didn't need this (although it just handled udp, not tcp). I've written  
 my own little web servers, my own little vpn and a star topology  
 network redirector... Not remembering ever playing games with signals  
 like this for any of them?

If they didn't do any signal handling at all, that might have been a
reason.

For syslogd one issue might be if two SIGHUPs (telling it to restart in
order to reread its configuration file) occur in quick succession so
that cleanup() is interrupted, and then called a second time which might
cause a double free().

+  for (;;) {
+memcpy(copy, readfds, sizeof(copy));
+ret = pselect(nfds, copy, 0, 0, 0, action.sa_mask);
+if (ret == -1) {
+  if (errno != EINTR) error_exit (pselect);
+  if (timeout  daemon_sig == SIGALRM) alarm(timeout);
+  sighandler(daemon_sig);
  
   Unconditionally calling sighandler(), so we don't have the option  
  of it
   being null and just getting the default behavior for signals. And  
  the
   caller can't have its own use of alarm outside this helper.
  
  Yes, for the broader use it would be good to make the signal handling
  optional (but that's actually the main point of this function). The  
  caller
  can have its own use of alarm if it doesn't use the one built into  
  this
  function.
 
 There's a timeout built into the blocking function. Possibly you have  
 to recalculate the timeout before calling the function again if you  
 care about next scheduled action instead of just it took too long.  
 This is a heavyweight operation?

No. Is alarm() a heavyweight operation?  (Unlike the wrapper function
the linux system call even updates the timeout like select() does.)

   Ok, net win. (I need a lib/pending.h. Right now adding stuff to
   lib/pending.c and then yanking it out again affects the commit  
  history
   of permanent files...)
  
  I guess using signal() instead of sigaction() would save some lines.  
  But I
  didn't want to mix the new pselect with the old signal().
 
 I still don't understand why you're doing all this stuff with signals.
 
   But this patch is doing a lot more than just introducing the new
   library functions and making stuff use them, so I've got to do  
  _more_
   review before I can come to an actual conclusion about whether or  
  not
   to apply it...
  
  It is really just moving stuff around. Luckily the local variables in  
  the
  main functions nicely broke up into the three seperate functions each.
  Anyway, I still need to improve the restarting of syslogd.
  
   I really do appreciate this work. Sorry to give you such a hard time
   about it. I just want to get it _right_...
  
  No problem. Please feel free to wait with coming back to this until  
  after
  the (impending?) release.
 
 I have an extra day in ohio (the linuxfest sunday content wasn't worth  
 staying

Re: [Toybox] [PATCH] tail fixes

[Felix Janda]

Rob Landley wrote:
 Catching up on email while waiting at the airport. (Ohio Linuxfest!)
 
 On 09/06/2013 04:48:10 PM, Felix Janda wrote:
  Rob Landley wrote:
   What The Hurd implements is not. Those religious fanatics are simply
   not relevant, except sometimes as a warning of what _not_ to do.
  
  I just wanted to mention this. It means that we have some more freedom
  in the implementation.
 
 I've since done a checkin that obsoletes most of the analysis here, but  
 there were a few other topics...

Yeah, it seems so.

   pos was a ssize_t because lseek() can return failure, now you've  
  made
   it a size_t which can't be negative. (Do you have something against
   signed numbers?)
  
  It can't be negative. But it can be -1.
 
 Uh-huh.

Just some magic value. (Ok, if sizeof(size_t)  sizeof(int), there is
a problem with this.)

  But actually we should use
  an off_t for pos. One might be curious about the end of a 8GB file
  in a 32bit system.
 
 Is ssize_t not sufficient? (That's what the lseek version is using for  
 pos. The non-lseek version isn't particularly keeping track of pos  
 after grabbing enough characters and lines.)

In my particular example obviously not. The return value of lseek will
be truncated and we will output stuff from somewhere in the file but
not at its end.

 Also, the argument parsing logic is using long for the numbers we  
 input. This means a 32 bit system can't accept a -c larger than 2 gigs.  
 (Known limitation, didn't want to complicate the code to cope with 32  
 bit systems when everything and its dog has 64 bit variants these days,  
 and the year 2038 problem is going to flush out the remaining 32-bit  
 systems in 25 years. Support them, yes. Make them handle large numbers  
 in all cases, not necessarily.)
 
 (The real hiccup with argument parsing is that  
 sizeof(long)==sizeof(pointer) so I allocate pointer sized slots at the  
 start of GLOBALS() to save arguments into. I can jump through hoops to  
 get around that, but the simple approach is to wait for the switch to  
 64 bit arm.)

Thanks for mentioning this known limitation. Someone commented on
fallocate on github. It should also use off_t but the argparsing logic can't
handle it anyway. There might be some reasons to create large files with
this toy on 32bit systems.

atolx() seems to have pretty undefined behavior with large numbers?

  The previous code had the assumption that every chunk is terminated by
  a newline (which is utterly wrong). This is the simplest  
  implementation
  I could come up. I'll try to explain further things below.
  
  After spending quite some time with this reply I see the simple
  (less efficient (in terms of number of instructions)) algorithm.
 
 I think I fixed that, but hopefully if there are still problems they'll  
 be mentioned in one of the other pending mails I'm reading towards...

It looks good to me.

   Actually we don't have an exclusion in the option string for [!nc]  
  and
   I vaguely recall I was trying to make it work if you specified both  
  at
   the same time. (So it showed you that many characters from the end
   _plus_ that many lines before that point.)
  
  I didn't know that. Could you specify it in more detail (the order  
  matters)?
 
 It was a bad idea, and the attempt at implementing it was broken in  
 several places. I made this implementation produce the same results  
 other ones do instead.

Ok.

   Let's see, assuming the actual bugs were the uninitialized next  
  pointer
   in the last list entry and the list entries being added in the wrong
   order (so only the last list entry every showed), so if I separate  
  the
   count and line codepaths into an if/else but keep the head/tail
   traversing design without this sideband line array...
  
  There were more. (Just try the previous lseek logic with something big
  containing no newlines.)
 
 Hopefully the current version works. (I had the test suite do a larger  
 test file. Probably didn't hit everything.)

Did a few tests. Seems good.

   You see why I just did a doubly linked list? That way I had one list
   pointer fed to the function and the function could use it to find  
  the
   end of the list, but I _didn't_ have different list and list head
   types. The downside is each node has an extra pointer, for which I  
  get
   the ability to traverse the list backwards, even if I never use it.)
  
  It's still a bit confusing to me. list always points to the head and
  list-prev points to the tail?
 
 Yes, it's a circular doubly linked list. Fairly common data structure,  
 the kernel uses 'em all over the place. (You have a head pointer, from  
 there you can traverse in either direction, and when you hit it again  
 you're done.)
 
 This sort of thing is actually why the kernel has magic PID 1 and  
 rootfs entries that never go away: the head pointer points to them and  
 if that entry is never removed, you never have to move the head pointer  
 which makes some

Re: [Toybox] - Add route

[Felix Janda]

Ashwini Sharma wrote:
 On Wed, Sep 11, 2013 at 5:30 AM, Felix Janda felix.ja...@posteo.de wrote:
 
  Ashwini Sharma wrote:
[...]
   Didn't use show_help(), as this doesn't exit whereas an exit is intended
   for route.
   may be show_help() can be updated to do that.
 
  For this there is toys.exithelp. Set it to one and then call error_exit().
 
 
 Yup, show_route_help, already does that. It was to be called many times,
 hence made a function
 show_route_help().

Oh, sorry for not looking at your patch. (Could you maybe get your MUA to 
produce
text/x-patch attachments?)

You're right it's called quite often. It seems that the command is doing
argparsing in several places so that you can't just set toys.exithelp in the
beginning and reset it when you're done with the argparsing.

Felix
___
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net

Re: [Toybox] - Add route

[Felix Janda]

Ashwini Sharma wrote:
 On Sat, Sep 7, 2013 at 11:47 PM, Isaac ibid...@gmail.com wrote:
 
  On Mon, Sep 02, 2013 at 11:15:14AM +0900, Ashwini Sharma wrote:
   HI Rob  list,
  
 Attached is the patch for _route_ command. It does display, add and del
   functions for routing tables.
   Have a look at it.
  
   regards,
   Ashwini Kumar
 
  OK, I took a quick look through it.
  First, thanks for doing route.
 
  I note that show_route_help is identical to show_help() apart from the
  error message.  That's probably not needed.
 
 
 Didn't use show_help(), as this doesn't exit whereas an exit is intended
 for route.
 may be show_help() can be updated to do that.

For this there is toys.exithelp. Set it to one and then call error_exit().

Felix
___
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net

Re: [Toybox] [PATCH] Factor out dhcpd's and syslogd's main loops into a library function

[Felix Janda]

Rob Landley wrote:
 On 09/01/2013 08:54:27 AM, Felix Janda wrote:
  diff -r 587b7572aac2 -r 752d22ece0fe lib/lib.h
  --- a/lib/lib.h Sun Sep 01 15:13:58 2013 +0200
  +++ b/lib/lib.h Sun Sep 01 15:25:08 2013 +0200
  @@ -201,4 +201,7 @@
   char  *astrcat (char *, char *);
   char *xastrcat (char *, char *);
  
  +// daemon helper functions
   void daemonize(void);
  +void daemon_main_loop(int nfds, fd_set *readfds, long timeout, int  
  *signals,
  + void (*sighandler)(int), void  
  (*selecthandler)(fd_set *fds));
 
 It's not really daemon, is it? Netcat isn't a daemon, less isn't a  
 daemon, tail -f isn't a daemon... All of 'em could potentially benefit  
 from a select loop. It's pretty much just select_loop().

I hadn't thought of it that general. Right now it unconditionally sets
up signals but that could be easily fixed.

 Also, the current code in netcat is using poll(), not select(). (Array  
 of file descriptors vs magic sized bitfield where you have to calculate  
 nfds, which _this_ function is asking the caller to do. Neither one  
 actually scales to thousands of filehandles gracefully, but if that's  
 an issue it's probably not common library code the one or two  
 filehandle case is going to want to use...)

  diff -r 587b7572aac2 -r 752d22ece0fe lib/pending.c
  --- a/lib/pending.c Sun Sep 01 15:13:58 2013 +0200
  +++ b/lib/pending.c Sun Sep 01 15:25:08 2013 +0200
  @@ -92,3 +92,41 @@
 dup2(fd, 2);
 if (fd  2) close(fd);
   }
  +
  +static int daemon_sig;
  +
 
 Gratuitous global variable. I've been trying to eliminate those. (I  
 just added libbuf analogous to toybuf but for use by lib/*.c,  
 presumably it could use that. But... is it worth doing?)

It seems inevitable to me to keep some global state for the signal
handling.

  +static void daemon_sighandler(int sig)
  +{
  +  daemon_sig = sig;
  +}
  +
 
 If we weren't messing with alarm, and instead of using the timeouts  
 built into select, would signals be our problem at all? (Might have to  
 calculate a timeout, but we do that anyway when actual data comes in if  
 we need a periodic action, and you can get data with a bad checksum  
 that causes spurious data arrived, no wait it didn't wakeups anyway.  
 Getting interrupted happens, we need to cope.)
 
 (And still confused about the syscall having a timeout but us not using  
 it...)

SIG_ALRM was an afterthought when I realized that interrupting pselect
messes up the timeouts.

pselect is a replacement for the select + signal self-pipe.

  +/*
  + * main loop for daemons listening to signals and file handles
  +*/
  +void daemon_main_loop(int nfds, fd_set *readfds, long timeout, int  
  *signals,
  + void (*sighandler)(int), void  
  (*selecthandler)(fd_set *fds))
  +{
  +  fd_set copy;
  +  sigset_t sigmask;
  +  struct sigaction action;
  +  int *i, ret;
  +
  +  sigemptyset(sigmask);
  +  action.sa_handler = daemon_sighandler;
  +  sigemptyset(action.sa_mask);
  +  action.sa_flags = 0;
  +  if (timeout) alarm(timeout);
  +  for (i = signals; *i; i++) sigaddset(sigmask, *i);
  +  sigprocmask(SIG_SETMASK, sigmask, 0);
  +  for (i = signals; *i; i++) sigaction(*i, action, 0);
 
 To quote from the ppoll man page:
 
 Other than the difference in the precision of the timeout  
 argument, the
 following ppoll() call:
 
 ready = ppoll(fds, nfds, timeout_ts, sigmask);
 
 is equivalent to atomically executing the following calls:
 
 sigset_t origmask;
 int timeout;
 
 timeout = (timeout_ts == NULL) ? -1 :
   (timeout_ts.tv_sec * 1000 + timeout_ts.tv_nsec /  
 100);
 sigprocmask(SIG_SETMASK, sigmask, origmask);
 ready = poll(fds, nfds, timeout);
 sigprocmask(SIG_SETMASK, origmask, NULL);
 
 And yet a call to pselect needs its own sigprocmask() call as setup?  
 So... why are we using pselect again?

Didn't I say so? Before the call to pselect we block everything. Then
pselect atomically unblocks everything, does the select and afterwards
blocks everything again. So the only thing that can be interrupted by
signals is pselect. No need to think about race conditions. The relation
between pselect and select is the same as between ppoll and poll.

The main reason for not using ppoll is that it is linux only. pselect
is in POSIX. OpenBSD however doesn't seem to have it... The portable
way would be to use the self-pipe as before. But then there is even
more global state...

  +  for (;;) {
  +memcpy(copy, readfds, sizeof(copy));
  +ret = pselect(nfds, copy, 0, 0, 0, action.sa_mask);
  +if (ret == -1) {
  +  if (errno != EINTR) error_exit (pselect);
  +  if (timeout  daemon_sig == SIGALRM) alarm(timeout);
  +  sighandler(daemon_sig);
 
 Unconditionally calling sighandler(), so we don't have the option of it  
 being null and just getting the default behavior for signals. And the  
 caller

[Toybox] [PATCH] Factor out dhcpd's and syslogd's main loops into a library function

[Felix Janda]

Both syslogd and dhcpd wait for sockets, signals or a timeout. Currently,
they use select() with its timeout to listen to the sockets and a
self-pipe for the signals.

The new library function instead uses pselect() without a timeout and
alarm(). With pselect() signals are only unblocked during this system
call; so the xwrappers don't need to be aware about interrupted syscalls.
alarm() is used instead of the timeout argument to pselect() so that
the length of the intervals between timeouts is not affected by being
interrupted.

On SIGHUP syslogd restarts. Previously everything relevant was contained
in syslogd_main() and the restarting was implemented by a goto. Now a
longjmp() would be necessary. I instead used an xexec(). Since xexec()
doesn't change the pid the pidfile needs to be unlinked() before
terminating so that xpidfile() doesn't complain. Furthermore if not run
in the foreground syslogd will daemonize() again when restarting. Could
one detect in daemonize() whether daemonizing is necessary?

Felix

# HG changeset patch
# User Felix Janda felix.ja...@posteo.de
# Date 1378041908 -7200
# Node ID 752d22ece0fe3fa9763fd0b6e231e653cc173890
# Parent  587b7572aac2a64988503458155e1f2546672525
Factor out daemon select() loops into a library function

diff -r 587b7572aac2 -r 752d22ece0fe lib/lib.h
--- a/lib/lib.h Sun Sep 01 15:13:58 2013 +0200
+++ b/lib/lib.h Sun Sep 01 15:25:08 2013 +0200
@@ -201,4 +201,7 @@
 char  *astrcat (char *, char *);
 char *xastrcat (char *, char *);
 
+// daemon helper functions
 void daemonize(void);
+void daemon_main_loop(int nfds, fd_set *readfds, long timeout, int *signals,
+ void (*sighandler)(int), void (*selecthandler)(fd_set *fds));
diff -r 587b7572aac2 -r 752d22ece0fe lib/pending.c
--- a/lib/pending.c Sun Sep 01 15:13:58 2013 +0200
+++ b/lib/pending.c Sun Sep 01 15:25:08 2013 +0200
@@ -92,3 +92,41 @@
   dup2(fd, 2);
   if (fd  2) close(fd);
 }
+
+static int daemon_sig;
+
+static void daemon_sighandler(int sig)
+{
+  daemon_sig = sig;
+}
+
+/*
+ * main loop for daemons listening to signals and file handles
+*/
+void daemon_main_loop(int nfds, fd_set *readfds, long timeout, int *signals,
+ void (*sighandler)(int), void (*selecthandler)(fd_set *fds))
+{
+  fd_set copy;
+  sigset_t sigmask;
+  struct sigaction action;
+  int *i, ret;
+
+  sigemptyset(sigmask);
+  action.sa_handler = daemon_sighandler;
+  sigemptyset(action.sa_mask);
+  action.sa_flags = 0;
+  if (timeout) alarm(timeout);
+  for (i = signals; *i; i++) sigaddset(sigmask, *i);
+  sigprocmask(SIG_SETMASK, sigmask, 0);
+  for (i = signals; *i; i++) sigaction(*i, action, 0);
+
+  for (;;) {
+memcpy(copy, readfds, sizeof(copy));
+ret = pselect(nfds, copy, 0, 0, 0, action.sa_mask);
+if (ret == -1) {
+  if (errno != EINTR) error_exit (pselect);
+  if (timeout  daemon_sig == SIGALRM) alarm(timeout);
+  sighandler(daemon_sig);
+} else selecthandler(copy);
+  }
+}
diff -r 587b7572aac2 -r 752d22ece0fe toys/pending/dhcpd.c
--- a/toys/pending/dhcpd.c  Sun Sep 01 15:13:58 2013 +0200
+++ b/toys/pending/dhcpd.c  Sun Sep 01 15:25:08 2013 +0200
@@ -206,11 +206,9 @@
 {msstaticroutes , DHCP_STCRTS | 0xf9, NULL, 0},
 };
 
-struct fd_pair { int rd; int wr; };
 static server_config_t gconfig;
 static server_state_t gstate;
 static uint8_t infomode;
-static struct fd_pair sigfd;
 static int constone = 1;
 
 // calculate options size.
@@ -316,29 +314,6 @@
   }
 }
 
-// Generic signal handler real handling is done in main funcrion.
-static void signal_handler(int sig)
-{
-  unsigned char ch = sig;
-  if (write(sigfd.wr, ch, 1) != 1) dbg(can't send signal\n);
-}
-
-// signal setup for SIGUSR1 SIGTERM
-static int setup_signal()
-{
-  if (pipe((int *)sigfd)  0) {
-dbg(signal pipe failed\n);
-return -1;
-  }
-  fcntl(sigfd.wr , F_SETFD, FD_CLOEXEC);
-  fcntl(sigfd.rd , F_SETFD, FD_CLOEXEC);
-  int flags = fcntl(sigfd.wr, F_GETFL);
-  fcntl(sigfd.wr, F_SETFL, flags | O_NONBLOCK);
-  signal(SIGUSR1, signal_handler);
-  signal(SIGTERM, signal_handler);
-  return 0;
-}
-
 // String STR to UINT32 conversion strored in VAR
 static int strtou32(const char *str, void *var)
 {
@@ -1073,15 +1048,114 @@
   return ret;
 }
 
+static void sig_handler(int sig)
+{
+  switch (sig) {
+case SIGALRM:
+  write_leasefile();
+  if (get_interface(gconfig.interface, gconfig.ifindex, 
gconfig.server_nip, gconfig.server_mac)0)
+perror_exit(Interface lost %s\n, gconfig.interface);
+  gconfig.server_nip = htonl(gconfig.server_nip);
+  break;
+case SIGUSR1:
+  infomsg(infomode, Received SIGUSR1);
+  write_leasefile();
+  break;
+case SIGTERM:
+  infomsg(infomode, Received SIGTERM);
+  write_leasefile();
+  unlink(gconfig.pidfile);
+  exit(0);
+  break;
+  }
+}
+
+static void select_handler(fd_set *rfds)
+{
+  uint8_t *optptr, msgtype = 0;
+  uint32_t serverid = 0, requested_nip = 0;
+  uint32_t reqested_lease = 0

[Toybox] [PATCH] tail fixes

[Felix Janda]

This should now hopefully fix the earlier segfaults.

In the case that lseek doesn't work and we count from the end
the algorithms for tail -n and tail -c are now more separate.
(tail -c is now more efficient since it adds lenghts instead of
counting bytes.)


POSIX tail BTW does only accept one input file. tail is not specified
by LSB. I guess it is convenient when combined with -f to watch
multiple files.

For tail -f, how about putting the fds into an array and then using
select() to watch the fds?

GNU and busybox print the header again when the file last read from
changes. They behave differently when a file occurs on the command
line more than once.

# HG changeset patch
# User Felix Janda felix.ja...@posteo.de
# Date 1377945041 -7200
# Node ID 3cd61d16aabec82505da96a142663cedbba8d12a
# Parent  9a059e9b3a17a372fc8b225f512af57c72f4eeaa
tail: Some fixes

- Rewrite most of the not lseek() logic
- Change meaning of len in line_list
- Use single instead of double linked list

diff -r 9a059e9b3a17 -r 3cd61d16aabe toys/posix/tail.c
--- a/toys/posix/tail.c Fri Aug 30 20:09:10 2013 +0200
+++ b/toys/posix/tail.c Sat Aug 31 12:30:41 2013 +0200
@@ -38,19 +38,20 @@
 )
 
 struct line_list {
-  struct line_list *next, *prev;
+  struct line_list *next;
   char *data;
-  int len;
+  size_t len;
 };
 
-static struct line_list *get_chunk(int fd, int len)
+static struct line_list *get_chunk(int fd, size_t len)
 {
   struct line_list *line = xmalloc(sizeof(struct line_list)+len);
 
-  line-data = ((char *)line) + sizeof(struct line_list);
+  line-data = (char*)line + sizeof(struct line_list);
   line-len = readall(fd, line-data, len);
+  line-next = 0;
 
-  if (line-len  1) {
+  if (line-len + 1  2) {
 free(line);
 return 0;
   }
@@ -61,7 +62,9 @@
 static void dump_chunk(void *ptr)
 {
   struct line_list *list = ptr;
-  xwrite(1, list-data, list-len);
+  size_t len = list-len - (list-data - (char*)list - sizeof(struct 
line_list));
+
+  xwrite(1, list-data, len);
   free(list);
 }
 
@@ -71,11 +74,11 @@
 static int try_lseek(int fd, long bytes, long lines)
 {
   struct line_list *list = 0, *temp;
-  int flag = 0, chunk = sizeof(toybuf);
-  ssize_t pos = lseek(fd, 0, SEEK_END);
+  int flag = 0;
+  size_t chunk = sizeof(toybuf), pos = lseek(fd, 0, SEEK_END);
 
   // If lseek() doesn't work on this stream, return now.
-  if (pos0) return 0;
+  if (pos == -1) return 0;
 
   // Seek to the right spot, output data from there.
   if (bytes) {
@@ -88,40 +91,36 @@
 
   bytes = pos;
   while (lines  pos) {
-int offset;
+size_t offset;
 
 // Read in next chunk from end of file
-if (chunkpos) chunk = pos;
+if (chunk  pos) chunk = pos;
 pos -= chunk;
 if (pos != lseek(fd, pos, SEEK_SET)) {
   perror_msg(seek failed);
   break;
 }
 if (!(temp = get_chunk(fd, chunk))) break;
-if (list) list-next = temp;
+if (list) temp-next = list;
 list = temp;
 
 // Count newlines in this chunk.
 offset = list-len;
 while (offset--) {
   // If the last line ends with a newline, that one doesn't count.
-  if (!flag) {
-flag++;
-
-continue;
-  }
+  if (!flag) flag++;
 
   // Start outputting data right after newline
-  if (list-data[offset] == '\n'  !++lines) {
+  else if (list-data[offset] == '\n'  !++lines) {
 offset++;
 list-data += offset;
-list-len -= offset;
 
-break;
+goto done;
   }
 }
   }
 
+done:
   // Output stored data
   llist_traverse(list, dump_chunk);
 
@@ -143,58 +142,54 @@
   // Are we measuring from the end of the file?
 
   if (bytes0 || lines0) {
-struct line_list *list = 0, *new;
+struct line_list *head = 0, *tail, *new;
+// circular buffer of lines
+struct {
+  char *start;
+  struct line_list *inchunk;
+} *l = xzalloc(2*-lines*sizeof(void*));
+int i = 0, flag = 0;
+size_t count, len = bytes;
 
 // The slow codepath is always needed, and can handle all input,
 // so make lseek support optional.
-if (CFG_TAIL_SEEK  try_lseek(fd, bytes, lines));
+if (CFG_TAIL_SEEK  try_lseek(fd, bytes, lines)) return;
 
 // Read data until we run out, keep a trailing buffer
-else for (;;) {
-  int len, count;
+for (;;) {
   char *try;
 
   if (!(new = get_chunk(fd, sizeof(toybuf break;
   // append in order
-  dlist_add_nomalloc((void *)list, (struct double_list *)new);
+  if (head) tail-next = new;
+  else head = new;
+  tail = new;
 
-  // Measure new chunk, discarding extra data from buffer
-  len = new-len;
   try = new-data;
-  for (count=0; countlen; count++) {
-if ((toys.optflags  FLAG_c)  bytes) {
-  bytes++;
-  continue;
+  if (lines) for (count = 0; count  new-len; count++, try++) {
+if (flag) { // last char was a newline
+while (l[i].inchunk  (l[i].inchunk!=head)) 
free(llist_pop(head));
+l[i

[Toybox] [CLEANUP] syslogd - Pass 6 + Questions

[Felix Janda]

I had broken resolve_config() and the daemonizing (didn't notice the
latter because I've always run it in the foreground.)...


syslogd doesn't seem very standardized between linux, the BSDs and MAC.
Let me consider the options to specify which sockets to listen to:

toybox:

syslogd -p defsock -a sock1:sock2

OpenBSD, inetutils:

syslogd -p defsock -a sock1 -a sock2

FreeBSD:

syslogd -p defsock -l 600:sock1 -l 666:sock2

NetBSD:

syslogd -p sock1 -p sock2


The NetBSD version looks IMO the nicest here. OTOH the inetutils
version should be the most widely used on linux.


Then I'm not so sure about behavior in error conditions. Right now
when creating one socket fails, only an error message is printed to
stderr and the command doesn't exit unless it can't open any
socket at all. It exits when there's a problem with the config file
or with initializing network logging. If it can't open a usual
logging file, it opens /dev/console instead.

Is there a system behind this? How about just failing when
something goes wrong before entering the select loop? Do we care
about cleaning up the socket files when we crash?


# HG changeset patch
# User Felix Janda felix.ja...@posteo.de
# Date 1377546933 -7200
# Node ID 3a2fbd46c2f33ae0a6148f677887c5763e3c6d69
# Parent  d177bfe52a4b94b4d24806c0b3b9423d0e55b53f
syslogd: cleanup

- fix bugs introduced in the cleanups
- inline addrfds() and open_unix_socks() and simplify them
- use xpidfile()
- remove isNetwork from struct logfile
- invert the meaning of facility and level in struct logfile so
  that they are automatically correctly initialized
- fix memory leak regarding the filenames of logfiles
- TT.sd was unused

diff -r d177bfe52a4b -r 3a2fbd46c2f3 lib/lib.h
--- a/lib/lib.h Sat Aug 24 12:04:45 2013 +0200
+++ b/lib/lib.h Mon Aug 26 21:55:33 2013 +0200
@@ -123,6 +123,7 @@
 void xsetuid(uid_t uid);
 char *xreadlink(char *name);
 long xparsetime(char *arg, long units, long *fraction);
+void xpidfile(char *name);
 
 // lib.c
 void verror_msg(char *msg, int err, va_list va);
diff -r d177bfe52a4b -r 3a2fbd46c2f3 lib/xwrap.c
--- a/lib/xwrap.c   Sat Aug 24 12:04:45 2013 +0200
+++ b/lib/xwrap.c   Mon Aug 26 21:55:33 2013 +0200
@@ -452,7 +452,7 @@
   sprintf(pidfile, /var/run/%s.pid, name);
   // Try three times to open the sucker.
   for (i=0; i3; i++) {
-fd = open(pidfile, O_CREAT|O_EXCL, 0644);
+fd = open(pidfile, O_CREAT|O_EXCL|O_WRONLY, 0644);
 if (fd != -1) break;
 
 // If it already existed, read it.  Loop for race condition.
diff -r d177bfe52a4b -r 3a2fbd46c2f3 toys/pending/syslogd.c
--- a/toys/pending/syslogd.cSat Aug 24 12:04:45 2013 +0200
+++ b/toys/pending/syslogd.cMon Aug 26 21:55:33 2013 +0200
@@ -49,7 +49,6 @@
 struct logfile {
   struct logfile *next;
   char *filename;
-  int isNetwork;
   uint32_t facility[8];
   uint8_t level[LOG_NFACILITIES];
   int logfd;
@@ -69,8 +68,6 @@
 
   struct unsocks *lsocks;  // list of listen sockets
   struct logfile *lfiles;  // list of write logfiles
-  fd_set rfds;// fds for reading
-  int sd;// socket for logging remote messeges.
   int sigfd[2];
 )
 
@@ -94,53 +91,6 @@
   return itoa(val);
 }
 
-// Adds opened socks to rfds for select()
-static int addrfds(void)
-{
-  struct unsocks *sock = TT.lsocks;
-  int ret = 0;
-  FD_ZERO(TT.rfds);
-
-  for (; sock; sock = sock-next) {
-if (sock-sd  2) {
-  FD_SET(sock-sd, TT.rfds);
-  ret = sock-sd;
-}
-  }
-  FD_SET(TT.sigfd[0], TT.rfds);
-  return (TT.sigfd[0]  ret) ? TT.sigfd[0] : ret;
-}
-
-/*
- * initializes unsock_t structure
- * and opens socket for reading
- * and adds to global lsock list.
- */
-static int open_unix_socks(void)
-{
-  struct unsocks *sock;
-  int ret = 0;
-
-  for(sock = TT.lsocks; sock; sock = sock-next) {
-sock-sdu.sun_family = AF_UNIX;
-strcpy(sock-sdu.sun_path, sock-path);
-sock-sd = socket(AF_UNIX, SOCK_DGRAM, 0);
-if (sock-sd  0) {
-  perror_msg(OPEN SOCKS : failed);
-  continue;
-}
-unlink(sock-sdu.sun_path);
-if (bind(sock-sd, (struct sockaddr *) sock-sdu, sizeof(sock-sdu))) {
-  perror_msg(BIND SOCKS : failed sock : %s, sock-sdu.sun_path);
-  close(sock-sd);
-  continue;
-}
-chmod(sock-path, 0777);
-ret++;
-  }
-  return ret;
-}
-
 /*
  * recurses the logfile list and resolves config
  * for evry file and updates facilty and log level bits.
@@ -182,17 +132,17 @@
 lvl++;
   }
 }
-if (bits  1) levval = 0xff;
-if (lvl) {
+if (bits  2) levval = 0xff;
+if (*lvl) {
   if ((i = logger_lookup(1, lvl)) == -1) return -1;
-  levval |= (bits  2) ? LOG_MASK(i) : LOG_UPTO(i);
-  if (bits  4) levval = ~levval;
+  levval |= (bits  4) ? LOG_MASK(i) : LOG_UPTO(i);
+  if (bits  8) levval = ~levval;
 }
 
 for (i = 0, set = levval; set; set = 1, i++)
-  if (set  0x1) file-facility[i] |= facval;
+  if (set  0x1) file-facility[i] |= ~facval;
 for (i = 0; i  LOG_NFACILITIES

Re: [Toybox] [CLEANUP] syslogd - Pass 5

[Felix Janda]

Rob Landley wrote:
 On 08/24/2013 05:20:06 AM, Felix Janda wrote:
[...]
  If none of these have been applied yet, I could prepare new patches
  which don't leave the toy broken in between them.
 
 I like the incremental nature of it, but I'm still not sure why patch 4  
 _doesn't_ seem to break defconfig. (C is weird about structs it hasn't  
 seen the declaration for yet...)

We had the same discussion for find and I copied from there. (Look at
the mails around the 20th of April.)

 I need to frown at it a bit more...
 
 Rob
___
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net

Re: [Toybox] grep corner cases

[Felix Janda]

Rob Landley wrote:
 On 08/20/2013 12:29:08 PM, Felix Janda wrote:
  Rob Landley wrote:
   On 08/19/2013 02:26:55 PM, Felix Janda wrote:
Hi,
   
I saw the comment in changeset 1017 on possible bugs in GNU grep.
   
The failing tests are for me:
   
testing grep -vo grep -vo one input two\nthree\n
onetwoonethreeone\n 
testing grep -Fx '' grep -Fx '' input one one one\n one one
one\n 
testing grep -F -e blah -e '' grep -F -e blah -e '' input one
one one\n \
  one one one\n 
   
-o is a GNU extension making grep only output the matched parts of
each
matched line. So since -v inverts the set of all matched lines  
  grep
-vo
should not output anything.
  
   Does it invert the set of matched _lines_, or does it invert the  
  match
   criteria? I made it so that:
  
   -v
   Select lines not matching any of the specified patterns. If the
   -v option is not specified, selected lines shall be those that
   match any of the specified patterns.
  
  Does sound to me like the former. This fits the line based nature of  
  many
  of the POSIX tools. It however doesn't make grep -vo very useful.
 
 Posix does not have the -o option. The -o option is not line based.  
 This is about the effect of other options on the -o option.

Since you still can't match things spanning lines -o doesn't seem to
make grep into a byte based tool.

What should

echo on | grep -vo a

output? If you say that the sense of matching is inverted shouldn't
the output be


o
n
on

or some permutation thereof? The current output of toybox is pretting
interesting...

  echo oneandtwoandthree | grep -ov
  
  Shouldn't it be
  
  echo oneandtwoandthree | grep -ov and
 
 Yes.
 
   would produce:
  one
  two
  three
  
   (I pondered onetwothree but that's not how -o without -v works...)
  
   The reason there are deviating test cases to consider is I'm not  
  taking
   what gcc does as an inherent definition of the right thing to  
  do.
  
  But maybe it's a reason to spend some thought on the validity of the
  test case and maybe do some testing against other implementations. For
  example busybox grep also doesn't output anything.
 
 What other implementations? -o is a gnu/dammit extension.

Did you read the last paragraph you are quoting carefully?

Just FYI: obase doesn't contain grep.

   That implies that
  
  echo one | grep -F -e walrus -e ''
  
   Should match one, but with the gnu/dammit version it only does so
   _without_ the -F. Or with -F and just one argument...
  
  busybox grep also agrees with you.
 
 And it's inconsistent:
 
$ echo hello | grep -F -e ''
hello
$ echo hello | grep -F -e 'one' -e ''
$ echo hello | grep -e 'one' -e ''
hello
 
 (That's pretty clearly a bug. If you're wondering why I'm not slavishly  
 copying gnu/dammit behavior it's because they're not very good at this.  
 They've just had a very large testing base reporting bugs for a very  
 long time.)

I don't care specifically about the GNU version. With GNU grep lying in
/usr/bin it's just too convenient to run tests against it.

Combined with -x IMO
it should only match empty lines.
  
   I asked the Austin guys how -F and -x interact. It's not obvious to  
  me
   from reading it.
  
  Sounds like a good idea.
 
 They said -x nukes '' being a wildcard (which matches the non-F version  
 of the gnu/dammit behavior), but BSD gets it wrong:
 
 
 http://permalink.gmane.org/gmane.comp.standards.posix.austin.general/7923

Thanks.

 Rob

Felix
___
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net

Re: [Toybox] grep corner cases

[Felix Janda]

Rob Landley wrote:
 On 08/19/2013 02:26:55 PM, Felix Janda wrote:
  Hi,
  
  I saw the comment in changeset 1017 on possible bugs in GNU grep.
  
  The failing tests are for me:
  
  testing grep -vo grep -vo one input two\nthree\n  
  onetwoonethreeone\n 
  testing grep -Fx '' grep -Fx '' input one one one\n one one  
  one\n 
  testing grep -F -e blah -e '' grep -F -e blah -e '' input one  
  one one\n \
one one one\n 
  
  -o is a GNU extension making grep only output the matched parts of  
  each
  matched line. So since -v inverts the set of all matched lines grep  
  -vo
  should not output anything.
 
 Does it invert the set of matched _lines_, or does it invert the match  
 criteria? I made it so that:

 -v 

 Select lines not matching any of the specified patterns. If the
 -v option is not specified, selected lines shall be those that
 match any of the specified patterns.   
  

Does sound to me like the former. This fits the line based nature of many
of the POSIX tools. It however doesn't make grep -vo very useful.

echo oneandtwoandthree | grep -ov

Shouldn't it be

echo oneandtwoandthree | grep -ov and

 would produce:
one
two
three
 
 (I pondered onetwothree but that's not how -o without -v works...)

 The reason there are deviating test cases to consider is I'm not taking  
 what gcc does as an inherent definition of the right thing to do.

But maybe it's a reason to spend some thought on the validity of the
test case and maybe do some testing against other implementations. For
example busybox grep also doesn't output anything.

  -F turns on fixed string matching so '' is no longer the empty regex
  which matches everything, but the empty string.
 
 http://pubs.opengroup.org/onlinepubs/9699919799/utilities/grep.html
 
 -F  Match using fixed strings. Treat each pattern specified as a string
  instead of a regular expression. If an input line contains any of  
 the
  patterns as a contiguous sequence of bytes, the line shall be  
 matched.
  A null string shall match every line.

Sorry, I somehow overread the last line. So I agree with you on the
second test case.

 That implies that
 
echo one | grep -F -e walrus -e ''
 
 Should match one, but with the gnu/dammit version it only does so  
 _without_ the -F. Or with -F and just one argument...

busybox grep also agrees with you.

  Combined with -x IMO
  it should only match empty lines.
 
 I asked the Austin guys how -F and -x interact. It's not obvious to me  
 from reading it.

Sounds like a good idea.

Felix
___
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net

Re: [Toybox] Put syslogd and logger into the same file?

[Felix Janda]

Rob Landley wrote:
 On 08/18/2013 11:55:22 AM, Felix Janda wrote:
  Rob Landley wrote:
   On 08/10/2013 04:22:58 PM, Felix Janda wrote:
Right now syslogd.c contains code from sys/syslog.h declaring  
  the
arrays
prioritynames and facilitynames, which map human readable strings  
  to
numerical ids. logger.c contains very similar array definitions.
   
sys/syslog.h declares these arrays only when SYSLOG_NAMES has  
  been
#defined. The structure with the descriptive name CODE used for  
  the
array members is also only defined conditionally. Something like
   
#ifndef SYSLOG_NAMES
extern CODE prioritynames[];
extern CODE facilitynames[];
#endif
   
is also missing in sys/syslog.h.
   
IMO the least bad way to use the arrays from sys/syslog.h would  
  be
to
combine syslogd.c and logger.c into one file and there define
SYSLOG_NAMES before including toys.h.
  
   It's a good idea, but:
  
   What I haven't figured out yet is how to do the argument parsing.
   syslogd and logger have very different option strings.
  
   Right now there's only one GLOBALS block per file. That's easy  
  enough
   to fix, and we can #undef TT and #define TT toys. But the FLAG_*  
  macros
   are also defined based on FOR_command. And that's harder to clean  
  up...
  
  I was aware that there might be some problems with the build  
  architecture
  but couldn't name them. Thanks for the clarification.
  
  I have no idea how to get two commands with overlapping conflicting  
  options
  to work when put into the same file.
 
 Neither do I, which is the problem. :)

The problem could also be the abuse of the build infrastructure.

 In theory I could do a second #include to get different FLAG macros,  
 but it would have to #undefine all the current flag macros first, which  
 means I'd have to tell it which version I was switching _from_, so it'd  
 look something like:
 
#define FROM_command
#define FOR_command
#include generated/switchflags.h
 
 As with the OPTSTR_name stuff for oldtoy, the complexity of the  
 infrastructure to implement this (and resulting large and ugly  
 generated/blah.h) makes the mechanism less attractive even if using in  
 commands can then be very simple. I don't like having the common  
 infrastructure be incomprehensible black magic even if it just works  
 when you use it.
 
 Tradeoffs. Lots and lots of tradeoffs. I still might do it, but I have  
 to get comfortable with the idea of adding more crap to generated/*.h...

I don't want a much more complex infrastructure just because of this
minor problem.

  So how about the following hack?
  
  Keep them in separate files. Have one common config symbol,
 
 Which implies one common .c file because the top level config symbols  
 determine which toys/*/*.c files get fed to the gcc command line.
 
 Ok, that's no longer ENTIRELY true. It used to be that the config  
 symbol CP would include cp.c but CP_MORE wouldn't becuase ther was an  
 underscore in it. Then I needed to support stuff like switch_root and  
 nbd-client which have underscores in their config symbols (- gets  
 squashed to _ and then everything is uppercased), so now what it does  
 is checks each config symbol to see if their _is_ a corresponding .c  
 file and assembles a gcc command line from the ones that match.
 
http://landley.net/hg/toybox/rev/654
 
 So it'll work, but it's confusing. (I normally try to put common code  
 in lib, or not have it.)

I decided that it's better (and it even makes sense) to have logger
depend on syslogd. See the code in my other mail.

  tear everything
  out of logger.c but logger_main, declare SYSLOG_NAMES in syslogd.c,  
  put
  something like the current lookup() in logger.c to syslogd.c and make  
  an
  extern declaration in logger.c. The new lookup() (better use a less  
  generic
  name then) should accept an integer as its first argument instead of  
  the
  table, say 0 stands for facilities and 1 for priorities.
 
 I've read that twice and am not following it, which is not a good sign.

 Alas, how to be simple is not, itself, always simple.

The patch in my other mail should make clear what I meant.

 Could you poke me about this again next week? I want to try to clear  
 some of the other pending stuff of my plate. (Todo stack too deep.)

I'll do that.

Felix
___
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net

[Toybox] [CLEANUP] syslogd

[Felix Janda]

# HG changeset patch
# User Felix Janda felix.ja...@posteo.de
# Date 1377033348 -7200
# Node ID c8e6b5f10c57bcca7859b4c2891dfdf346ecac6d
# Parent  f9271a80fedc23b91559f0527da8e9c22d152b5d
syslogd: cleanup

- Remove structure fd_pair so that sigfd can go into GLOBALS
- Remove struct typedefs
- Inline setup_signal()
- Small fix in Usage message

diff -r f9271a80fedc -r c8e6b5f10c57 toys/pending/syslogd.c
--- a/toys/pending/syslogd.cMon Aug 19 22:11:22 2013 +0200
+++ b/toys/pending/syslogd.cTue Aug 20 23:15:48 2013 +0200
@@ -11,7 +11,7 @@
   bool syslogd
   default n
   help
-  Usage: syslogd  [-a socket] [-p socket] [-O logfile] [-f config file] [-m 
interval]
+  Usage: syslogd  [-a socket] [-O logfile] [-f config file] [-m interval]
   [-p socket] [-s SIZE] [-b N] [-R HOST] [-l N] [-nSLKD]
 
   System logging utility
@@ -52,25 +52,22 @@
   struct arg_list *lfiles;  // list of write logfiles
   fd_set rfds;// fds for reading
   int sd;// socket for logging remote messeges.
+  int sigfd[2];
 )
 
 #define flag_get(f,v,d)  ((toys.optflags  f) ? v : d)
 #define flag_chk(f)((toys.optflags  f) ? 1 : 0)
 
 
-// Signal handling 
-struct fd_pair { int rd; int wr; };
-static struct fd_pair sigfd;
-
 // UNIX Sockets for listening
-typedef struct unsocks_s {
+struct unsocks {
   char *path;
   struct sockaddr_un sdu;
   int sd;
-} unsocks_t;
+};
 
 // Log file entry to log into.
-typedef struct logfile_s {
+struct logfile {
   char *filename;
   char *config;
   uint8_t isNetwork;
@@ -78,26 +75,26 @@
   uint8_t level[LOG_NFACILITIES];
   int logfd;
   struct sockaddr_in saddr;
-} logfile_t;
+};
 
 // Adds opened socks to rfds for select()
 static int addrfds(void)
 {
-  unsocks_t *sock;
+  struct unsocks *sock;
   int ret = 0;
   struct arg_list *node = TT.lsocks;
   FD_ZERO(TT.rfds);
 
   while (node) {
-sock = (unsocks_t*) node-arg;
+sock = (struct unsocks*) node-arg;
 if (sock-sd  2) {
   FD_SET(sock-sd, TT.rfds);
   ret = sock-sd;
 }
 node = node-next;
   }
-  FD_SET(sigfd.rd, TT.rfds);
-  return (sigfd.rd  ret)?sigfd.rd:ret;
+  FD_SET(TT.sigfd[0], TT.rfds);
+  return (TT.sigfd[0]  ret) ? TT.sigfd[0] : ret;
 }
 
 /*
@@ -108,11 +105,11 @@
 static int open_unix_socks(void)
 {
   struct arg_list *node;
-  unsocks_t *sock;
+  struct unsocks *sock;
   int ret = 0;
 
   for(node = TT.lsocks; node; node = node-next) {
-sock = (unsocks_t*) node-arg;
+sock = (struct unsocks*) node-arg;
 sock-sdu.sun_family = AF_UNIX;
 strcpy(sock-sdu.sun_path, sock-path);
 sock-sd = socket(AF_UNIX, SOCK_DGRAM, 0);
@@ -158,7 +155,7 @@
 static struct arg_list *get_file_node(char *filename, struct arg_list *list)
 {
   while (list) {
-if (!strcmp(((logfile_t*) list-arg)-filename, filename)) return list;
+if (!strcmp(((struct logfile*) list-arg)-filename, filename)) return 
list;
 list = list-next;
   }
   return list;
@@ -168,7 +165,7 @@
  * recurses the logfile list and resolves config
  * for evry file and updates facilty and log level bits.
  */
-static int resolve_config(logfile_t *file)
+static int resolve_config(struct logfile *file)
 {
   char *tk, *fac, *lvl, *tmp, *nfac;
   int count = 0;
@@ -254,7 +251,7 @@
 // Parse config file and update the log file list.
 static int parse_config_file(void)
 {
-  logfile_t *file;
+  struct logfile *file;
   FILE *fp = NULL;
   char *confline = NULL, *tk = NULL, *tokens[2] = {NULL, NULL};
   int len, linelen, tcount, lineno = 0;
@@ -266,7 +263,7 @@
*/
   if (flag_chk(FLAG_K)) {
 node = xzalloc(sizeof(struct arg_list));
-file = xzalloc(sizeof(logfile_t));
+file = xzalloc(sizeof(struct logfile));
 file-filename = /dev/kmsg;
 file-config = *.*;
 memset(file-level, 0xFF, sizeof(file-level));
@@ -283,7 +280,7 @@
*/
if (flag_chk(FLAG_R)) {
  node = xzalloc(sizeof(struct arg_list));
- file = xzalloc(sizeof(logfile_t));
+ file = xzalloc(sizeof(struct logfile));
  file-filename = xmsprintf(@%s,TT.remote_log);
  file-isNetwork = 1;
  file-config = *.*;
@@ -331,7 +328,7 @@
 node = get_file_node(tokens[1], TT.lfiles);
 if (!node) {
   node = xzalloc(sizeof(struct arg_list));
-  file = xzalloc(sizeof(logfile_t));
+  file = xzalloc(sizeof(struct logfile));
   file-config = xstrdup(tokens[0]);
   if (resolve_config(file)==-1) {
 error_msg(error in '%s' at line %d, TT.config_file, lineno);
@@ -343,7 +340,7 @@
   node-next = TT.lfiles;
   TT.lfiles = node;
 } else {
-  file = (logfile_t*) node-arg;
+  file = (struct logfile*) node-arg;
   int rel = strlen(file-config) + strlen(tokens[0]) + 2;
   file-config = xrealloc(file-config, rel);
   sprintf(file-config, %s;%s, file-config, tokens[0]);
@@ -360,7 +357,7 @@
*/
   if (!fp){
 node = xzalloc(sizeof(struct arg_list));
-file = xzalloc(sizeof(logfile_t));
+file = xzalloc(sizeof(struct logfile));
 file-filename

Re: [Toybox] lspci text output

[Felix Janda]

Isaac wrote:
 On Fri, Aug 02, 2013 at 10:35:30PM +0200, Felix Janda wrote:
  Isaac wrote:
   On Sat, Jul 27, 2013 at 09:56:53AM +0200, Felix Janda wrote:
  [...]
Anyway Isaac is doing (or planning to do) a major rewrite enabling lspci
to read the pciid database for human readable names of vendors and 
devices.
   
   I have a question or two in regards to that.
   1-mmap or fgets?
   It's a smallish file as data goes (currently~600 kb), and can be searched 
   in very little time if mmap()ed. 
   The alternative is an fgets() based loop, which could involve 
   reading over 11,000 entries, and decent performance would require 
   using a linked list for temporary storage, extracting and sorting the 
   entries
   from the linked list, looping over the sorted entries and storing the 
   text,
   then printing from the linked list (though you can print from the sorted 
   data
   if you don't care about order).
   I'm thinking mmap sounds more practical.
  
  I don't understand your argument. What would be the algorithm using mmap()?
 
 With mmap, the time is small enough to search the whole file every time.
 But now that I try fgets, I think I overestimated the time it takes:
 $ time ./lookup-fgets C 11 80   /usr/share/misc/pci.ids
 0: Signal processing controller : gnal processing controller
 0m0.01s real 0m0.01s user 0m0.00s system
 
 (5400 RPM HDD, Atom N270 locked to 800 MHz)
 When Vendor is C **, device is 2 chars long, and I'm not sure what's a 
 good way to handle that.
 See attached files for mmap (thanks to jamesbond at the Puppy forums) 
 and fgets examples.
 Usage is 
 lookup-fgets vendor device /usr/share/misc/pci.ids
 lookup vendor device
 lookup-fgets can be used with USB devices as well.

Thanks for the test programs. After clearing the caches
(echo 3  /proc/sys/vm/drop_caches) it takes

0m0.05s real 0m0.01s user 0m0.00s system

for me and nothing time(1)-measurable the second time
(720 RPM HDD, K8 CPU scaled to 1 GHz)

What are these classes used for? (Do we really need to care about them?)

Under usual circumstances the repeated searching shouldn't be noticeable.
It should be ok to use the simplest approach.

So one could even stay with the general current design? (Without the need to
keep a list of devices.)

  How about the following approach using fgets? Say in a simplified version
  we start out with a list of
  
  struct foo {
struct foo *next;
struct bar *strs;
char vendor[4], device[4];
  }
  
  , where
  
  struct bar {
struct foo *ref;
char *vendor, *device;
  }
  
  , filled out from the /sys/ files. The fields strs are initialized to point
  to elements (one for each device) of an array of struct bar and the ref 
  fields
  of the elements of this array should point back to the corresponding list
  element. Then, sort the array of struct bars according to ref-vendor and
  ref-device. Next, update ref-strs to point to the right element in the
  sorted array. Now we can start reading the (sorted) pci database line by
  line and fill out the other elements of the struct bar array. For the
  printing we iterate through the list of struct foo.
  
 That seems to make more sense than what I was thinking of, 
 if it really is needed (see time).

Yeah, quite some book keeping would be needed for the approach.

Felix
___
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net

Re: [Toybox] [patch] add renice

[Felix Janda]

Strake wrote:
 On 28/07/2013, Felix Janda felix.ja...@posteo.de wrote:
  Some comments below.
 
 Thanks.

Thanks for your new patch.

[...]
  +  for (ii = 0; ii  toys.optc; ii++) {
 
  You don't really need the index. Just loop over toys.optargs, which are
  conveniently null terminated.
 
 To me, at least, it is clearer with the index, as it's a commoner idiom.

I can't really argue with that.

  +id_t id;
  +
  +if (isdigit (toys.optargs[ii][0])) id = strtoul (toys.optargs[ii], 0,
  10);
  +else if (toys.optflags  FLAG_u) id = getpwnam (toys.optargs[ii])
  - pw_uid;
 
  Mail mungling FTW.
 
 ?

Sorry, s/mungling/mangling/. Your mail client or something else has broken
the patch. Anyway, this doesn't apply to the new patch.

[...]
 From 2741527c9b56cf654ff653c6171751112985bae6 Mon Sep 17 00:00:00 2001
 From: Strake strake...@gmail.com
 Date: Sun, 28 Jul 2013 09:30:35 -0500
 Subject: [PATCH] add renice
 
 ---
  toys/pending/renice.c | 55 
 +++
  1 file changed, 55 insertions(+)
  create mode 100644 toys/pending/renice.c
 
 diff --git a/toys/pending/renice.c b/toys/pending/renice.c
 new file mode 100644
 index 000..3671f91
 --- /dev/null
 +++ b/toys/pending/renice.c
 @@ -0,0 +1,55 @@
 +/* renice.c - renice process
 + *
 + * Copyright 2013 CE Strake strake888 at gmail.com
 + *
 + * See http://pubs.opengroup.org/onlinepubs/9699919799/utilities/
 + * See 
 http://refspecs.linuxfoundation.org/LSB_4.1.0/LSB-Core-generic/LSB-Core-generic/cmdbehav.html
 +
 +TO DO: -n is required, so use '|' modifier when available
 +
 +USE_RENICE(NEWTOY(renice, gpun#, TOYFLAG_BIN))
 +
 +config RENICE
 +  bool renice
 +  default n
 +  help
 +usage: renice [-gpu] -n increment ID ...
 +*/
 +
 +#define FOR_renice
 +#include toys.h
 +
 +GLOBALS(
 +  long nArgu;
 +)
 +
 +void renice_main (void) {
 +  int ii;
 +  int which = toys.optflags  FLAG_g ? PRIO_PGRP :
 +  toys.optflags  FLAG_u ? PRIO_USER :
 +  PRIO_PROCESS;
 +
 +  if (!(toys.optflags  FLAG_n)) error_exit (no increment given);
 +
 +  for (ii = 0; ii  toys.optc; ii++) {
 +id_t id;
 +
 +if (toys.optflags  FLAG_u) {
 +  struct passwd *p_pw;
 +  p_pw = getpwnam (toys.optargs[ii]);
 +  if (p_pw) {
 +id = p_pw - pw_uid;
 +goto have_id;
 +  }
 +}

 +if (!isdigit (toys.optargs[ii][0])) {
 +  error_msg (not a number: %s, toys.optargs[ii]);
 +  continue;
 +}
 +id = strtoul (toys.optargs[ii], 0, 10);

Any reason only to check the first char? Why not

char *endptr;
...
id = strtoul (toys.optargs[ii], endptr, 10);
if (!(toys.optargs[ii][0]  *endptr)) {
  error_msg (not a number: %s, toys.optargs[ii]);
  continue;
}

 +have_id:
 +if (setpriority (which, id, getpriority (which, id) + TT.nArgu)  0) {
 +  perror_msg (failed to setpriority of %d, id);
 +}
 +  }
 +}

Felix
___
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net

Re: [Toybox] [TOY] lspci - no *alloc

[Felix Janda]

Isaac wrote:
 On Wed, Jul 24, 2013 at 09:20:51PM +0200, Felix Janda wrote:
  Attached Isaac's toy edited to use toybuf instead of
  dynamic memory allocation. It could look prettier, though...
  
  Felix
  
 Broken:
 /toybox lspci -e
 00:00.0 Class 0600: 8086
 :27ac
  
 00:02.0 Class 0300: 8086
 :27ae
  
 ..
 /toybox lspci
 00:00.0 Class 0600: 8086:27ac 
 00:02.0 Class 0300: 8086:27ae
 ..
 
 The -e flag should be expanding the width of class (it isn't),
 and it should not be making lspci print 3 lines per entry.
 And I'm not sure how to fix it.

Sorry, I obviously didn't test -e...

  size = 6 + 2*((toys.optflags  FLAG_e)  (p != toybuf));
  size = 6 + 2*((toys.optflags  FLAG_e)  (p == toybuf));

Or better:

  size = ((toys.optflags  FLAG_e)  (p == toybuf)) ? 8 : 6;

So I guess that the code is quite hard to comprehend.

 By the way, there are requests for more features (-v/-vv and text output) 
 that would mean slightly different approaches to getting data; (p)readat_name 
 would work in many of these cases, but text output means reading to toybuf
 (which would clobber this), and I would also need to add more fields of 
 varying sizes.

Yeah, I guess I did some premature optimizations. But still the data read from 
the
/sys files is of relatively small fixed size?

Instead of to toybuf one could also put the buffers on the stack freeing up 
toybuf
for other purposes.

For the text output the structure of the toy would change a lot if we want to
avoid reparsing the pci id database for each device.

Felix
___
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net

Re: [Toybox] [TOY] lspci - no *alloc

[Felix Janda]

Attached Isaac's toy edited to use toybuf instead of
dynamic memory allocation. It could look prettier, though...

Felix
/*
 * lspci - written by Isaac Dunham

USE_LSPCI(NEWTOY(lspci, emkns:, TOYFLAG_USR|TOYFLAG_BIN))

config LSPCI
  bool lspci
  default n
  help
usage: lspci [-ekmn]

List PCI devices.
-e  Print all 6 digits in class (like elspci)
-k  Print kernel driver
-m  Machine parseable format
-n  Numeric output (default)
*/
#define FOR_lspci
#include toys.h

int do_lspci(struct dirtree *new)
{
  int alen = 8, dirfd;
  char *dname = dirtree_path(new, alen);
  struct {
char class[16], vendor[16], device[16], module[256];
  } *bufs = (void*)(toybuf + 2);

  if (!strcmp(/sys/bus/pci/devices, dname)) return DIRTREE_RECURSE;
  errno = 0;
  dirfd = open(dname, O_RDONLY);
  if (dirfd  0) {
char *p, **fields = (char*[]){class, vendor, device, };

for (p = toybuf; **fields; p+=16, fields++) {
  int fd, size;

  if ((fd = openat(dirfd, *fields, O_RDONLY))  0) continue;
  size = 6 + 2*((toys.optflags  FLAG_e)  (p != toybuf));
  p[read(fd, p, size)] = '\0';
  close(fd);
}

close(dirfd);
if (!errno) {
  char *driver = ;
  char *fmt = toys.optflags  FLAG_m ? %s, \%s\ \%s\ \%s\ \%s\\n
   : %s Class %s: %s:%s %s\n;

  if (toys.optflags  FLAG_k) {
strcat(dname, /driver);
if (readlink(dname, bufs-module, sizeof(bufs-module)) != -1)
  driver = basename(bufs-module);
  }
  printf(fmt, new-name + 5, bufs-class, bufs-vendor, bufs-device, 
   driver);
}
  }
  return 0;
}

void lspci_main(void)
{
  dirtree_read(/sys/bus/pci/devices, do_lspci);
}
___
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net

[Toybox] [CLEANUP] logger

[Felix Janda]

Various fixes to logger in below patch.

Mainly inline parse_priority(). Many other small changes e.g. s/const//,
move around some variables, ...

-Felix

# HG changeset patch
# User Felix Janda felix.ja...@posteo.de
# Date 1374706109 -7200
# Node ID 8cc26924c2ef2e862c975a31167a685e18f555ec
# Parent  8ad85a95f7c3b75214fbd3a46fe834de45c39417
logger: Some cleanup

diff -r 8ad85a95f7c3 -r 8cc26924c2ef toys/pending/logger.c
--- a/toys/pending/logger.c Tue Jul 23 20:19:31 2013 -0500
+++ b/toys/pending/logger.c Thu Jul 25 00:48:29 2013 +0200
@@ -18,23 +18,18 @@
 #define FOR_logger
 #include toys.h
 #include syslog.h
-#include strings.h
-#include string.h
 
 GLOBALS(
   char *priority_arg;
   char *ident;
-
-  int facility;
-  int priority;
 )
 
 struct mapping {
-  const char *key;
+  char *key;
   int value;
 };
 
-static const struct mapping facilities[] = {
+static struct mapping facilities[] = {
   {user, LOG_USER}, {main, LOG_MAIL}, {news, LOG_NEWS},
   {uucp, LOG_UUCP}, {daemon, LOG_DAEMON}, {auth, LOG_AUTH},
   {cron, LOG_CRON}, {lpr, LOG_LPR}, {local0, LOG_LOCAL0},
@@ -44,64 +39,51 @@
   {NULL, 0}
 };
 
-static const struct mapping priorities[] = {
+static struct mapping priorities[] = {
   {emerg, LOG_EMERG}, {alert, LOG_ALERT}, {crit, LOG_CRIT},
   {err, LOG_ERR}, {warning, LOG_WARNING}, {notice, LOG_NOTICE},
   {info, LOG_INFO}, {debug, LOG_DEBUG},
   {NULL, 0}
 };
 
-static int lookup(const struct mapping *where, const char *key)
+static int lookup(struct mapping *where, char *key)
 {
-  int i;
-  for (i = 0; where[i].key; i++)
-if (!strcasecmp(key, where[i].key))
-  return where[i].value;
+  for (; where-key; where++)
+if (!strcasecmp(key, where-key)) return where-value;
 
   return -1;
 }
 
-static void parse_priority()
+void logger_main(void)
 {
-  char *sep = strchr(TT.priority_arg, '.');
+  int facility = LOG_USER, priority = LOG_NOTICE;
+  char *message = NULL;
 
-  if (sep)
-  {
-*sep = '\0';
-if ((TT.facility = lookup(facilities, TT.priority_arg)) == -1)
-  error_exit(bad facility: %s, TT.priority_arg);
-TT.priority_arg = sep+1;
+  if (toys.optflags  FLAG_p) {
+char *sep = strchr(TT.priority_arg, '.');
+
+if (sep) {
+  *sep = '\0';
+  if ((facility = lookup(facilities, TT.priority_arg)) == -1)
+error_exit(bad facility: %s, TT.priority_arg);
+  TT.priority_arg = sep+1;
+}
+
+if ((priority = lookup(priorities, TT.priority_arg)) == -1)
+  error_exit(bad priority: %s, TT.priority_arg);
   }
 
-  if ((TT.priority = lookup(priorities, TT.priority_arg)) == -1)
-error_exit(bad priority: %s, TT.priority_arg);
-}
+  if (!(toys.optflags  FLAG_t)) {
+struct passwd *pw = getpwuid(geteuid());
 
-void logger_main(void)
-{
-  if (toys.optflags  FLAG_p)
-parse_priority();
-  else
-  {
-TT.facility = LOG_USER;
-TT.priority = LOG_NOTICE;
-  }
-
-  if (!(toys.optflags  FLAG_t))
-  {
-struct passwd *pw = getpwuid(geteuid());
-if (!pw)
-  perror_exit(getpwuid);
+if (!pw) perror_exit(getpwuid);
 TT.ident = xstrdup(pw-pw_name);
   }
 
-  char *message = NULL;
   if (toys.optc) {
-int length = 0;
-int pos = 0;
+int length = 0, pos = 0;
 
-for (;*toys.optargs; (void) *(toys.optargs)++) // shut up gcc
-{
+for (;*toys.optargs; toys.optargs++) {
   length += strlen(*(toys.optargs)) + 1; // plus one for the args spacing
   message = xrealloc(message, length + 1); // another one for the null byte
 
@@ -113,7 +95,7 @@
 message = toybuf;
   }
 
-  openlog(TT.ident, (toys.optflags  FLAG_s ? LOG_PERROR : 0) , TT.facility);
-  syslog(TT.priority, %s, message);
+  openlog(TT.ident, (toys.optflags  FLAG_s ? LOG_PERROR : 0) , facility);
+  syslog(priority, %s, message);
   closelog();
 }
___
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net

Re: [Toybox] [TOY] lspci

[Felix Janda]

Georgi Chorbadzhiyski wrote:
 On 7/22/13 10:05 PM, Felix Janda wrote:
  Isaac wrote:
 
  I've written an lspci implementation.
  
  Cool.
  
  Currently it supports -emkn; -e is an extension (class is a 24-bit 
  number,
  but lspci only shows 16 bits; one person on the Puppy forums mentioned
  that they need those last 8 bits).
  -n is a no-op for compatability with standard lspci.
  
  Are there any more options you have in mind that should be implemented?
  Just out of curiosity: Could you post a link to the forum post?
  
  75 lines total.
  
  Down to 70 after application of the below patch with small fixes.
  
  It felt more sensible to me to read 2 more bytes and ignore them than to
  seek for 2 bytes in a file. So preadat_name now has one argument less.
  
  Use calloc instead of malloc + memset. With this also the off-by-one
  error from (origninally) line 32 is fixed. It would be nice to use
  toybuf for this, wouldn't it? That would also make valgrind happier.
  
  Print  instead of . when .../driver is missing.
  
  The only difference between the usual output and the machine readable
  output is the format string. Use this to avoid some repetition.
  
  Felix
  
  --- a/lspci.c   2013-07-22 18:05:37.514838360 +0200
  +++ b/lspci.c   2013-07-22 19:12:03.0 +0200
  @@ -17,19 +17,14 @@
   */
   #define FOR_lspci
   #include toys.h
  -char * preadat_name(int dirfd, char *fname, size_t nbyte, off_t offset)
  +char * preadat_name(int dirfd, char *fname, size_t nbyte)
   {
 int fd;
  -  char *buf = malloc(nbyte+1);
  -  memset(buf, 0, sizeof(buf));
  -  fd = openat(dirfd, fname, O_RDONLY);
  -  if (fd  0) {
  -return NULL;
  -  }
  -  lseek(fd, offset, SEEK_SET);
  +  char *buf = calloc(1, nbyte+1);
  +
  +  if (0  (fd = openat(dirfd, fname, O_RDONLY))) return NULL;
 
 It would be better to move calloc after openat (which may fail).

That's true. Actually I'd prefer having no dynamic allocation at all.
Have something like

struct {
  char class[8], vendor[6], device[6], module[256];
};

at toybuf and use that as the buffer. Maybe also remove preadat
all together and use a loop in do_lspci?

Georgi Chorbadzhiyski wrote:
 And another thing, IMHO
 
 ((fd = openat(dirfd, fname, O_RDONLY))  0) return NULL;
 
 is better than
 
 if (0  (fd = openat(dirfd, fname, O_RDONLY))) return NULL;

I agree.

Felix
___
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net

Re: [Toybox] [CLEANUP] paste

[Felix Janda]

Rob Landley wrote:
 Description of the paste cleanup that accidentally got checked in  
 during commit 944 because hg import -f has side effects.
 
http://landley.net/hg/toybox/rev/944
 
 (Yes, that commit description is actively ironic. Scroll down to  
 patch.c.)
[...]

The accidental cleanup checked in does not seem to reflect what you
describe in the mail. It looks like a zeroth round of cleanup fixing
only some whitespace.

Thanks,
Felix
___
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net

Re: [Toybox] Sigh. Anybody spot the bug?

[Felix Janda]

Rob Landley wrote:
[...]
  than count*sizeof(toybuf) and contain no newlines.
 
 Count is initialized in a for loop and starts at 0 on line 164 so I  
 dunno what this means...

Sorry for the confusion. I meant the number of lines tail is told to
output.

[...]
  diff -r f8db1f6ec4ab toys/posix/tail.c
  --- a/toys/posix/tail.c Tue Jul 02 00:16:16 2013 -0500
  +++ b/toys/posix/tail.c Wed Jul 03 20:54:24 2013 +0200
  @@ -168,7 +168,7 @@
   }
  
   if (lines) {
  -  if(try[count] != '\n'  count != len-1) continue;
  +  if(try[count] != '\n') continue;
 
 Hmmm, I believe that's related to getting the line count right when the  
 last line doesn't end with a newline? But I don't remember the  
 details...

See Isaac's replies in this topic. The current condition is just wrong:

$ cat toys.h toys.h toys.h | tail | wc -l
9

This is fixed with the patch. However in some situations it still gives
wrong output since other code still assumes that there are newlines at
the end of each sizeof(toybuf) piece. Maybe it's simpler to rewrite this
part...

Felix
___
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net

[Toybox] [PATCH] Fill out test(1)

[Felix Janda]

Hello,

attached is a patch filling out part of Rob's test(1) template and
adding some test cases.

Everything but the obsolete XSI optional combination of tests with -a
and -o is (hopefully) implemented. The SUSV4 TC1 (and not they alone)
advises people to use the shell's  and || instead. Are many shell
scripts in the wild using these options? Should they be implemented,
it looks like some code could be shared with find.


Maybe it would be better to use a goto for the error handling in test.c.

Felix
# HG changeset patch
# User Felix Janda felix.ja...@posteo.de
# Date 1374189502 -7200
# Node ID 9c47751a9c4b053000c0a87e01b1ff6ca8c50c2e
# Parent  62d59b8aea34874c59135c8a72a9eef300e3f800
Implement test

diff --git a/scripts/test/test.test b/scripts/test/test.test
new file mode 100755
--- /dev/null
+++ b/scripts/test/test.test
@@ -0,0 +1,63 @@
+#!/bin/bash
+
+[ -f testing.sh ]  . testing.sh
+
+#testing name command result infile stdin
+
+# TODO: Should also have device and socket files
+
+mkdir d
+touch f
+ln -s /dev/null L
+echo nonempty  s
+mkfifo p
+
+type_test()
+{
+  result=
+  for i in d f L s p n
+  do
+if test $* $i
+then
+  result=$result$i
+fi
+  done
+  printf %s $result
+}
+
+testing test -b type_test -b   
+testing test -c type_test -c L  
+testing test -d type_test -d d  
+testing test -f type_test -f fs  
+testing test -h type_test -h L  
+testing test -L type_test -L L  
+testing test -s type_test -s ds  
+testing test -S type_test -S   
+testing test -p type_test -p p  
+testing test -e type_test -e dfLsp  
+testing test ! -e type_test ! -e n  
+
+rm f L s p
+rmdir d
+
+# TODO: Test rwx gu t
+
+testing test test  || test a  echo yes yes\n  
+testing test -n test -n  || test -n a  echo yes yes\n  
+testing test -z test -n a || test -n   echo yes yes\n  
+testing test a = b test a = b || test  =   echo yes yes\n  
+testing test a != b test  !=  || test a = b  echo yes yes\n  
+
+arith_test()
+{
+  test -1 $1 1  echo l
+  test 0 $1 0  echo e
+  test -3 $1 -5  echo g
+}
+
+testing test -eq arith_test -eq e\n  
+testing test -ne arith_test -ne l\ng\n  
+testing test -gt arith_test -gt g\n  
+testing test -ge arith_test -ge e\ng\n  
+testing test -lt arith_test -lt l\n  
+testing test -le arith_test -le l\ne\n  
diff --git a/toys/pending/test.c b/toys/pending/test.c
--- a/toys/pending/test.c
+++ b/toys/pending/test.c
@@ -41,5 +41,74 @@
 
 void test_main(void)
 {
+  int id, not;
+  char *s, *err_fmt = Bad flag '%s';
+
+  toys.exitval = 2;
+  if (!strcmp([, toys.which-name))
+if (!strcmp(], toys.optargs[--toys.optc])) error_exit(Missing ']');
+  if (!strcmp(!, toys.optargs[0])) {
+not = 1;
+toys.optargs++;
+toys.optc--;
+  }
+  if (!toys.optc) toys.exitval = 0;
+  else if (toys.optargs[0][0] == '-') {
+id = stridx(bcdefghLpSsurwxznt, toys.optargs[0][1]);
+if (id == -1 || toys.optargs[0][2]) error_exit(err_fmt, toys.optargs[0]);
+if (id  12) {
+  struct stat st;
+  int nolink;
+
+  toys.exitval = 1;
+  if (lstat(toys.optargs[1], st) == -1) return;
+  nolink = !S_ISLNK(st.st_mode);
+  if (!nolink  (stat(toys.optargs[1], st) == -1)) return;
+
+  if (id == 0) toys.exitval = !S_ISBLK(st.st_mode); // b
+  else if (id == 1) toys.exitval = !S_ISCHR(st.st_mode); // c
+  else if (id == 2) toys.exitval = !S_ISDIR(st.st_mode); // d
+  else if (id == 3) toys.exitval = 0; // e
+  else if (id == 4) toys.exitval = !S_ISREG(st.st_mode); // f
+  else if (id == 5) toys.exitval = !(st.st_mode  S_ISGID); // g
+  else if ((id == 6) || (id == 7)) toys.exitval = nolink; // hL
+  else if (id == 8) toys.exitval = !S_ISFIFO(st.st_mode); // p
+  else if (id == 9) toys.exitval = !S_ISSOCK(st.st_mode); // S
+  else if (id == 10) toys.exitval = st.st_size == 0; // s
+  else toys.exitval = !(st.st_mode  S_ISUID); // u
+}
+else if (id  15) // rwx
+  toys.exitval = access(toys.optargs[1], 1  (id - 12)) == -1;
+else if (id  17) // zn
+  toys.exitval = toys.optargs[1]  !*toys.optargs[1] ^ (id - 15);
+else { // t
+  struct termios termios;
+  toys.exitval = tcgetattr(atoi(toys.optargs[1]), termios) == -1;
+}
+  }
+  else if (toys.optc == 1) toys.exitval = *toys.optargs[0] == 0;
+  else if (toys.optc == 3) {
+if (*toys.optargs[1] == '-') {
+  long a = atol(toys.optargs[0]), b = atol(toys.optargs[2]);
+  
+  s = toys.optargs[1] + 1;
+  if (!strcmp(eq, s)) toys.exitval = a != b;
+  else if (!strcmp(ne, s)) toys.exitval = a == b;
+  else if (!strcmp(gt, s)) toys.exitval = a  b;
+  else if (!strcmp(ge, s)) toys.exitval = a = b;
+  else if (!strcmp(lt, s)) toys.exitval = a  b;
+  else if (!strcmp(le, s)) toys.exitval = a = b;
+  else error_exit(err_fmt, toys.optargs[1]);
+}
+else {
+  int result = strcmp(toys.optargs[0], toys.optargs[2]);
+
+  s = toys.optargs[1];
+  if (!strcmp(=, s)) toys.exitval = !!result;
+  else

Re: [Toybox] Sigh. Anybody spot the bug?

[Felix Janda]

Isaac wrote:
 On Wed, Jul 03, 2013 at 08:56:46PM +0200, Felix Janda wrote:
  Rob Landley wrote:
   Tail has a double free somewhere. (Aboriginal's more/buildall.sh is  
   complaining, that uses toybox in host-tools.) Haven't had time to track  
   it down yet, wondering if anybody else could spot it.
   
From the behavior it's looking like it's on file close...
  [...]
  
  It seems reproducible when using tail on non-seekable files which are bigger
  than count*sizeof(toybuf) and contain no newlines.
  
 I presume this is with TAIL_SEEK=y.

No. With TAIL_SEEK=n it's even easier to trigger. Just try to tail any large 
file.

 I'm not sure what count*sizeof(toybuf) means since count is a loop counter.
 Would a 4097-byte file with no \n cause it? Or 512*4096+1?
 (I'm wanting to test the other proposed solution.)

With count I meant the argument to the -n option (default 10). (Sorry for the
confusion.) Just do a

yes | paste -s | tail

to trigger the bug.

The other proposed solution can't solve this specific problem since the changed
code isn't touched in this situation.

Felix
___
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net

Re: [Toybox] Sigh. Anybody spot the bug?

[Felix Janda]

Rob Landley wrote:
 Tail has a double free somewhere. (Aboriginal's more/buildall.sh is  
 complaining, that uses toybox in host-tools.) Haven't had time to track  
 it down yet, wondering if anybody else could spot it.
 
  From the behavior it's looking like it's on file close...
[...]

It seems reproducible when using tail on non-seekable files which are bigger
than count*sizeof(toybuf) and contain no newlines.

The following seems to fix it. I don't really understand the code though.

diff -r f8db1f6ec4ab toys/posix/tail.c
--- a/toys/posix/tail.c Tue Jul 02 00:16:16 2013 -0500
+++ b/toys/posix/tail.c Wed Jul 03 20:54:24 2013 +0200
@@ -168,7 +168,7 @@
 }
 
 if (lines) {
-  if(try[count] != '\n'  count != len-1) continue;
+  if(try[count] != '\n') continue;
   if (lines0) {
 if (!++lines) ++lines;
 continue;


Felix
___
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net

[Toybox] generated/help.h depends on CONFIG_TOYBOX_HELP

[Felix Janda]

Hello,

the build of toybox was broken if CONFIG_TOYBOX_HELP was enabled but
CONFIG_HELP was disabled (and help.h was not yet generated). The
attached patch fixes it.

Felix
# HG changeset patch
# User Felix Janda felix.ja...@posteo.de
# Date 1367349103 -7200
# Node ID f2a8f8981c83cb496f61bdd7e5692f4c8cbcb477
# Parent  5735c500f835540a5465adf7f1f4fa0286e6c54c
generated/help.h depends on CONFIG_TOYBOX_HELP

diff -r 5735c500f835 -r f2a8f8981c83 scripts/make.sh
--- a/scripts/make.sh	Tue Apr 30 00:31:01 2013 -0500
+++ b/scripts/make.sh	Tue Apr 30 21:11:43 2013 +0200
@@ -120,7 +120,7 @@
 
 echo generated/help.h
 # Only recreate generated/help.h if python is installed
-if [ ! -z $(which python) ]  [ ! -z $(grep 'CONFIG_HELP=y' .config) ]
+if [ ! -z $(which python) ]  [ ! -z $(grep 'CONFIG_TOYBOX_HELP=y' .config) ]
 then
   echo Extract help text from Config.in.
   scripts/config2help.py Config.in  generated/help.h || exit 1
___
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net

[Toybox] find: Fix -mtime

[Felix Janda]

Hi,

find -mtime didn't work as expected in the first place and I've broken
it even more with the cleanups. The attached patch should fix it (and
simplify the relevant code a bit).

Felix
# HG changeset patch
# User Felix Janda felix.ja...@posteo.de
# Date 1367010929 -7200
# Node ID 96a66b48149557cf6a890e8b24ac8243e86d735d
# Parent  faecf1d6e90a2b533a7b595ba343e0b9608d8dca
Fix find -mtime

diff -r faecf1d6e90a -r 96a66b481495 toys/pending/find.c
--- a/toys/pending/find.c	Mon Apr 22 23:18:05 2013 +0200
+++ b/toys/pending/find.c	Fri Apr 26 23:15:29 2013 +0200
@@ -35,7 +35,7 @@
   union {
 char *name_regex;
 struct {
-  char time_op;
+  int sign;
   time_t time;
 } t;
 mode_t type;
@@ -49,10 +49,9 @@
 GLOBALS(
   char *dir;
   struct filter_node *filter_root;
+  time_t itime;
 )
 
-#define SECONDS_PER_DAY (24*60*60)
-
 /* filter operation types */
 #define OP_UNKNOWN	0
 #define OP_OR		2
@@ -74,10 +73,6 @@
 
 #define IS_ACTION(x)(x = 20)
 
-#define TEST_LT		0
-#define TEST_EQ		1
-#define TEST_GT		2
-
 /* executes the command for a filter node
returns 0 for failure or 1 for success
  */
@@ -145,27 +140,9 @@
 return !strcmp(filter-data.name_regex, node-name);
 
   if (op==CHECK_MTIME) {
-time_t node_time = node-st.st_mtime/SECONDS_PER_DAY;
-result = 1;
-switch (filter-data.t.time_op) {
-  /* do time compare here */
-  case TEST_LT:
-/* FIXTHIS - should result be  or = ?*/
-if (node_time  filter-data.t.time) result = 0;
-break;
-  case TEST_GT:
-if (node_time  filter-data.t.time) result = 0;
-break;
-  case TEST_EQ:
-if (node_time != filter-data.t.time) result = 0;
-break;
-  default:
-/* how'd I get here? */
-result = 0;
-break;
-}
-return result;
-
+long delta = (long)(((double)TT.itime - node-st.st_mtime) / 86400)
+ -filter-data.t.time;
+return filter-data.t.sign == (delta  0) - (delta  0);
   }
   if (op==CHECK_TYPE) return (node-st.st_mode  S_IFMT) == filter-data.type;
 
@@ -253,19 +230,18 @@
   case CHECK_MTIME:
 switch(**arg) {
   case '+':
-node-data.t.time_op=TEST_GT;
-arg++;
+node-data.t.sign=+1;
+(*arg)++;
 break;
   case '-':
-node-data.t.time_op=TEST_LT;
-arg++;
+node-data.t.sign=-1;
+(*arg)++;
 break;
   default:
-node-data.t.time_op=TEST_EQ;
+node-data.t.sign=0;
 break;
 }
-/* convert to days (very crudely) */
-node-data.t.time = atoi(*arg)/SECONDS_PER_DAY;
+node-data.t.time = atoi(*arg);
 break;
   case CHECK_TYPE:
 if (-1 == (j = stridx(fdcblsp, **arg)))
@@ -373,6 +349,7 @@
 
 void find_main(void)
 {
+  TT.itime = time(NULL);
   /* parse filters, if present */
   build_filter_list();
 
___
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net

Re: [Toybox] More find cleanup

[Felix Janda]

On 04/21/13 at 11:01pm, Rob Landley wrote:
 [snip]
  Ok, I thought more about the extra jumping around than about the  
  integer
  checks. But actually it should only 2 additional jumps (jumping to the
  case label and jumping out of the switch with break).
 
 Disk access should totally dominate here. Even if it's cached, the  
 syscalls to fetch the next directory entries should dominate. If it  
 doesn't, then worry about it.

Actually it matters even less since the command line is processed only
once...

Felix
___
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net

Re: [Toybox] More find cleanup

[Felix Janda]

On 04/21/13 at 12:38pm, Rob Landley wrote:
 On 04/20/2013 01:39:10 PM, Felix Janda wrote:
  Hello,
  
  some more find cleanup in an attached patch.
  
  The main change was to make some code in build_filter_list() less
  repetitive using a suitable struct and a loop.
 
 Problem: you defined a struct locally, then used that structure  
 definition in GLOBALS. This works for buliding find itself, but breaks  
 for building every other command, because they haven't got this  
 structure definition.

 [snip]

 (If there's a good enough reason you can throw the structure definition  
 into lib/lib.h but the rule of thumb there is used by more than one  
 command.)

Is this no longer relevant because of the following?

On 04/21/13 at 12:40pm, Rob Landley wrote:
 Huh, it didn't complain building defconfig. (Presumably because it's a  
 pointer not an instance of the struct, so it didn't care about the  
 sizeof the struct because that doesn't affect the sizeof the pointer.)
 
 *shrug* Learn something new every day...
 
 Rob


  The command line parsing
  is however now less efficient since after an argument has been tested
  for equality with different strings like -mtime and the  
  corresponding
  type (CHECK_MTIME) has been saved, the saved integer has again to be
  checked for equality with stuff to find out what further command line
  parsing is necessary.
 
 An integer check is trivial. String comparisons do an order of  
 magnitude more work and evict cache lines in the process. On processors  
 with multiple execution units (on x86 that's everything since the  
 original pentium) an extra integer operation may even use an otherwise  
 idle execution unit so literally take no time at all. (Branches can be  
 expensive, it depends.)

Ok, I thought more about the extra jumping around than about the integer
checks. But actually it should only 2 additional jumps (jumping to the
case label and jumping out of the switch with break).

 I'm sure I've written up the whole spiel about branch prediction and  
 register renaming, and why the embedded world chooses not to at some  
 point. If I was more awake I could probably not only find it but work  
 in a joke about at quoted block's structural parallels to the full  
 title of The Hobbit. Lemme know if you're curious enough for me to redo  
 it here...

No thanks.


  BTW: POSIX-1.2008 TC1 is out
 
 I might be happy about this if they hadn't deleted the original  
 Posix-2008 to post 5 years of updates all at once (Posix-2001 to  
 Posix-2008 was only 7 years) without an obvious changelog. (It says the  
 rationale has one, but that's just why we removed tar and cpio in  
 favor of a pax command nobody will ever use, why we decided not to  
 include chroot, su, mknod, login, dc because who uses _that_... All  
 2008 decisions, no 2013 diffs.)

 Instead, I'm kind of annoyed about it. Luckily, I downloaded the old  
 one.
 
 (Version control, guys. It's a thing.)

Isn't there a CHANGE HISTORY for each command? Is for example the -path
primary new for find? Not that this diff format is very handy.

Felix

 Rob
___
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net

Re: [Toybox] [CLEANUP] stat: Some cleanup

[Felix Janda]

On 04/21/13 at 12:09am, Rob Landley wrote:
 On 04/19/2013 05:28:27 PM, Felix Janda wrote:
  [snip]
  The functionality of get_access_str() is also implemented in the ls  
  toy
  (likely much better). Should something like it be in the library?
 
 Once code can be shared by a second command, putting it in lib makes  
 sense.
 
  Actually since stat and ls are both frontends to the stat system call
  more is duplicated.
 
 If you can come up with a clean way to share the code, go for it.

Ok, actually I'd only put something like get_access_str() of stat into
the lib.

 I note that I have a pending todo item in ls: ls -l /dev/null doesn't  
 show the major/minor numbers. (Oops. Needs another output mode for  
 device nodes. If I did one, it's not triggering right...)

Ok.

  I also wonder whether toybox will ever have SELinux support... My  
  local
  man page does even not document the -Z option to stat.
 
 I'm leaning against it. If you saw my toybox talk at ELC (it's on  
 youtube) I talked about security approaches and containers vs selinux.

I anticipated that (and read your outline and skimmed the video). So it
would be ok to remove the SELinux parts from the command (which anyway
only say that they are not implemented)?

Removing these the only thing that Needs to be implemented is %T for
file systems i.e. file system type in human readable form. So to
implement this one would need to find names for the magic numbers in
linux/magic.h.

(Interesting:

#define XENFS_SUPER_MAGIC   0xabba1974

But fortunately no deadbeef.)

Should %T be implemented?

Apart from this I think everything using only the information from
stat/statfs should be implemented (like %t %T for devices). Then there
are %w and %W, which regard file birth time, and for which one would
need an xstat system call... (

https://lwn.net/Articles/397442/

says something about file birth times and links to an article on xstat.)

Felix
___
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net

[Toybox] Yet more find cleanup

[Felix Janda]

A little more cleanup in an attached patch.

-exec does only record whether the exit status of the command is zero or
not.

Some unnecessary use of stat could be removed. (The dirtree functions
already do a stat and save its results.)

If no action is found make the command exactly behave as if the operand
expression was in brackets and a -print was added to the end.
Previously the code for -print was at two places.


In the beginning of find.c there is a comment

/* To Do:
 * -exec action
 */

-exec [...] ; seems to be implemented. Then at the end there is also a
comment

void find_main(void)
{
  /* parse filters, if present */
  build_filter_list();

  /* FIXTHIS - parse actions, if present */

  dirtree_read(TT.dir, check_node_callback);
}

That seems to be obsolete (or I don't understand it at all).

Felix
# HG changeset patch
# User Felix Janda felix.ja...@posteo.de
# Date 1366580735 -7200
# Node ID 96ce8f013db472a194e906facd116e4f4769
# Parent  d1fd3d670ab737c071bd9fe064ad77d03e2d
More find cleanup

diff -r d1fd3d670ab7 -r 96ce8f013db4 toys/pending/find.c
--- a/toys/pending/find.c	Sat Apr 20 17:25:41 2013 +0200
+++ b/toys/pending/find.c	Sun Apr 21 23:45:35 2013 +0200
@@ -48,7 +48,6 @@
 
 GLOBALS(
   char *dir;
-  int have_action;
   struct filter_node *filter_root;
 )
 
@@ -80,18 +79,18 @@
 #define TEST_GT		2
 
 /* executes the command for a filter node
- * return the program return value (0=success)
+   returns 0 for failure or 1 for success
  */
 static int do_exec(struct filter_node *filter, struct dirtree *node)
 {
   char *path = NULL;
-  int plen;
   char **arg_array;
   pid_t pid;
   int status;
 
   arg_array = filter-data.e.exec_args;
   if (filter-data.e.arg_path_index) {
+int plen;
 path = dirtree_path(node, plen);
 arg_array[filter-data.e.arg_path_index] = path;
   }
@@ -100,7 +99,7 @@
   free(path);
   waitpid(pid, status, 0);
 
-  return WEXITSTATUS(status);
+  return !WEXITSTATUS(status);
 }
 
 /* prefix evaluator */
@@ -109,10 +108,6 @@
   struct filter_node **fnext)
 {
   int result;
-  char *path;
-  int plen = 0;
-  struct stat st_buf;
-  time_t node_time;
   int op;
   static int skip = 0;
 
@@ -150,13 +145,7 @@
 return !strcmp(filter-data.name_regex, node-name);
 
   if (op==CHECK_MTIME) {
-// do mtime check
-path = dirtree_path(node, plen);
-result = stat(path,  st_buf);
-free(path);
-if (result0) return 0;
-
-node_time = st_buf.st_mtime/SECONDS_PER_DAY;
+time_t node_time = node-st.st_mtime/SECONDS_PER_DAY;
 result = 1;
 switch (filter-data.t.time_op) {
   /* do time compare here */
@@ -178,16 +167,12 @@
 return result;
 
   }
-  if (op==CHECK_TYPE) {
-path = dirtree_path(node, plen);
-result = lstat(path,  st_buf);
-free(path);
-if (result0) return 0;
-return (st_buf.st_mode  S_IFMT) == filter-data.type;
-  }
+  if (op==CHECK_TYPE) return (node-st.st_mode  S_IFMT) == filter-data.type;
 
 
   if (op==ACTION_PRINT || op==ACTION_PRINT0) {
+char *path;
+int plen = 0;
 char terminator = (op==ACTION_PRINT)*'\n';
 
 path = dirtree_path(node, plen);
@@ -204,7 +189,6 @@
 
 static int check_node_callback(struct dirtree *node)
 {
-  int result;
   struct filter_node *junk;
 
   /* only recurse on . at the root */
@@ -215,15 +199,7 @@
 (node-name[1]=='.'  !node-name[2])))
 return 0;
 
-  result = evaluate(TT.filter_root, node, junk);
-  if (result  !TT.have_action) {
-/* default action is just print the path */
-char *path;
-int plen = 0;
-path = dirtree_path(node, plen);
-printf(%s\n, path);
-free(path);
-  }
+  evaluate(TT.filter_root, node, junk);
   return DIRTREE_RECURSE;
 }
 
@@ -234,12 +210,12 @@
   char **arg;
   int j;
   int prevop = 0;
+  int have_action = 0;
 
   /* part optargs here and build a filter list in prefix format */
 
   TT.dir = .;
   node_list = NULL;
-  TT.have_action = 0;
   for (arg = toys.optargs; *arg; arg++) {
 struct {
   char *arg;
@@ -312,7 +288,7 @@
 break;
 }
 
-TT.have_action |= IS_ACTION(node-op);
+have_action |= IS_ACTION(node-op);
 
 if (node-op == OP_UNKNOWN) {
   if (**arg == '-') error_exit(bad option '%s', *arg);
@@ -339,7 +315,8 @@
   }
 
   /* now convert from infix to prefix */
-  TT.filter_root = NULL;
+  if(have_action) TT.filter_root = NULL;
+  else TT.filter_root = (struct filter_node){0, ACTION_PRINT};
   op_stack = NULL;
   node = node_list;
   while( node ) {
@@ -386,6 +363,12 @@
 TT.filter_root = op_node;
 op_node = op_stack;
   }
+  if(!have_action) {
+node = TT.filter_root;
+TT.filter_root = (struct filter_node*) xmalloc(sizeof(struct filter_node));
+TT.filter_root-next = node;
+TT.filter_root-op = OP_AND;
+  }
 }
 
 void find_main(void)
___
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net

[Toybox] More find cleanup

[Felix Janda]

Hello,

some more find cleanup in an attached patch.

The main change was to make some code in build_filter_list() less
repetitive using a suitable struct and a loop. The command line parsing
is however now less efficient since after an argument has been tested
for equality with different strings like -mtime and the corresponding
type (CHECK_MTIME) has been saved, the saved integer has again to be
checked for equality with stuff to find out what further command line
parsing is necessary.

Apart from this I moved around some variables and got rid of the
unnecessary index i in build_filter_list().

Felix

BTW: POSIX-1.2008 TC1 is out
# HG changeset patch
# User Felix Janda felix.ja...@posteo.de
# Date 1366471541 -7200
# Node ID 9419e28b582891a34eb1734dbec1643e573a1f3d
# Parent  1700c6bfc8f5d87b9f382bb8e334d43bda5f0720
More find cleanup

diff -r 1700c6bfc8f5 -r 9419e28b5828 toys/pending/find.c
--- a/toys/pending/find.c	Thu Apr 18 22:37:09 2013 +0200
+++ b/toys/pending/find.c	Sat Apr 20 17:25:41 2013 +0200
@@ -29,14 +29,6 @@
 #define FOR_find
 #include toys.h
 
-GLOBALS(
-  char *dir;
-)
-
-#define SECONDS_PER_DAY (24*60*60)
-
-int have_action;
-
 struct filter_node {
   struct filter_node *next;
   int op;
@@ -54,7 +46,13 @@
   } data;
 };
 
-static struct filter_node *filter_root;
+GLOBALS(
+  char *dir;
+  int have_action;
+  struct filter_node *filter_root;
+)
+
+#define SECONDS_PER_DAY (24*60*60)
 
 /* filter operation types */
 #define OP_UNKNOWN	0
@@ -75,6 +73,8 @@
 #define ACTION_PRINT0	21
 #define ACTION_EXEC	22
 
+#define IS_ACTION(x)(x = 20)
+
 #define TEST_LT		0
 #define TEST_EQ		1
 #define TEST_GT		2
@@ -204,8 +204,6 @@
 
 static int check_node_callback(struct dirtree *node)
 {
-  char *path;
-  int plen = 0;
   int result;
   struct filter_node *junk;
 
@@ -217,9 +215,11 @@
 (node-name[1]=='.'  !node-name[2])))
 return 0;
 
-  result = evaluate(filter_root, node, junk);
-  if (result  !have_action) {
+  result = evaluate(TT.filter_root, node, junk);
+  if (result  !TT.have_action) {
 /* default action is just print the path */
+char *path;
+int plen = 0;
 path = dirtree_path(node, plen);
 printf(%s\n, path);
 free(path);
@@ -231,93 +231,92 @@
 static void build_filter_list(void)
 {
   struct filter_node *node_list, *op_stack, *node, *op_node, *next;
-  char *arg, **arg_array;
-  int i, j;
+  char **arg;
+  int j;
   int prevop = 0;
 
   /* part optargs here and build a filter list in prefix format */
 
   TT.dir = .;
   node_list = NULL;
-  have_action = 0;
-  for(i=0; toys.optargs[i]; i++) {
-node = (struct filter_node *)
-  xmalloc(sizeof(struct filter_node));
+  TT.have_action = 0;
+  for (arg = toys.optargs; *arg; arg++) {
+struct {
+  char *arg;
+  int op;
+  int extrarg;
+} arg_map[] = {{-o, OP_OR, 0},
+   {-a, OP_AND, 0},
+   {!, OP_NOT, 0},
+   {(, LPAREN, 0},
+   {), RPAREN, 0},
+   {-name, CHECK_NAME, 1},
+   {-mtime, CHECK_MTIME, 1},
+   {-type, CHECK_TYPE, 1},
+   {-print, ACTION_PRINT, 0},
+   {-print0, ACTION_PRINT0, 0},
+   {-exec, ACTION_EXEC, 1}
+};
+mode_t types[]={S_IFREG,S_IFDIR,S_IFCHR,S_IFBLK,S_IFLNK,S_IFSOCK,S_IFIFO};
+char **arg_array;
+node = (struct filter_node *) xmalloc(sizeof(struct filter_node));
 node-op = OP_UNKNOWN;
-arg = toys.optargs[i];
-if (!strcmp(arg, -o)) node-op = OP_OR;
-if (!strcmp(arg, -a)) node-op = OP_AND;
-if (!strcmp(arg, !)) node-op = OP_NOT;
-if (!strcmp(arg, ()) node-op = LPAREN;
-if (!strcmp(arg, ))) node-op = RPAREN;
-
-if (!strcmp(arg, -name)) {
-  node-op = CHECK_NAME;
-  arg = toys.optargs[++i];
-  if (!arg) error_exit(Missing argument to -name);
-  node-data.name_regex = arg;
+for (j=0; j  sizeof(arg_map)/sizeof(*arg_map); j++) {
+  if (!strcmp(*arg, arg_map[j].arg)) {
+node-op = arg_map[j].op;
+if (arg_map[j].extrarg  !*(++arg))
+	   error_exit(Missing argument to %s, arg_map[j].arg);
+	break;
+  }
 }
 
-if (!strcmp(arg, -mtime)) {
-  node-op = CHECK_MTIME;
-  arg = toys.optargs[++i];
-  if (!arg) error_exit(Missing argument to -mtime);
-  switch(arg[0]) {
-case '+':
-  node-data.t.time_op=TEST_GT;
+switch(node-op) {
+  case CHECK_NAME:
+node-data.name_regex = *arg;
+break;
+  case CHECK_MTIME:
+switch(**arg) {
+  case '+':
+node-data.t.time_op=TEST_GT;
+arg++;
+break;
+  case '-':
+node-data.t.time_op=TEST_LT;
+arg++;
+break;
+  default:
+node-data.t.time_op=TEST_EQ;
+break;
+}
+/* convert to days (very crudely) */
+node-data.t.time = atoi(*arg)/SECONDS_PER_DAY;
+break;
+  case CHECK_TYPE

[Toybox] find: Improve operator handling

[Felix Janda]

Hello,

attached is a patch which improves how find processes composite
expressions like for example:

find . -name file -type f -o -name dir -type d


Below is a short description of some aspects of the toy which are
relevant for the patch or have changed with it.

The first step of build_filter_list() is to step through the command
line and fill the linked list node_list with one entry for each
expression and operator; e.g. in the above example we have four
expressions and one operator. I adapted this step to add the implicit
-as where necessary; e.g. two would need to be added for the above
example. The condition when to add -a is unfortunately rather
complicated:

if ((o1MAX_OP  o2MAX_OP) ||
   (o1==RPAREN  o2=OP_NOT  o2!=RPAREN) ||
   (o1=OP_NOT  o1!=LPAREN  o2==LPAREN)) {

To understand this you need to know:

#define OP_UNKNOWN  0
#define OP_OR   2
#define OP_AND  3
#define OP_NOT  4

#define LPAREN  1
#define RPAREN  5

#define MAX_OP  5

#define CHECK_NAME  7
#define CHECK_MTIME 8
#define CHECK_TYPE  9

#define ACTION_PRINT20
#define ACTION_PRINT0   21
#define ACTION_EXEC 22

In the example the resulting list (in words, read from tail to head) is:

-name file -a -type f -o -name dir -a -type d


Ok, after the command line has been processed into node_list it is
brought into a prefix form leaving the order of the expressions intact.
In the above example the prefix form would look like:

-o -a -name file -type f -a -name dir -type d

The algorithm is to walk through the list (which corresponds to walking
backwards through the command line) pushing expressions onto filter_root
and operators (which are not brackets) to op_stack. Depending on the
precendence of the operators, which is encoded in the OP_* and *PAREN
defines, at some points op_stack is emptied onto filter_root.

At the end of build_filter_list() filter_root contains the list of
expressions and operators in prefix form.


On to:

static int evaluate(struct filter_node *filter, struct dirtree *node,
  struct filter_node **fnext)

After having parsed the command line find uses the dirtree functions to
walk the directory tree. For each node evaluate() is called to process
filter_root for the node. evaluate() is a recursive function. It reads
the first part of filter updates *fnext to point to the next part if
the first part was an expression and otherwise calls itself recursively.

-a and -o are here the most interesting since they will not evaluate
their second argument (with its side-effects), if their first argument
evaluated to 0 or 1 respectively. The same applies for conditional
expressions in C but we can't use that because *fnext still needs to be
updated. The relevant code is:

  if (op==OP_OR || op==OP_AND) {
result = evaluate(filter-next, node, fnext);
if(!skip) {
  if (op==OP_OR) {
skip = result;
result = evaluate(*fnext, node, fnext) || result;
  } else {
skip = !result;
result = evaluate(*fnext, node, fnext)  result;
  }
  skip = 0;
}
else result = evaluate(*fnext, node, fnext);
return result;
  }

Here skip is a static int intialized to 0. skip tells evaluate to return
after *fnext has been updated.


find now passes the tests I've written but these miss out still a lot of
stuff which can go wrong.

Cheers,
Felix
# HG changeset patch
# User Felix Janda felix.ja...@posteo.de
# Date 1366317429 -7200
# Node ID 4985115191be966c915d8111da696b14495adc9f
# Parent  a612857eb52d49922508911d2ba853fd8939d0bf
find: Improve operator processing

diff -r a612857eb52d -r 4985115191be toys/pending/find.c
--- a/toys/pending/find.c   Tue Apr 16 22:45:47 2013 -0500
+++ b/toys/pending/find.c   Thu Apr 18 22:37:09 2013 +0200
@@ -58,13 +58,15 @@
 
 /* filter operation types */
 #define OP_UNKNOWN 0
-#define OP_NOT 1   
-#define OP_OR  2   
-#define OP_AND 3   
+#define OP_OR  2
+#define OP_AND 3
+#define OP_NOT 4
 
-#define LPAREN 4 
+#define LPAREN 1 
 #define RPAREN 5 
 
+#define MAX_OP 5
+
 #define CHECK_NAME 7
 #define CHECK_MTIME8
 #define CHECK_TYPE 9
@@ -111,30 +113,43 @@
   int plen = 0;
   struct stat st_buf;
   time_t node_time;
-  char terminator;
+  int op;
+  static int skip = 0;
 
   /* if no filters, success */
   if (!filter) {
 *fnext = NULL;
 return 1;
   }
+  op = filter-op;
 
-  if (filter-op==OP_NOT) return !evaluate(filter-next, node, fnext);
+  if (op==OP_NOT) return !evaluate(filter-next, node, fnext);
 
-  if (filter-op==OP_OR)
-return evaluate(filter-next, node, fnext) || evaluate(*fnext, node, 
fnext);
-
-  if (filter-op==OP_AND)
-return evaluate(filter-next, node, fnext)  evaluate(*fnext, node, 
fnext);
+  if (op==OP_OR || op==OP_AND) {
+result = evaluate(filter-next, node, fnext);
+if(!skip) {
+  if (op==OP_OR

Re: [Toybox] (no subject)

[Felix Janda]

On 04/16/13 at 10:41am, Isaac Dunham wrote:
 ...
 I forget where Landley commented on this, but he mentioned a few issues:

Here:

http://lists.landley.net/pipermail/toybox-landley.net/2013-April/000891.html

Felix
___
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net

[Toybox] Tests for find

[Felix Janda]

Hello,

attached is a patch with a test script for find.

It tests only the expression handling and uses only primaries which are
currently implemented in the find toy.

Felix
# HG changeset patch
# User Felix Janda felix.ja...@posteo.de
# Date 1365936336 -7200
# Node ID c2a5fd17d7885f99d74304458c07245b2b336eea
# Parent  db9878df884a93d895a0662f972e5c8827b0f073
Add tests for find's expression parsing

diff -r db9878df884a -r c2a5fd17d788 scripts/test/find.test
--- /dev/null	Thu Jan 01 00:00:00 1970 +
+++ b/scripts/test/find.test	Sun Apr 14 12:45:36 2013 +0200
@@ -0,0 +1,42 @@
+#!/bin/bash
+
+[ -f testing.sh ]  . testing.sh
+
+mkdir dir
+cd dir
+touch file
+mkfifo fifo
+ln -s fifo link
+cd ..
+
+#testing name command result infile stdin
+
+# Testing operators
+
+testing find -type l -a -type d -o -type p \
+	find dir -type l -a -type d -o -type p dir/fifo\n  
+testing find -type l -type d -o -type p find dir -type l -type d -o -type p \
+	dir/fifo\n  
+testing find -type l -o -type d -a -type p \
+	find dir -type l -o -type d -a -type p dir/link\n  
+testing find -type l -o -type d -type p find dir -type l -o -type d -type p \
+	dir/link\n  
+testing find -type l ( -type d -o -type l ) \
+	find dir -type l \( -type d -o -type l \) dir/link\n  
+testing find extra parantheses \
+	find dir \( \( -type l \) \( -type d -o \( \( -type l \) \) \) \) \
+	dir/link\n  
+testing find ( -type p -o -type d ) -type p \
+	find dir \( -type p -o -type d \) -type p dir/fifo\n  
+testing find -type l -o -type d -type p -o -type f \
+	find dir -type l -o -type d -type p -o -type f | sort \
+	dir/file\ndir/link\n  
+
+# Testing short-circuit evaluations
+
+testing find -type f -a -print \
+	find dir -type f -a -print dir/file\n  
+testing find -print -o -print \
+	find dir -type f -a \( -print -o -print \) dir/file\n  
+
+rm -rf dir
___
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net

Re: [Toybox] Broken find [Was: Re: Towards find cleanup]

[Felix Janda]

...
 I'm not following this.
 
 Rob

I'm sorry for the confusion. I was talking about the evaluate() function
in find.c (should have added the brackets ()) which evaluates a pre-fix
expression (which is not directly the command line to find). But Tim's
mail has cleared that up, hasn't it?

Felix
___
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net

Re: [Toybox] Broken find [Was: Re: Towards find cleanup]

[Felix Janda]

On 04/11/13 at 05:53pm, Tim Bird wrote:
 On 04/11/2013 03:12 PM, Rob Landley wrote:
  On 04/11/2013 04:37:36 PM, Felix Janda wrote:
  Now I think that I know better how this toy works and see that my  
  cleanup
  has broken it.
 
  if (filter-op==OP_OR) {
  -   result = evaluate(filter-next, node, fnext);
  -   result2 = evaluate(*fnext, node, fnext);
  -   if (result) {
  -   return result;
  -   } else {
  -   if (result2) {
  -   return result2;
  -   } else {
  -   return 0;
  -   }
  -   }
  +   return evaluate(filter-next, node, fnext) ||  
  evaluate(*fnext, node, fnext);
  This will have different side effects than the original code.
  The original code evaluates both sides, and then returns the 'or'  
  operation
  on the results.  The new one will (of course) do short-circuit  
  evaluation of
  the '||',  meaning that sometimes the second node won't be  
  evaluated.
  I believe that at one time, I actually had it coded that way, but  
  for some
  reason or other I went back to forcing the evaluation of both  
  nodes.
  Unfortunately, I can't remember the reason why I went back.  I  
  would have expected
  regular find to do short-circuit evaluation of expressions with  
  'or' operations,
  so I'd be happy if this were correct.  I just have a nagging  
  feeling that
  there was some reason I couldn't do it this way.  I could be  
  completely wrong,
  though.  So unless testing reveals the need for evaluating both  
  sides of the
  'or', this should be fine.
 
  The reason is that we want the side effects evaluate has on *fnext. If
  we have a prefix expression like
 
  OR AND CONDITION1 CONDITION2 CONDITION3
  
  You can have and/or prefix expressions?
  
  $ find . -or -exec echo one {} \; -exec echo two {} \; | grep two
  find: invalid expression; you have used a binary operator '-or' with  
  nothing before it.
  
  and CONDITION1 is false, the current version of evaluate will
  1. see OR
  2. descend to AND
  3. find that CONDITION1 is false (side effect: *fnext=CONDITION2)
  4. don't evaluate *fnext but go back to OR
  5. OR now checks CONDITION2 instead of CONDITION3.
  
  I'm not following this at all.
  
  I did however try this:
  
 find . -exec echo one {} \; -or -exec echo two {} \; | grep two
  
  It does not print two for any of the files, so -or is successfully  
  short-circuiting. This is in the gnu/dammit version.
  
  In the previous version of evaluate in step 4 CONDITION2 would have  
  been
  evaluated, ignored and as a side effect fnext would then point to
  CONDITION3.
  
  Which behavior is right, and can you give me a test command line I can  
  try in the other implementation?
  
 
  Do you know if regular find does short-circuit evaluation?
  What would regular find do with
'find . -exec test1_with_side_effects {} \; -o -exec  
  test2_with_side_effects {} \;'
  ?
 
  POSIX says that these short-circuit evaluations shall be applied.  
  (The
  same applies for -a.)
 
  Now we have the problem that we want to have some side effects of
  evaluate but not all of them.
  
  I'm not following this.
 
 The prefix evaluation is an artifact of the implementation.
 find expects and infix expression on the command line.  This find
 converts that to prefix during parsing.  I did this because I
 thought it would be easier to evaluate (with my weird stateful
 evaluator :( )
 
 I think the easiest solution is to keep the pre-fix format of
 the parsed expression, but handle short-circuiting directly.
 So OR would become something like:
 if (filter-op==OP_OR) {
   result = evaluate(filter-next, node, fnext);
   if (result) {
   skip(*fnext, node, fnext);
   return result;
   } else
   return evaluate(*fnext, node, fnext);
 }

Yeah, I also thought of something like that. Maybe instead of
implementing skip() add an extra parameter to evaluate() (or use a
static variable).

 Of maybe the expression tree should actually be put into a binary
 tree form, instead of serial pre-fix form, for easier traversal.

It confused me a bit when first looking at the code. IMO there should
be a comment which explains the fnext argument to evaluate().

I think that the implementation using the prefix form is really simple.
There are however some details of the evaluation which are different from
what POSIX wants to have:

1. We've already talked about the short-cut evaluations and Tim has given
a possible solution to it.

2. expression1 expression2 should behave like expression1 -a
expression2. (This also already came up.) It should be possible to
insert the missing AND-operators in build_filter_list(), shouldn't it?

3. Operator precedence. POSIX prescribes that from highest to lowest
precendence we have:

( )
!
-a
-o

Re: [Toybox] Towards find cleanup

[Felix Janda]

On 04/10/13 at 07:34pm, Rob Landley wrote:
snip
  I think that some function parameters should be made const and that
  the code could be made less repetitive.
 
 I specify static because it allows the compiler to produce better  
 code, but I've never found const to actually produce better code. The  
 compiler's optimizer does liveness analysis, it can tell when you don't  
 actually change a value after it's assigned to. In theory const lets  
 the compiler do some of this across translation units, but that's what  
 link time optimization is for these days.
 
 In addition, const's syntax is unfortunately subtle when pointers are  
 involved. Is the pointer what's const, is what it points to that's  
 const (it binds left except at the left edge...), how about a pointer  
 to a pointer, or pointer to an array...
 
 I also haven't seen it interface well with the actual read-only  
 sections ELF can produce. There's an -mebedded-data compiler flag that  
 says to put data in the .rodata section. The main use of .rodata is to  
 strings constants in there by default. Yet those strings aren't  
 const, you can assign a string constant to a regular char * without  
 the compiler ever complaining. (And not being able to do so would  
 pretty fundamentally break C.)
 
 What const really seems like to me is a leakage from C++'s public,  
 private, protected into C. It's a language facility that assumes  
 programmers can't trust themselves or their successors to get it right.

I'd thought telling the compiler stuff can't hurt. Thanks for the
detailed explainations.

 So I tend to remove const from the code when I can. It doesn't  
 justify its existence: if we're not supposed to modify the value from  
 here, then don't do it. I trust the programmer to not suck, and I trust  
 reviewers to catch it when they screw up.

I'll keep that in mind.

 Rob

Felix
___
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net

[Toybox] Broken find [Was: Re: Towards find cleanup]

[Felix Janda]

Now I think that I know better how this toy works and see that my cleanup
has broken it.

 if (filter-op==OP_OR) {
   - result = evaluate(filter-next, node, fnext);
   - result2 = evaluate(*fnext, node, fnext);
   - if (result) {
   - return result;
   - } else {
   - if (result2) {
   - return result2;
   - } else {
   - return 0;
   - }
   - }
   + return evaluate(filter-next, node, fnext) || evaluate(*fnext, 
   node, fnext);
  This will have different side effects than the original code.
  The original code evaluates both sides, and then returns the 'or' operation
  on the results.  The new one will (of course) do short-circuit evaluation of
  the '||',  meaning that sometimes the second node won't be evaluated.
  I believe that at one time, I actually had it coded that way, but for some
  reason or other I went back to forcing the evaluation of both nodes.
  Unfortunately, I can't remember the reason why I went back.  I would have 
  expected
  regular find to do short-circuit evaluation of expressions with 'or' 
  operations,
  so I'd be happy if this were correct.  I just have a nagging feeling that
  there was some reason I couldn't do it this way.  I could be completely 
  wrong,
  though.  So unless testing reveals the need for evaluating both sides of the
  'or', this should be fine.

The reason is that we want the side effects evaluate has on *fnext. If
we have a prefix expression like

OR AND CONDITION1 CONDITION2 CONDITION3

and CONDITION1 is false, the current version of evaluate will
1. see OR
2. descend to AND
3. find that CONDITION1 is false (side effect: *fnext=CONDITION2)
4. don't evaluate *fnext but go back to OR
5. OR now checks CONDITION2 instead of CONDITION3.

In the previous version of evaluate in step 4 CONDITION2 would have been
evaluated, ignored and as a side effect fnext would then point to
CONDITION3.

  
  Do you know if regular find does short-circuit evaluation?
  What would regular find do with
'find . -exec test1_with_side_effects {} \; -o -exec 
  test2_with_side_effects {} \;'
  ?
 
 POSIX says that these short-circuit evaluations shall be applied. (The
 same applies for -a.)

Now we have the problem that we want to have some side effects of
evaluate but not all of them.

Felix
___
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net

Re: [Toybox] Towards find cleanup

[Felix Janda]

Hi Tim,

thanks for your feedback.

On 04/10/13 at 11:12am, Tim Bird wrote:
 ...
  The cleanup was mainly mechanical and I still don't understand the toy
  well. (Before and after the cleanup) there seems to be a problem with
  the rule parsing/interpretation. For example
  
  find . \( -type f -type f \) -o -type d
  
  does not print directories.
 
 Hmmm.  I'm not sure that's a legal filter expression.  what is
 -type f -type f?  Is there supposed to be an operator (-a or -o)
 in there?

 In general, regular find may be a little more liberal interpreting
 filter expressions than the find toy.  I think that regular find
 will often imply an 'and' when no operator is specified.  I considered
 this for the find toy, but didn't want to gum up the parser with weird
 handling.  I know that lots of times people do things like string a bunch
 of filters together without 'and's, like so:
 find . -type f -name *.o -mtime -20

 These types of sequential and's ought to be supported.  But allowing
 for unspecified 'and's in weird places in a complex parse expression
 seems like overkill for a toy.

POSIX says that -a is optional. Adding in -a does still not print
directories.

 There used to be debug code in the find toy to dump the expression tree,
 but Rob removed it.  (I'm not complaining, I think the code should have
 been removed eventually).  To debug this, please use a debugger to make
 sure the filter expression tree matches what you think it should be based
 on the command line, then verify that the actually filtering (expression
 evaluation) proceeds as desired.
 
 With the missing operator between '-type f' and '-type f' above, I'm not
 even sure what the desired expression is.

Thanks for the debugging hints.

  The toy also does not follow the whitespace conventions in toybox. But I
  think that someone has scripts lying around to fix that.
  
  I think that some function parameters should be made const and that
  the code could be made less repetitive. 
 
 Thanks very much for working on this.  I haven't had time to look
 at it myself, and I'm very glad to see someone taking it up. :-)
 'find' is one of the most significant omissions in toolbox, and one
 of the most useful things to have in the toy arsenal (IMHO).
 
 I'll send some comments on the patch in another mail.

I'm looking forward it.

  -- Tim

Felix
___
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net

Re: [Toybox] Towards find cleanup

[Felix Janda]

Tim, thanks for your thourough review.

Some (rather selective) comments:

Regarding the == 0 vs ! thing: Maybe I was too eager here. I think
that it does not make a great difference in readability but should be
uniform along in all the toys. Grepping in toys/posix ! is used
much more often but there are some occurences of == 0.  Let's hear
Rob...

On 04/10/13 at 11:55am, Tim Bird wrote:
 snip
  if (filter-op==OP_OR) {
  -   result = evaluate(filter-next, node, fnext);
  -   result2 = evaluate(*fnext, node, fnext);
  -   if (result) {
  -   return result;
  -   } else {
  -   if (result2) {
  -   return result2;
  -   } else {
  -   return 0;
  -   }
  -   }
  +   return evaluate(filter-next, node, fnext) || evaluate(*fnext, 
  node, fnext);
 This will have different side effects than the original code.
 The original code evaluates both sides, and then returns the 'or' operation
 on the results.  The new one will (of course) do short-circuit evaluation of
 the '||',  meaning that sometimes the second node won't be evaluated.
 I believe that at one time, I actually had it coded that way, but for some
 reason or other I went back to forcing the evaluation of both nodes.
 Unfortunately, I can't remember the reason why I went back.  I would have 
 expected
 regular find to do short-circuit evaluation of expressions with 'or' 
 operations,
 so I'd be happy if this were correct.  I just have a nagging feeling that
 there was some reason I couldn't do it this way.  I could be completely wrong,
 though.  So unless testing reveals the need for evaluating both sides of the
 'or', this should be fine.
 
 Do you know if regular find does short-circuit evaluation?
 What would regular find do with
   'find . -exec test1_with_side_effects {} \; -o -exec 
 test2_with_side_effects {} \;'
 ?

POSIX says that these short-circuit evaluations shall be applied. (The
same applies for -a.)

Felix
___
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net

Re: [Toybox] Add remaining pwd options

[Felix Janda]

Sorry for this repeated hair-splitting.

On 01/12/13 at 11:33pm, Rob Landley wrote:
 On 01/10/2013 02:25:13 PM, Felix Janda wrote:
  On 01/02/13 at 12:41am, Rob Landley wrote:
   What I did was disable #3 in the case where cwd doesn't exist. So  
  the
   new rule #3 is:
  
   3) If cwd exists and $PWD doesn't point to it, fall back to -P.
  
  Thanks for the clarification.
  
  Your version of 3) depends on whether pwd is builtin or not. Do you  
  mean
  something like If getcwd() fails ...?
 
 cwd is what getcwd() returns. $PWD is an environment variable.

I wanted to differentiate between the current working directory and its
name. Doesn't an unliked current working still exist for the processes
its the cwd of? (I was wrong about that 3) depends on whether pwd in
builtin or not since child processes inherit cwd.)

BTW, in the case that one has deleted and recreated one's current
working
directory one could also use cd . to get to the new directory.
  
   Good to know. (This means the shell is special casing . as well as
   ... I need to read the susv4 shell stuff thoroughly, it's been
   years...)
  
  The susv4 page special cases . and .. a bit, but it seems to me  
  only
  in the $CDPATH handling. Ah, I see that you don't care about $CDPATH  
  from
  the about page.
 
 $CDPATH and $PWD are separate.

I just read

http://landley.net/toybox/about.html:
 And some things (like $CDPATH support in cd) await a good explanation
 of why to bother with them.

and interpreted it as a reluctance to implement $CDPATH support.

 
  Then I think one can leave out step 5 on susv4's page on
  cd, and cd . is no more special than cd dir; it does a chdir to  
  $PWD/.
  or $PWD/dir respectively and then updates $PWD to its canonical  
  form. (and
  modifies $OLDPWD also if necessary)
 
 Um, steps 4 and 8 are the ones that say cd . and .. are special?

Step 4 means that $CDPATH shouldn't be taken into account when you do
something like cd ./dir or cd ../dir.

In Step 8 the usual formal processes of simplifying a path (by removing .
dot components and so on) described.

Of course here . and .. are treated specially, but this treatment
affects only $PWD, since chdir(/some/dir/.) should do the same as
chdir(/some/dir).

Step 9 looks like fun...

  Another interesting situation is if your current directory /dir has  
  been
  moved to /olddir and say /dir has been recreated. Then cd .  
  will move
  you to new directory whereas cd $(pwd -P) will preserve your cwd  
  and fix up
  $PWD. (at least for a shell behaving posixly correct)
 
 Preserving the cwd is what I wanted to do, yes.

  Imagine the same situation but with /dir not being recreated after  
  being
  moved. Then cd . should fail according to susv4 since $PWD/. =  
  /dir/.,
  which does not exist. Would you like to have cd . behave the same as
  cd $(pwd) in this case? Bash does this if not in POSIX mode.  
  Busybox ash
  doesn't do this and for some reason even cd $(pwd) fails.
 
 I want the great mass of existing shell scripts to work, which means  
 reproducing historical behavior. Posix is (mostly) a reasonable  
 consensus documentation of historical behavior.

Ok

Felix
___
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net

Re: [Toybox] Add remaining pwd options

[Felix Janda]

On 12/30/12 at 05:47pm, Rob Landley wrote:
 On 12/30/2012 05:16:41 AM, Felix Janda wrote:
  On 12/30/12 at 04:43am, Rob Landley wrote:
  POSIX contains many surprises. In the section on environment  
  variables it
  says that $PWD should be set if pwd -P was specified. What happens  
  if an
  error happens seems unspecified.

Sorry, this is wrong. It has been changed between SUSV4 and SUSV3. Now pwd
must not change $PWD. (It would be really nice to have SUSV4 man pages...)

 Translation: pwd must be a shell builtin running within the shell's  
 process ID, and cannot sanely be implemented any other way.

 It would be nice if they would just _identify_ these. I did a pass to  
 find them (in the roadmap), but missed this.

I agree that it's sensible to have it as a builtin. I'm still not sure
whether an external implementation can't be sane, though.

Let's go back to the situation of a directory /dir deleted in a subshell.
What is then the path name of the current working directory of the shell?
(I'd say it's undefined.) Both getcwd() and stat(/dir) fail in this
situation for both the shell and external commands. Does the builtin pwd
have any advantage over the external pwd in making sure that $PWD is sane?

   Sigh. And the whole PWD defaults to -P unless POSIXLY_CORRECT  
  thing
   above: while I'm sure that code is in there, it's not actually what
   it's doing here. Because GNU code is INSANE, and someone somewhere
   thought this tangle of corner cases might help somehow.
  
   Right, in the case of a deleted directory $PWD is all we've got, so
   have -L (which is the default) print it but first validate it's an
   absolute path with no .. in it. Only validate that current directory
   and path directory point to the same place if there IS a current
   directory. If that's not what they want, -P exists.
  
  In the corner case shouldn't pwd (-L and -P) just give an error  
  message?
  ($PWD does not contain an absolute pathname of the current working
  directory.)
 
 If something deletes the directory you're working in, cd .. should  
 work if the directory above you exists. That can't happen if $PWD isn't  
 there.

What exactly is the relation of this to the pwd command? cd .. should
call chdir() with $PWD/.. after canonicalization. On contrast to pwd, cd
_has_ to be builtin since a chdir() in a child process won't affect the
parent shell.

 Also, when a directory gets deleted and recreated I do cd $(pwd) all  
 the time. It's useful to still have pwd if some other process takes out  
 the directory you're in.

Ok, I see that this is handy. Alternative one could use cd $PWD.
I find that this application really contradicts POSIX since here . and
$PWD are completely different directories.


Your fun corner case is still strange. From playing a bit around bash seems
to keep the PWD in addition to the environment variable somewhere internally
(pwd still works after unsetting $PWD.) On the other hand pwd -P seems to
reset this internal state for some reason. Maybe it's a bug. dash also seems
to keep some internal state, but pwd still works after pwd -P has failed.

Felix
___
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net

[Toybox] Add remaining pwd options

[Felix Janda]

Hi,

the first patch adds the -L and -P options to pwd as specified by POSIX.
The test script again uses stat. This time in order to get inode numbers
of directories.

Felix
# HG changeset patch
# User Felix Janda felix.ja...@posteo.de
# Date 1356627399 -3600
# Node ID 592dab5e536c053ac8b8696f368045f76c8a30b9
# Parent  017b8fd3c9ac5a86dd849831622c4878fddebe5d
Add options -L and -P to pwd.

diff -r 017b8fd3c9ac -r 592dab5e536c toys/posix/pwd.c
--- a/toys/posix/pwd.c	Wed Dec 26 19:39:51 2012 -0600
+++ b/toys/posix/pwd.c	Thu Dec 27 17:56:39 2012 +0100
@@ -3,26 +3,34 @@
  * Copyright 2006 Rob Landley r...@landley.net
  *
  * See http://opengroup.org/onlinepubs/9699919799/utilities/echo.html
- *
- * TODO: add -L -P
 
-USE_PWD(NEWTOY(pwd, NULL, TOYFLAG_BIN))
+USE_PWD(NEWTOY(pwd, 0LP[!LP], TOYFLAG_BIN))
 
 config PWD
   bool pwd
   default y
   help
-usage: pwd
+usage: pwd [-L|-P]
 
 The print working directory command prints the current directory.
+
+-P  Avoid all symlinks
+-L  Use the value of the environment variable PWD if valid
+
+The option -L is implied by default.
 */
 
+#define FOR_pwd
 #include toys.h
 
 void pwd_main(void)
 {
-  char *pwd = xgetcwd();
+  char *pwd = xgetcwd(), *env_pwd;
+  struct stat st[2];
 
-  xprintf(%s\n, pwd);
+  if (!(toys.optflags  FLAG_P)  (env_pwd = getenv(PWD)) 
+!stat(pwd, st[0])  !stat(env_pwd, st[1]) 
+(st[0].st_ino == st[1].st_ino)) xprintf(%s\n, env_pwd);
+  else xprintf(%s\n, pwd);
   if (CFG_TOYBOX_FREE) free(pwd);
 }
# HG changeset patch
# User Felix Janda felix.ja...@posteo.de
# Date 1356729021 -3600
# Node ID f5b0f21ef92f73e13c3415d8449be86d9c531186
# Parent  dbf0480c88f4895724d719738c7d75ffc9f6c957
Add some tests for pwd.

diff --git a/scripts/test/pwd.test b/scripts/test/pwd.test
new file mode 100755
--- /dev/null
+++ b/scripts/test/pwd.test
@@ -0,0 +1,26 @@
+#!/bin/bash
+
+[ -f testing.sh ]  . testing.sh
+
+#testing name command result infile stdin
+
+#TODO: Find better tests
+
+testing pwd [ $(stat -c %i $(pwd)) = $(stat -c %i .) ]  echo yes \
+	yes\n  
+testing pwd -P [ $(stat -c %i $(pwd -P)) = $(stat -c %i .) ]  echo yes \
+	yes\n  
+
+
+ln -s . sym
+cd sym
+testing pwd [ $(stat -c %i $(pwd)) = $(stat -c %i $PWD) ]  echo yes \
+	yes\n  
+testing pwd -P [ $(stat -c %i $(pwd -P)) = $(stat -c %i $PWD) ] || echo yes \
+	yes\n  
+cd ..
+rm sym
+
+export PWD=walrus
+testing pwd (bad PWD) [ $(pwd) = $(cd . ; pwd) ]  echo yes \
+	yes\n  
___
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net

Re: [Toybox] [groups] : ! More than one in group is error

[Felix Janda]

When trying to implement the remaining options for pwd I could also test
toybox's group option handling.

The option string I wanted to use is: 0LP[!LP]
(It's the same for LP[!LP].)

With the current argparsing I get a segfault for pwd -L -P. When enabling
TOYBOX_DEBUG it says pwd: trailing [!LP]. On the other hand when using
Ashwini's suggestion with disabled TOYBOX_DEBUG everthing seems to work,
whereas with enabled TOYBOX_DEBUG I get the same error message as before.

Felix
___
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net

[Toybox] Tests for mkfifo

[Felix Janda]

Similar patches for mkfifo. mkfifo -m was actually already implemented
although the comments in mkfifo.c said it still had to be done. The tests
are very similar to the mkdir ones.

Felix
# HG changeset patch
# User Felix Janda felix.ja...@posteo.de
# Date 1356627134 -3600
# Node ID 6554a8b33e82310673e775845beee9eb2c375bde
# Parent  017b8fd3c9ac5a86dd849831622c4878fddebe5d
mkfifo -m is already implemented.

diff -r 017b8fd3c9ac -r 6554a8b33e82 toys/posix/mkfifo.c
--- a/toys/posix/mkfifo.c	Wed Dec 26 19:39:51 2012 -0600
+++ b/toys/posix/mkfifo.c	Thu Dec 27 17:52:14 2012 +0100
@@ -3,8 +3,6 @@
  * Copyright 2012 Georgi Chorbadzhiyski geo...@unixsol.org
  *
  * See http://opengroup.org/onlinepubs/9699919799/utilities/mkfifo.html
- *
- * TODO: Add -m
 
 USE_MKFIFO(NEWTOY(mkfifo, 1m:, TOYFLAG_BIN))
 
@@ -30,9 +28,7 @@
   char **s;
 
   TT.mode = 0666;
-  if (toys.optflags  FLAG_m) {
-TT.mode = string_to_mode(TT.m_string, 0);
-  }
+  if (toys.optflags  FLAG_m) TT.mode = string_to_mode(TT.m_string, 0);
 
   for (s = toys.optargs; *s; s++) {
 if (mknod(*s, S_IFIFO | TT.mode, 0)  0) {
# HG changeset patch
# User Felix Janda felix.ja...@posteo.de
# Date 1356627321 -3600
# Node ID 7b602ab6a797d57aedccf9bc69963eccbef57055
# Parent  017b8fd3c9ac5a86dd849831622c4878fddebe5d
Add tests to mkfifo based on tests for mkdir.

diff --git a/scripts/test/mkfifo.test b/scripts/test/mkfifo.test
new file mode 100755
--- /dev/null
+++ b/scripts/test/mkfifo.test
@@ -0,0 +1,28 @@
+#!/bin/bash
+
+[ -f testing.sh ]  . testing.sh
+
+#testing name command result infile stdin
+
+testing mkfifo mkfifo one  [ -p one ]  echo yes yes\n  
+rm one
+
+touch existing
+testing mkfifo existing \
+	mkfifo existing 2 /dev/null || [ -f existing ]  echo yes yes\n  
+rm existing
+
+testing mkfifo one two \
+	mkfifo one two  [ -p one ]  [ -p two ]  echo yes yes\n  
+rm one two
+
+umask 123
+testing mkfifo (default permissions) \
+	mkfifo one  stat -c %a one 644\n  
+rm one
+
+umask 000
+
+testing mkfifo -m 124 \
+	mkfifo -m 124 one  stat -c %a one 124\n  
+rm -f one
___
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net

Re: [Toybox] More expand cleanups

[Felix Janda]

On 12/01/12 at 12:42am, Rob Landley wrote:
 On 11/30/2012 12:56:06 PM, Felix Janda wrote:
  On 11/30/12 at 02:51am, Rob Landley wrote:
   I updated wc to theoretically deal with buffer wraps better. In  
  reality
   I haven't got UTF8 test data to run through this, and should  
  probably
   find some at some point.
  
  Didn't the original version already deal well with it. (That's what  
  the
  strange tests are about.) Notice that the C library keeps an internal
  state which is updated each time mbrtowc is called. If there's only  
  part
  of a character on the end mbrtowc will return -2, but remember the  
  part.
  To continue you just have to read a new toybuf and point mbrtowc to  
  the
  _new_ data. (The r in mbrtowc stands for restartable.)
 
 Huh, I didn't know that was what it was doing. Let me go back to your  
 code then.
 
  Do the tests still work?
 
 Yes, but nothing in the test suite tests a buffer wrap. Still, if libc  
 already handles this I'm happy to let it do so.

The $(seq 1 8192)s are exactly in order to produce something bigger than
sizeof(toybuf). The first test uses a string with the first byte a normal
char and then 8192 double byte chars. By some luck the test still produces
the right results with your version. If you build a debug statement to detect
invalid chars into wc, you will see that mbrtowc encounters an invalid byte
sequence although the input is valid.

(As you have written in your commit message invalid byte sequences are
ignored in both versions.)

   This also assumes that all characters are the same width, which is
   probably wrong and I need help with if so. (I dunno how to do
   fontmetrics here?)
  
  I think that this depends on the terminal emulator. Look for example  
  at the
  -cjk_width option of xterm.
 
 I mean can UTF-8 produce characters that are different sizes in the  
 terminal? If the answer is no then i don't have to worry. If it can  
 produce double-wide characters or similar, I'd have to deal with that.

Yes, look at that option of xterm. Depending on whether it is set CJK
characters take 1 or 2 columns in xterm.

I wouldn't try to detect this...

 I know the terminal can run right to left, but that's symmetrical and I  
 don't have to care from a programming standpoint, I think...
 
  Felix
 
 Rob
___
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net

Re: [Toybox] More expand cleanups

[Felix Janda]

On 11/30/12 at 02:51am, Rob Landley wrote:
 I updated wc to theoretically deal with buffer wraps better. In reality  
 I haven't got UTF8 test data to run through this, and should probably  
 find some at some point.

Didn't the original version already deal well with it. (That's what the
strange tests are about.) Notice that the C library keeps an internal
state which is updated each time mbrtowc is called. If there's only part
of a character on the end mbrtowc will return -2, but remember the part.
To continue you just have to read a new toybuf and point mbrtowc to the
_new_ data. (The r in mbrtowc stands for restartable.)

Do the tests still work?

 I redid the actual expand function to be simpler: read data into toybuf  
 and then write it to stdout using either fputc(char, stdout) or  
 xprintf(%*c, len, ' ') depending on whether it's a tab or something  
 else. It checks for tab (trigger the space behavior) and newline (reset  
 counters).
 
 What it does _not_ currently do is track spaces advanced separately  
 from bytes advanced, that needs the utf8 stuff to grab groups of  
 bytes that represent a single character, and to make _that_ work I need  
 to copy the logic I just added to wc, which means maybe I should  
 genericize it into lib/lib.c somehow? Needs more thought.

 This also assumes that all characters are the same width, which is  
 probably wrong and I need help with if so. (I dunno how to do  
 fontmetrics here?)

I think that this depends on the terminal emulator. Look for example at the
-cjk_width option of xterm.

Felix
___
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net

[Toybox] New tests for dirname and wc

[Felix Janda]

Hello,

attached are some simple tests for dirname and wc and
a fix for a small typo in another test script.

Posix specifies an -m option for wc, which toybox does
not implement. Should there be a test for this, too?

Why do the scripts actually use bash?

Regards,
Felix
diff -r b88859043af2 -r 7f41c9c49509 scripts/test/cmp.test
--- a/scripts/test/cmp.test Mon Sep 17 00:17:16 2012 -0500
+++ b/scripts/test/cmp.test Sat Oct 27 22:29:02 2012 +0200
@@ -1,4 +1,4 @@
-#/bin/bash
+#!/bin/bash
 
 [ -f testing.sh ]  . testing.sh
 
diff -r 88fa9133995e -r 929003d8c4f9 scripts/test/dirname.test
--- /dev/null   Thu Jan 01 00:00:00 1970 +
+++ b/scripts/test/dirname.test Sun Oct 28 00:06:38 2012 +0200
@@ -0,0 +1,10 @@
+#!/bin/bash
+
+[ -f testing.sh ]  . testing.sh
+
+#testing name command result infile stdin
+
+testing dirname /-only dirname /// /\n  
+testing dirname trailing / dirname a// .\n  
+testing dirname combined dirname /a///b///c///d/ 
/a///b///c\n   
+testing dirname /a/ dirname /a/// /\n  
diff -r 7f41c9c49509 -r 88fa9133995e scripts/test/wc.test
--- /dev/null   Thu Jan 01 00:00:00 1970 +
+++ b/scripts/test/wc.test  Sat Oct 27 23:52:43 2012 +0200
@@ -0,0 +1,22 @@
+#!/bin/bash
+
+[ -f testing.sh ]  . testing.sh
+
+#testing name command result infile stdin
+
+cat file1 EOF
+some words . 
+
+some
+lines
+EOF
+
+testing wc wc /dev/null  echo yes yes\n  
+testing wc empty file wc 0 0 0\n  
+testing wc standard input wc 1 3 5\n  a b\nc
+testing wc -c wc -c file1 26 file1\n  
+testing wc -l wc -l file1 4 file1\n  
+testing wc -w wc -w file1 5 file1\n  
+testing wc format wc file1 4 5 26 file1\n  
+testing wc multiple files wc input - file1 1 2 3 input\n0 2 3 -\n4 5 26 
file1\n5 9 32 total\n a\nb a b
+rm file1
___
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net