Re: PUPA emu patch

[Marco Gerards]

Jeroen Dekkers <address@hidden> writes:

> 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.

Right.

> > +  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.

Perhaps this can fail on the Hurd when a translator dies?

> > +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.

You are right about DEV and FILE, I will change that.  I've included
the zeros to silent gcc.  It produces a lot of warnings.  I hate it
when I get a lot of warnings because that makes difficult to see real
problems.

> 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.

Well, there is the same problem with getopt_long.  I prefer to use
argp, but I can change this.  What do you think Okuji?

Thanks,
Marco