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

[Jim Meyering]

"Alfred M. Szmidt" <ams@kemisten.nu> wrote:
> [I'm keeping bug-hurd in the CC since maybe someone on that list has
> something to comment.]
>
> Ping.

Thanks for the patch and the prod :-)

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

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

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

...
> 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;
> +
> +    /* 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
> +
...
> @@ -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)


> +         {
> +           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));

> +           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?
Is there some symbolic constant you can use instead of the literal?

> +              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")

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

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