[BusyBox] dietlibc patch for pre5

[Manuel Novoa III]

Hello,

On Fri, Jan 30, 2004 at 03:06:19PM +0100, Hannes Reinecke wrote:
> Hi all,
> 
> I've whacked together a patch for dietlibc support for -pre5. It 
> basically adds a new configuration option and includes some #defines 
> around mempcpy() and strchrnul(), both of which do not exist within 
> dietlibc.

Not really sure if a new configuration option is actually needed,
since __dietlibc__ can already be tested for.  Regarding the missing
functions, see my comments below.

> btw, I've also fixed an 'interesting' use of strchrnul:
> 
> 			(*vp->func)(strchrnul(s, '=') + 1);
> 
> I.e. if 's' does not contain the character '=', the argument to the 
> function will be a pointer to 's + strlen(s) + 1'. Uh-oh.

I'm sure vodz will take a look.

> Comments, suggestions etc. welcome.
> 
> Please keep me cc'ed since I'm not on the list.
> 
> Cheers,
> 
> Hannes
> -- 

Ok.  Here are my comments on your patch.  I'd be happy to discuss
things with you.

> Index: coreutils/tail.c
> ===================================================================
> RCS file: /var/cvs/busybox/coreutils/tail.c,v
> retrieving revision 1.46
> diff -u -r1.46 tail.c
> --- coreutils/tail.c	31 Oct 2003 00:35:58 -0000	1.46
> +++ coreutils/tail.c	30 Jan 2004 12:53:34 -0000
> @@ -62,7 +62,13 @@
>  static void tail_xprint_header(const char *fmt, const char *filename)
>  {
>  	/* If we get an output error, there is really no sense in continuing. */
> -	if (dprintf(STDOUT_FILENO, fmt, filename) < 0) {
> +	if (bb_full_write(STDOUT_FILENO, "\n===> ", 6) < 0) {
> +		bb_perror_nomsg_and_die();
> +	}
> +	if (bb_full_write(STDOUT_FILENO, filename, strlen(filename)) < 0) {
> +		bb_perror_nomsg_and_die();
> +	}
> +	if (bb_full_write(STDOUT_FILENO, " <===\n", 6) < 0) {
>  		bb_perror_nomsg_and_die();
>  	}
>  }

A better approach would be to add a dprintf support function to libbb
to be used if the relevant libc didn't supply one.  The above is not 
acceptable, as it unnecessarily bloats code when using more featureful
libcs.

> Index: libbb/printf.c
> ===================================================================
> RCS file: /var/cvs/busybox/libbb/printf.c,v
> retrieving revision 1.2
> diff -u -r1.2 printf.c
> --- libbb/printf.c	16 Apr 2003 23:02:35 -0000	1.2
> +++ libbb/printf.c	30 Jan 2004 12:53:35 -0000
> @@ -116,7 +116,23 @@
>   * you can extract the information you need from dietstdio.h.  See the
>   * other library implementations for examples.
>   */
> -#error dietlibc is currently not supported.  Please see the commented source.
> +
> +struct __stdio_file {
> +  int fd;
> +  int flags;
> +  unsigned int bs;	/* read: bytes in buffer */
> +  unsigned int bm;	/* position in buffer */
> +  unsigned int buflen;	/* length of buf */
> +  char *buf;
> +  struct __stdio_file *next;	/* for fflush */
> +  pid_t popen_kludge;
> +  unsigned char ungetbuf;
> +  char ungotten;
> +};
> +
> +#define ERRORINDICATOR 1
> +
> +#define SET_FERROR_UNLOCKED(S)    (((struct __stdio_file *)S)->flags |= ERRORINDICATOR)
>  
>  #else /* some other lib */
>  /* Please see the comments for the above supported libaries for examples

Well, I can live with something like that as long as a dietlib user wants
to support it.  Actually, specifying the first couple of fields is all
that's necessary.

I'd still want to keep a warning for the time being though, since the
whole intent of the code in this file is reliability when using stdio
and dietlibc's stdio "isn't there yet".  In fact, I received the
following as part of an email from Felix on Nov 14, 2003 as response
to some of my attempts to get him to look a number of dietlibc stdio
files in particular.

              As you might have noticed, I am not using
  stdio _at all_ in my code.  The result is that I am not very familiar
  with the APIs and standards, and I'm not going to learn the API just to
  implement it correctly, if implementing it badly is sufficient to run
  100% of the programs that I am linking with the diet libc (which is
  about 98% of the programs on my system that I'm working with, except X
  programs).  Unless someone complains (and has a valid complaint), I'm
  not adhering to specs just because they are there.  That was exactly the
  premise of trying to write a libc, because I was sure that I wouldn't
  make it at all had I only settled for perfection in every piece of code
  from the start.

I've been sitting on a number of stdio-related dietlibc bugs, waiting
for the developers and users to make more of an effort in testing.
Fortunately, this seems to be happening and I'll likely submit details
on those bugs in the near future.

> Index: networking/ping.c
> ===================================================================
> RCS file: /var/cvs/busybox/networking/ping.c,v
> retrieving revision 1.55
> diff -u -r1.55 ping.c
> --- networking/ping.c	22 Jul 2003 08:56:51 -0000	1.55
> +++ networking/ping.c	30 Jan 2004 12:53:35 -0000
> @@ -51,6 +51,83 @@
>  #include <stdlib.h>
>  #include "busybox.h"
>  
> +#ifdef __dietlibc__
> +/* Dietlibc is missing the BSD definition 'struct icmp' */
> +/*
> + * Internal of an ICMP Router Advertisement

... <code omitted> ...

Surely it would be better to submit a patch to upstream dietlibc
for this instead, rather than patching every program using struct icmp.

> Index: shell/ash.c
> ===================================================================
> RCS file: /var/cvs/busybox/shell/ash.c,v
> retrieving revision 1.87
> diff -u -r1.87 ash.c
> --- shell/ash.c	25 Jan 2004 08:46:10 -0000	1.87
> +++ shell/ash.c	30 Jan 2004 12:53:37 -0000
> +#endif

...

>  #include <sys/types.h>
>  #include <sys/cdefs.h>
> @@ -2531,9 +2533,17 @@
>  static void
>  onint(void) {
>  	int i;
> +#ifdef __dietlibc__
> +	sigset_t signals;
> +#endif
>  
>  	intpending = 0;
> +#ifndef __dietlibc__
>  	sigsetmask(0);
> +#else
> +	sigemptyset(&signals);
> +	sigprocmask(SIG_SETMASK,&signals,NULL);
> +#endif

A configuration flag for bsd signal functions would be more general,
and this really isn't dietlibc-specific.

>  	i = EXSIG;
>  	if (gotsig[SIGINT - 1] && !trap[SIGINT]) {
>  		if (!(rootshell && iflag)) {
> @@ -3263,16 +3273,14 @@
>  		const char *p = " %s";
>  
>  		p++;
> -		dprintf(preverrout_fd, p, ps4val());
> +		xwrite(preverrout_fd,ps4val(),strlen(ps4val()));
>  
>  		sp = varlist.list;
>  		for(n = 0; n < 2; n++) {
>  			while (sp) {
> -				dprintf(preverrout_fd, p, sp->text);
> +				xwrite(preverrout_fd, sp->text,strlen(sp->text));
> +				xwrite(preverrout_fd, " ",1);
>  				sp = sp->next;
> -				if(*p == '%') {
> -					p--;
> -				}
>  			}
>  			sp = arglist.list;
>  		}
> @@ -5817,7 +5825,12 @@
>  		}
>  		q = r;
>  		if (len > 0) {
> +#ifdef _GNU_SOURCE
>  			q = mempcpy(q, str, len);
> +#else
> +			memcpy(q, str, len);
> +			q += len;
> +#endif

Again, rather than hacking various bits of busybox code, it would be
better to add a support file in libbb which provided implementations
of various convenience functions not available in some libcs.

> +#ifdef _GNU_SOURCE		
>  		col = fmtstr(s, 32, strsignal(st));
> +#else
> +		if (st < NSIG && sys_siglist[st]) {
> +			col += fmtstr(s, 32, sys_siglist[st]);
> +		} else {
> +			col += fmtstr(s, 64, "Signal %d", st );
> +		}
> +#endif

Using sys_siglist and sys_errlist is evil (tm).  They are being phased
out of many libraries.  Compatibility functions in libbb would be better.

... <lots of strchrnul / mempcpy patch-lets omitted> ...

Again, a lot of unnecessary (an size-increasing) inlining of code that
would be better handled by providing conditional replacements in libbb.

Manuel