Re: PUPA emu patch

[Jeroen Dekkers]

On Sun, Nov 16, 2003 at 07:38:34PM +0100, Marco Gerards wrote:
>  /* This must define pupa_jmp_buf.  */
>  #include <pupa/cpu/setjmp.h>
>  
> +#ifdef PUPA_UTIL
> +#include <setjmp.h>
> +#define pupa_setjmp setjmp
> +#define pupa_longjmp longjmp
> +#else
>  int pupa_setjmp (pupa_jmp_buf env);
>  void pupa_longjmp (pupa_jmp_buf env, int val) __attribute__ ((noreturn));
> +#endif
>  
>  #endif /* ! PUPA_SETJMP_HEADER */

Instead of including pupa/cpu/setjmp.h you should typedef pupa_jmp_buf
to jmp_buf.

> +  if (stat (".", &prev_st) < 0)
> +    pupa_util_error ("Cannot stat `%s'", dir);
> +
> +  if (! S_ISDIR (prev_st.st_mode))
> +    pupa_util_error ("`%s' is not a directory", dir);
> +
> +  while (1)
> +    {
> +      if (chdir ("..") < 0)
> +     pupa_util_error ("Cannot change directory to the parent");
> +
> +      if (stat (".", &st) < 0)
> +     pupa_util_error ("Cannot stat current directory");
> +
> +      if (! S_ISDIR (st.st_mode))
> +     pupa_util_error ("Current directory is not a directory???");

Why do we check if the current directory is a directory?  Looks
redundant to me because if it isn't a directory chdir() will fail.

> +static struct argp_option options[] = {
> +  {"root-device", 'r', "FILE", 0, "use DEV as the root device", 0},
> +  {"device-map",  'm', "FILE", 0, "use DEV as the root device 
> [default=guessed]", 0},
> +  {"directory",   'd', "DIR",  0, "use PUPA files in the directory DIR", 0},
> +  {"verbose",     'v', 0     , 0, "print verbose messages", 0},
> +  { 0, 0, 0, 0, 0, 0 }
> +};

Either FILE should be DEV or DEV should be FILE but they need to be
the same.  You don't have to write all those trailing zero's.  The
last fields are filled with zero automatically by C.  So you could
write the last line as { 0 } for example.

I also wonder whether we really have to use argp for 4 options.  It's
pretty big and if we want to use it on a non-GNU platform (e.g. BSD)
we need to put it in the source.

Jeroen Dekkers