Some micro comments on your code itself.
Then at the end about the features.
> +AC_CHECK_HEADER(hurd.h,
> + [AC_DEFINE(HAVE_GNU_HURD_EXTENSIONS, 1,
> + [Define to 1 if we have GNU/Hurd extentions.])])
> +
If this is all there is to the check, then just use #if HAVE_HURD_H.
> +@item --author
> +@opindex --author
> +@cindex hurd, author, printing
> +Display the author field of a file. (This is an GNU/Hurd extention)
Put two spaces after the end of a sentence in a texinfo file.
Typo/spelling error: "extension"
Use complete sentences, i.e. a period after "extension."
(The same errors are repeated in the other documentation addition.)
> while ((c = getopt_long (argc, argv,
> - "abcdfghiklmnopqrstuvw:xABCDFGHI:LNQRST:UX1",
> + "abcd"
> +#if HAVE_GNU_HURD_EXTENSIONS
> + "e"
> +#endif
> + "fghiklmnopqrstuvw:xABCDFGHI:LNQRST:UX1",
> long_options, NULL)) != -1)
String concatenation is not portable to (very) old compilers. I don't know
whether fileutils tries to be highly portable, but if so then you can't use
string concatenation.
> +#if HAVE_GNU_HURD_EXTENSIONS
> + if ((files[files_index].stat.st_mode & S_IROOT)
> + && print_gnu_hurd_extensions)
> + {
> + file_t trans_port;
> + int trans_fd;
> + struct stat trans_stat;
> +
> + /* Get the underlying node */
> + trans_port = file_name_lookup (path, O_NOTRANS, 0);
> + if (trans_port == MACH_PORT_NULL)
> + {
> + error (0, errno, "%s", quotearg_colon (path));
> + exit_status = 1;
> + return 0;
> + }
> +
> + trans_fd = open (path, O_NOTRANS);
> + if (trans_fd == -1)
> + {
> + error (0, errno, "%s", quotearg_colon (path));
> + exit_status = 1;
> + return 0;
> + }
You leak the port here in the error cases. You should put the
file_name_lookup after the open, so you never do it in the error case.
But actually, you should not have both at all. Those two calls
(file_name_lookup and open) are doing the very same thing.
Just use getdport on the fd.
> + /* Get the top node. */
> + trans_fd = open (path, O_NORW);
> + if (trans_fd == -1)
> + {
> + error (0, errno, "%s", quotearg_colon (path));
> + exit_status = 1;
> + return 0;
> + }
Here you leak two fds from the two opens and the port from file_name_lookup.
> + fstat (trans_fd, &trans_stat);
> + files[files_index].trans_fsid = trans_stat.st_fsid;
Both the fstat calls and the file_get_translator call should check for
error. But there is no reason the second one should use open and fstat
at all. It can just call stat. But ls already did. I don't see that
you should need to do the second stat at all--the original stat done
without O_NOTRANS filled in st_fsid just as well as a second call would.
> + else
> + {
> +#endif
> + modebuf[10] = (FILE_HAS_ACL (f) ? '+' : ' ');
> + modebuf[11] = '\0';
> +#if HAVE_GNU_HURD_EXTENSIONS
> + }
> +#endif
Never have unbalanced braces in an #if if you can avoid it. In a case
like this, all you have to do is put the #endif between the else and the
open brace.
Now as to the features. I tend to think the switches should be added
unconditionally, and just marked as either no-op or an error at run
time. A switch like "append translator info on the end" can just be a
no-op on systems without translators. All the switches having to do
with the nobody bits can just always act as if all the bits are clear
when on a system where, in fact, they always are.
As to what switches we want, I don't think a single "Hurd extensions"
switch is right at all. There should be function-specific switches
about nobody bits and about translators.
First, a simple switch to print the nobody bits. It should print
something other than "---" or showing the other bits when S_IUSEUNK is
clear, e.g. "..." or "***" or something. That way it's never ambiguous
whether the nobody bits happen to be set the same as the other bits or
whether S_IUSEUNK is not set.
We might also want to have a switch that does something like print the
nobody bits iff they are set on any file in the listing. That behavior
could conceivably be the default in the absence of POSIXLY_CORRECT.
As to translators, I am not sure what all options we want. Certainly we
need an option to always use O_NOTRANS, analogous to -d. Under that
option, ls would use fd=open(,O_NORW|O_NOTRANS);fstat(fd) instead of
plain lstat. I'm not sure exactly which other options we need.
_______________________________________________
Bug-hurd mailing list
[EMAIL PROTECTED]
http://mail.gnu.org/mailman/listinfo/bug-hurd
