Re: [address@hidden: ls --translators (coreutils)]

[Alfred M. Szmidt]

   Thanks for the patch and the prod :-)

Atleast you wake up when being proded. :-)

   Please mention that this option is Hurd-specific
   in both --help output and in coreutils.texi.

In `--help' too?  I don't think its useful to document it in --help,
since well, its just a "reminder" of what the option does.  The info
pages should serve as the place for documentation of what the actual
option does, if it is specific to a system etc.

   > There is two small problems, the first one is argz.h.  Adding a
   > special check for it in configure.ac and then doing an ifdef
   > seems abit over kill for one function call.  Right now it just
   > checks for hurd.h, and

   Can you find a better way to do this?  Maybe something relating to
   support for translators?  Like the existence of this function:
   file_get_translator.

Hmm... Yeah, I guess thats ok.  Should it set HAVE_TRANSLATORS or
HAVE_FILE_GET_TRANSLATORS when checking for file_get_translator in
configure.ac?  I prefer the first one for obvious esthetic reasons.

   > assumes that argz.h exists (which will work since all GNU/Hurd
   > systems run glibc).  If the current solution is not liked then
   > I'll change it and send another patch (or the person commiting
   > the change can do it).
   >
   > The second problem is that I haven't tested if this compiles on
   > GNU/Linux, could someone do that for me and report back?

   Please document the following

   > --translator on GNU/Linux should be a no-op.

How about this:

 @itemx --translator
 @opindex --translator
 @cindex translator, hurd

 List each files passive or active translator and its respective fsid
 (also called the file system id) when producing long format listings
 to the right of the file name. On systems that do not support
 translators (eg. GNU/Linux) this will do nothing, currently this is
 only supported on GNU/Hurd.

   ...
   > 2003-08-16  Alfred M. Szmidt  <ams@kemisten.nu>
   >
   >    Add support for new ls option, --translator, for GNU/Hurd.
   >
   >    * doc/coreutils.texi (ls invocation), NEWS: Document this.
   >    * configure.ac: Check for <hurd.h>.
   >    * src/ls.c [HAVE_HURD_H]: Include <hurd.h> and <argz.h>.
   >    (struct fileinfo) [HAVE_HURD_H]: New members trans_name,
   >    trans_fsid and trans_mode.
   >    (print_translator): New variable.
   >    (TRANSLATOR_OPTION): New enum value.
   >    (long_options, decode_switches, gobble_file)
   >    (print_long_format, usage): Support --translator.
   ...
   > diff -upr /src-cvs/coreutils/src/ls.c /home/ams/src/coreutils/src/ls.c
   > - --- /src-cvs/coreutils/src/ls.c  2003-07-27 13:41:33.000000000 +0200
   > +++ /home/ams/src/coreutils/src/ls.c       2003-08-16 21:47:20.000000000 
+0200
   > @@ -52,6 +52,11 @@
   >  # include <sys/ptem.h>
   >  #endif
   >
   > +#if HAVE_HURD_H
   > +# include <hurd.h>
   > +# include <argz.h>
   > +#endif
   > +
   >  #include <stdio.h>
   >  #include <assert.h>
   >  #include <setjmp.h>
   > @@ -204,6 +209,18 @@ struct fileinfo
   >      /* For symbolic link, name of the file linked to, otherwise zero. */
   >      char *linkname;
   >
   > +#if HAVE_HURD_H
   > +    /* The translator that is attached to the node.  */

   Is this member guaranteed to be set before used?  Could be.  I
   haven't actually applied the patch, and as such haven't checked
   carefully enough.

   > +    char *trans_name;

Nope, it now gets set to NULL before getting the underlying node of a
translator.

   > +
   > +    /* The fsid for the active translator.  */
   > +    int trans_fsid;
   > +
   > +    /* If 1 then we have a translator attached and/or running on the node,
   > +       otherwise 0.  */

   This member is set but not used.

   > +    int trans_mode;
   > +#endif
   > +

Thanks for noticing this! I even forgot what that variable was
supposed to do.  It has now been squashed into oblivion.

   > @@ -2435,6 +2462,51 @@ gobble_file (const char *name, enum file
   >        free (linkpath);
   >    }
   >
   > +#if HAVE_HURD_H
   > +      if ((files[files_index].stat.st_mode & S_ITRANS) && 
print_translator)
   > +  {
   > +          int trans_fd;
   > +    file_t trans_port;
   > +          struct stat trans_stat;
   > +
   > +          files[files_index].trans_fsid = files[files_index].stat.st_fsid;
   > +
   > +          /* Get the underlying node */

   With the following, when the open fails (returning -1), the fstat
   will be called unnecessarily:

   > +    trans_fd = open (path, O_NOTRANS);
   > +    if ((trans_fd && fstat (trans_fd, &trans_stat)) < 0)

   How about this instead:
     if ((0 <= trans_fd && fstat (trans_fd, &trans_stat)) < 0)

Done.

   > +      {
   > +        error (0, errno, "%s", quotearg_colon (path));

   Please use a string saying what has failed, rather than just
   `"%s"'.  E.g.,
             error (0, errno, _("failed to get attributes of %s"), quote 
(path));

Done.

   > +        close (trans_fd);
   > +        exit_status = 1;
   > +        return 0;
   > +      }
   > +
   > +    trans_port = getdport (trans_fd);
   > +    close (trans_fd);
   > +
   > +          if (trans_stat.st_mode & S_IPTRANS)
   > +            {
   > +              char buf[1024], *trans = buf;

   Is this 1024-byte buffer guaranteed to be large enough?

Good one.  I poked around in the Hurd source code, and couldn't really
figure out if it would be guaranteed (some places use a 2048 byte
buffer).  I think that this should be less than the block size of the
current file-system (according to [hurd]/ext2fs/inode.c and
[hurd]/ufs/inode.c).  Could some Hurd god comment on this and what
should/could be done?

   Is there some symbolic constant you can use instead of the literal?

Doubt it.

   > +              int trans_len = sizeof (buf);
   > +
   > +        if (file_get_translator (trans_port, &trans, &trans_len))
   > +                {
   > +            mach_port_deallocate (mach_task_self(), trans_port);
   > +                  error (0, errno, "%s", quotearg_colon (path));

   Same as above.  e.g., _("failed to get translator for %s")

Done.

   > +                  exit_status = 1;
   > +                  return 0;
   > +                }
   > +
   > +              argz_stringify (trans, trans_len, ' ');
   > +
   > +              files[files_index].trans_name = strdup(trans);

   What if strdup fails?  Either handle it or use xstrdup.

Changed to use xstrdup().

   > +              files[files_index].trans_mode = 1;
   > +
   > +        mach_port_deallocate (mach_task_self(), trans_port);
   ...

Since you brought up a couple of internationalisation issues it
occured to me that the string "unknown" isn't translated.  I changed
the relevant code in print_long_format() to use `printf
(_("unknown"));" instead of `printf ("unknown");'

A new patch will be posted when the above nitpicks are resolved.  It
contains some minor fixes that I noticed while re-reading the code.

Thanks for the comments!