bug#59535: suggestion for: info ls

2022-11-24 Thread Pádraig Brady

On 23/11/2022 19:51, Günter Essers wrote:

Hi,

though here

https://www.gnu.org/software/coreutils/manual/html_node/ls-invocation.html

I had read "Later options override earlier options that are
incompatible", at first I was a bit surprised about the different output
for 'ls -lC' and 'ls -Cl'. (I contacted your website documenting GNU
coreutils 9.1.)

u@host$ mkdir dummy
u@host$ cd dummy/
u@host$ touch file1
u@host$ ls -lC
file1
u@host$ ls -Cl
insgesamt 0
-rw-r--r-- 1 u u 0 23. Nov 20:31 file1

Would a remark in the manual be an exaggeration and too fussy?

My working environment: GNU coreutils 8.32, Debian 11.5.

I hope not having wasted your time. Regards 


It's a good suggestion, which has actually already been implemented:
https://github.com/coreutils/coreutils/commit/1625916a1

Note the web page is generated from the info pages of the latest release.
It's just your distro is using an old release at this stage.

Marking this as done.

thanks,
Pádraig





bug#59533: BUG: files starting with "-" makes stat to say "invalid option -- 'S'" #64

2022-11-24 Thread Pádraig Brady

tag 59533 notabug
close 59533
stop

On 23/11/2022 19:34, Elias Tsolis wrote:

files starting with "-" makes stat to say "invalid option -- 'S'"

example, create a file "-fddsfdf.txt" and run "stat *" or "stat
-fddsfdf.txt"

and then again , create a file "fddsfdf.txt" and run "stat *" or "stat
fddsfdf.txt"

In first case, it will says "invalid option -- 'S'",
in second, it will be run normally.


This is usually handled with:
  stat ./-blah
  stat -- -blah

thanks,
Pádraig





bug#59382: cp(1) tries to allocate too much memory if filesystem blocksizes are unusual

2022-11-20 Thread Pádraig Brady

On 20/11/2022 03:50, Paul Eggert wrote:

The block size for filesystems can also be quite large (currently, up
to 16M).


It seems ZFS tries to "help" apps by reporting misinformation (namely a
smaller block size than actually preferred) when the file is small. This
is unfortunate, since it messes up cp and similar programs that need to
juggle multiple block sizes. Plus, it messes up any program that assumes
st_blksize is constant for the life of a file descriptor, which "cp"
does assume elsewhere.

GNU cp doesn't need ZFS's "help", as it's already smart enough to not
over-allocate a buffer when the input file is small but its blocksize is
large. Instead, this "help" from ZFS causes GNU cp to over-allocate
because it naively trusts the blocksize ZFS that reports.



The proposed patch attached removes the use of buffer_lcm()
and just picks the largest st_blksize, which would be 4MiB in your case.
It also limits the max buffer size to 32MiB in the edge case
where st_blksize returns a larger value that this.


I suppose this could break cp if st_blksize is not a power of 2, and if
the file is not a regular file, and reads must be a multiple of the
block size. POSIX allows such things though I expect nowadays it'd be
limited to weird devices.

Although we inadvertently removed support for weird devices in 2009 by
commit 55efc5f3ee485b3e31a91c331f07c89aeccc4e89, and nobody seems to
care (because people use dd or whatever to deal with weird devices), I
think it'd be better to limit the fix to regular files. And while we're
at it we might as well resurrect support for weird devices.


I wouldn't worry about weird devices TBH, for the reasons you state,
but it doesn't cost too much to support so fair enough.
I would definitely add a comment to the code in this regard though
as I certainly wasn't aware of that issue when I added commit 55efc5f3.


+#include 


No need for this, as static_assert works without  in C23, and
Gnulib's assert-h module support this.


cool


+/* Set a max constraint to avoid excessive mem usage or type overflow.  */
+enum { IO_BUFSIZE_MAX = 128 * IO_BUFSIZE };
+static_assert (IO_BUFSIZE_MAX <= MIN (IDX_MAX, SIZE_MAX) / 2 + 1);


I'm leery of putting in a maximum as low as 16 MiB. Although that's OK
now (it matches OpenZFS's current maximum), cp in the future will surely
deal with bigger block sizes. Instead, how about if we stick with GNU's
"no arbitrary limits" policy and work around the ZFS bug instead?


Well I wouldn't think of this as a functional limit,
more of a defensive programming technique with possible perf benefits.
Note as per the table in ioblksize.h, performance is seen to max out
at around 2^17 on most systems, and degrades on some systems beyond that.
So cp would be faster while being less memory efficient.
But yes if we're relying on these multiples for "weird devices" then fair 
enough,
that would be a functional limit, so we need to consider such large buffers.

Also to state it explicitly, for regular files, your change to clamp to a power 
of two
will effectively get buffer_lcm to pick the max of the two io_blksize() values.

I would change the commit summary from:
  cp: work around ZFS misinformation
to:
  copy: fix possible over allocation for regular files
because:
  - This also applies to cross device mv etc.
  - ZFS is giving more info TBH, just a bit naïvely
  - I do see this is a coreutils bug (as well)

Here is a NEWS entry as well:

  cp, mv, and install avoid allocating too much memory, and possibly
  triggering "memory exhausted" failures, on file systems like ZFS,
  which can return varied file system I/O block size values for files.
  [bug introduced in coreutils-6.0]

thanks!
Pádraig





bug#59382: cp(1) tries to allocate too much memory if filesystem blocksizes are unusual

2022-11-19 Thread Pádraig Brady

On 19/11/2022 08:14, Korn Andras wrote:

Hi,

on zfs, newfstatat() can return an st_blksize that is approximately equal to 
the file size in bytes (if the file fit into a single zfs record).

The block size for filesystems can also be quite large (currently, up to 16M).

The code at https://github.com/coreutils/coreutils/blob/master/src/copy.c#L1343 will try 
to compute the least common multiple of the input and output block sizes, then allocate a 
buffer of that size using mmap(). With such unusual block sizes, the least common 
multiple can be very large, causing the mmap() to return ENOMEM and cp to abort with 
"memory exhausted".

Example:

openat(AT_FDCWD, "tmp", O_RDONLY|O_PATH|O_DIRECTORY) = 3
newfstatat(AT_FDCWD, "usr/src/zfs-2.1.6/configure", {st_dev=makedev(0, 0x30), 
st_ino=29267, st_mode=S_IFREG|0755, st_nlink=1, st_uid=0, st_gid=0, st_blksize=2647552, 
st_blocks=5177, st_size=2647046, st_atime=1668786119 /* 
2022-11-18T16:41:59.760703544+0100 */, st_atime_nsec=760703544, st_mtime=1667705386 /* 
2022-11-06T04:29:46+0100 */, st_mtime_nsec=0, st_ctime=1668785062 /* 
2022-11-18T16:24:22.866737598+0100 */, st_ctime_nsec=866737598}, AT_SYMLINK_NOFOLLOW) = 0
newfstatat(3, "configure", {st_dev=makedev(0, 0x32), st_ino=3838, 
st_mode=S_IFREG|0700, st_nlink=1, st_uid=0, st_gid=0, st_blksize=4194304, st_blocks=1, 
st_size=0, st_atime=1668788205 /* 2022-11-18T17:16:45.416328293+0100 */, 
st_atime_nsec=416328293, st_mtime=1668788255 /* 2022-11-18T17:17:35.809758585+0100 */, 
st_mtime_nsec=809758585, st_ctime=1668788255 /* 2022-11-18T17:17:35.809758585+0100 */, 
st_ctime_nsec=809758585}, 0) = 0
openat(AT_FDCWD, "usr/src/zfs-2.1.6/configure", O_RDONLY|O_NOFOLLOW) = 
4
newfstatat(4, "", {st_dev=makedev(0, 0x30), 
st_ino=29267, st_mode=S_IFREG|0755, st_nlink=1, st_uid=0, st_gid=0, st_blksize=2647552, 
st_blocks=5177, st_size=2647046, st_atime=1668786119 /* 2022-11-18T16:41:59.760703544+0100 */, 
st_atime_nsec=760703544, st_mtime=1667705386 /* 2022-11-06T04:29:46+0100 */, st_mtime_nsec=0, 
st_ctime=1668785062 /* 2022-11-18T16:24:22.866737598+0100 */, st_ctime_nsec=866737598}, 
AT_EMPTY_PATH) = 0
openat(3, "configure", O_WRONLY|O_TRUNC) = 5
ioctl(5, BTRFS_IOC_CLONE or FICLONE, 4) = -1 EXDEV (Invalid 
cross-device link)
newfstatat(5, "", {st_dev=makedev(0, 0x32), st_ino=3838, 
st_mode=S_IFREG|0700, st_nlink=1, st_uid=0, st_gid=0, st_blksize=4194304, st_blocks=1, 
st_size=0, st_atime=1668788205 /* 2022-11-18T17:16:45.416328293+0100 */, 
st_atime_nsec=416328293, st_mtime=1668788266 /* 2022-11-18T17:17:46.326056957+0100 */, 
st_mtime_nsec=326056957, st_ctime=1668788266 /* 2022-11-18T17:17:46.326056957+0100 */, 
st_ctime_nsec=326056957}, AT_EMPTY_PATH) = 0
fadvise64(4, 0, 0, POSIX_FADV_SEQUENTIAL) = 0
copy_file_range(4, NULL, 5, 
NULL, 9223372035781033984, 0) = -1 EXDEV (Invalid cross-device link)
mmap(NULL, 21688754176, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) 
= -1 ENOMEM (Cannot allocate memory)
brk(0x55a7fa4b) = 0x55a2ed8ab000
mmap(NULL, 21689794560, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) 
= -1 ENOMEM (Cannot allocate memory)
openat(AT_FDCWD, "/usr/share/locale/en_US.UTF-8/LC_MESSAGES/coreutils.mo", 
O_RDONLY) = -1 ENOENT (No such file or directory)
openat(AT_FDCWD, "/usr/share/locale/en_US.utf8/LC_MESSAGES/coreutils.mo", 
O_RDONLY) = -1 ENOENT (No such file or directory)
openat(AT_FDCWD, "/usr/share/locale/en_US/LC_MESSAGES/coreutils.mo", O_RDONLY) 
= -1 ENOENT (No such file or directory)
openat(AT_FDCWD, "/usr/share/locale/en.UTF-8/LC_MESSAGES/coreutils.mo", 
O_RDONLY) = -1 ENOENT (No such file or directory)
openat(AT_FDCWD, "/usr/share/locale/en.utf8/LC_MESSAGES/coreutils.mo", 
O_RDONLY) = -1 ENOENT (No such file or directory)
openat(AT_FDCWD, "/usr/share/locale/en/LC_MESSAGES/coreutils.mo", O_RDONLY) = 
-1 ENOENT (No such file or directory)
write(2>, "cp: ", 4) = 4
write(2>, "memory exhausted", 16) = 16
write(2>, "\n", 1) = 1
lseek(0>, 0, SEEK_CUR) = -1 ESPIPE (Illegal seek)
close(0>)= 0
close(1>)= 0
close(2>)= 0
exit_group(1)   = ?

I think cp(1) shouldn't insist on using the lcm if it's very large; 
additionally, it should never try to allocate a buffer that is (much) larger 
than the source file.

Perhaps you could also try to use successively smaller buffers if allocating a 
large one fails?


Thank you for the detailed report.
This buffer_lcm code has been present for a long time,
but I agree in the presence of disparate st_blksize values
it can result in inappropriate buffer sizes.

In your case:
  src st_blksize = 2647552 (0x286600) (st_size = 2647046)
  dst st_blksize = 4194304 (0x40) (st_size = 0)
Which implies a lowest common multiple of 21688745984

Once the st_blksize is based on st_size we can get very large values like this.
In the zfs case it seems to set st_blksize to st_size rounded to next multiple 
of 512.

The proposed patch attached removes the use of buffer_lcm()
and just picks 

bug#58476: buggy version sort

2022-10-13 Thread Pádraig Brady

On 12/10/2022 22:07, Vincent Lefevre wrote:

A regression in version sort (used as the natural sort for "ls")
has been introduced from coreutils 8.32 to 9.1:

With coreutils 8.32 (Debian 11):

$ printf "%s\n" a a0 a1 a.b a0.b a1.b | sort -V
a
a.b
a0
a0.b
a1
a1.b

With coreutils 9.1 (Debian/unstable):

$ printf "%s\n" a a0 a1 a.b a0.b a1.b | sort -V
a
a0
a0.b
a.b
a1
a1.b

This is now completely illogical.


This looks to be the same point as discussed in https://bugs.gnu.org/58153
where a trailing '0' is essentially ignored when sorting,
as per the debian version sorting spec.

This is surprising, and perhaps we should diverge from the spec
in this regard, but I'm not sure.

thanks,
Pádraig





bug#58153: hungry sort eats lines

2022-09-28 Thread Pádraig Brady

On 28/09/2022 22:13, DrSlony wrote:

Hey

printf '%s\n' "key;foo" "key0;bar0" | sort -Vu -t ';' --key=1,1

sort 8.32 outputs:
  key;bar
  key0;foo

sort 9.1 outputs:
  key;foo

"key0;foo" is missing.


You're using version sort and '0' is special to version sorting.
Specifically as per 
https://www.debian.org/doc/debian-policy/ch-controlfields.html#version
In particular this portion of the documented comparison algorithm:

"Then the initial part of the remainder of each string which consists entirely
 of digit characters is determined. The numerical values of these two parts are 
compared,
  and any difference found is returned as the result of the comparison.
  For these purposes an empty string (which can only occur at the end of one or 
both
  version strings being compared) counts as zero."

You can see this in the simplified example:

# Use --check to see if any matches that need to be dropped:
$ printf '%s\n' "key" "key0" | sort -u -C -V || echo equal
equal

# Here 1 is treated differently:
$ printf '%s\n' "key" "key1" | sort -u -c -V && echo different
different

I agree this is surprising, but version sorting has lots of edge cases,
so it's probably best to stick to the documented algorithm here.

thanks,
Pádraig





bug#58050: [INSTALLED] rm: fix diagnostics on I/O error

2022-09-25 Thread Pádraig Brady

On 25/09/2022 00:37, Paul Eggert wrote:

I ran into this problem when attempting to recursively
remove a directory in a filesystem on flaky hardware.
Although the underlying readdir syscall failed with errno == EIO,
rm issued no diagnostic about the I/O error.


This looks good.
How about the attached to add a NEWS entry,
and add DS_EMPTY, DS_NONEMPTY enums to make the code easier to read?

thanks,
Pádraig
diff --git a/NEWS b/NEWS
index d1c400e67..7f4eab921 100644
--- a/NEWS
+++ b/NEWS
@@ -60,6 +60,10 @@ GNU coreutils NEWS-*- outline -*-
   date --debug now diagnoses if multiple --date or --set options are
   specified, as only the last specified is significant in that case.
 
+  rm outputs more accurate diagnostics in the presence of errors
+  when removing directories.  For example EIO will be faithfully
+  diagnosed, rather than being conflated with ENOTEMPTY.
+
 
 * Noteworthy changes in release 9.1 (2022-04-15) [stable]
 
diff --git a/src/remove.c b/src/remove.c
index 0b6754bf7..a19794d5e 100644
--- a/src/remove.c
+++ b/src/remove.c
@@ -164,11 +164,11 @@ write_protected_non_symlink (int fd_cwd,
This is -1 if the directory is empty, 0 if it is nonempty,
and a positive error number if there was trouble determining the status,
e.g., it is not a directory, or permissions problems, or I/O errors.
-   Use *DIR_STATUS is a cache for the status.  */
+   Use *DIR_STATUS as a cache for the status.  */
 static int
 get_dir_status (FTS const *fts, FTSENT const *ent, int *dir_status)
 {
-  if (*dir_status < -1)
+  if (*dir_status == DS_UNKNOWN)
 *dir_status = directory_status (fts->fts_cwd_fd, ent->fts_accpath);
   return *dir_status;
 }
@@ -254,7 +254,7 @@ prompt (FTS const *fts, FTSENT const *ent, bool is_dir,
 prompting the user  */
 if ( ! (x->recursive
 || (x->remove_empty_directories
-&& get_dir_status (fts, ent, dir_status) < 0)))
+&& get_dir_status (fts, ent, dir_status) == DS_EMPTY)))
   {
 write_protected = -1;
 wp_errno = *dir_status <= 0 ? EISDIR : *dir_status;
@@ -273,7 +273,7 @@ prompt (FTS const *fts, FTSENT const *ent, bool is_dir,
   /* Issue the prompt.  */
   if (dirent_type == DT_DIR
   && mode == PA_DESCEND_INTO_DIR
-  && get_dir_status (fts, ent, dir_status) == 0)
+  && get_dir_status (fts, ent, dir_status) == DS_NONEMPTY)
 fprintf (stderr,
  (write_protected
   ? _("%s: descend into write-protected directory %s? ")
@@ -427,14 +427,14 @@ excise (FTS *fts, FTSENT *ent, struct rm_options const *x, bool is_dir)
 static enum RM_status
 rm_fts (FTS *fts, FTSENT *ent, struct rm_options const *x)
 {
-  int dir_status = -2;
+  int dir_status = DS_UNKNOWN;
 
   switch (ent->fts_info)
 {
 case FTS_D:			/* preorder directory */
   if (! x->recursive
   && !(x->remove_empty_directories
-   && get_dir_status (fts, ent, _status) < 0))
+   && get_dir_status (fts, ent, _status) == DS_EMPTY))
 {
   /* This is the first (pre-order) encounter with a directory
  that we cannot delete.
@@ -512,7 +512,7 @@ rm_fts (FTS *fts, FTSENT *ent, struct rm_options const *x)
 enum RM_status s = prompt (fts, ent, true /*is_dir*/, x,
PA_DESCEND_INTO_DIR, _status);
 
-if (s == RM_USER_ACCEPTED && dir_status == -1)
+if (s == RM_USER_ACCEPTED && dir_status == DS_EMPTY)
   {
 /* When we know (from prompt when in interactive mode)
that this is an empty directory, don't prompt twice.  */
diff --git a/src/system.h b/src/system.h
index 91228ec13..c63b66741 100644
--- a/src/system.h
+++ b/src/system.h
@@ -291,6 +291,11 @@ readdir_ignoring_dot_and_dotdot (DIR *dirp)
0 if DIR is a nonempty directory,
and a positive error number if there was trouble determining
whether DIR is an empty or nonempty directory.  */
+enum {
+DS_UNKNOWN = -2,
+DS_EMPTY = -1,
+DS_NONEMPTY = 0,
+};
 static inline int
 directory_status (int fd_cwd, char const *dir)
 {
@@ -316,7 +321,7 @@ directory_status (int fd_cwd, char const *dir)
   no_direntries = !readdir_ignoring_dot_and_dotdot (dirp);
   saved_errno = errno;
   closedir (dirp);
-  return no_direntries && saved_errno == 0 ? -1 : saved_errno;
+  return no_direntries && saved_errno == 0 ? DS_EMPTY : saved_errno;
 }
 
 /* Factor out some of the common --help and --version processing code.  */


bug#13028: inplace

2022-09-12 Thread Pádraig Brady

On 12/09/2022 09:07, Reuben Thomas wrote:

On Mon, 16 May 2016 at 15:42, Pádraig Brady mailto:p...@draigbrady.com>> wrote:


I just don't have the time at present to complete this.

I did implement ACID file replacement using POSIX APIs a while ago in:
https://github.com/pixelb/crudini <https://github.com/pixelb/crudini>
The commit messages there have details on fsync()ing requirements etc.

Implementation in C in coreutils could also use other APIs where available
like renameat2(..., RENAME_EXCHANGE) and exchangedata() etc.
I noticed XFS_IOC_SWAPEXT but that's currently hardcoded
to support only same sized replacements (for defragment apps).
I was thinking of providing a wrapper for exchangedata() in gnulib,
which could be leveraged where available or falling back to
the current POSIX APIs.


Did you get any further? I was prompted to look up this thread by a deprecation message for 
"tempfile" from Victor Porton's "inplace" script, which I still use!


Not yet unfortunately






bug#33123: [PATCH] ls: --color: honor separate sequences for extension cases

2022-09-04 Thread Pádraig Brady

On 10/05/2022 15:21, Pádraig Brady wrote:

On 10/05/2022 09:45, Mikael Magnusson wrote:

Hi,

In bug #9086 [1] there was a lot of good discussion about case
insensitive matching for dircolors. This was all then summarily
ignored in [2] which made the behaviour unconditional. This breaks
coloring of uppercase files that should have a different color from
lowercase files, for example .C which is a c++ source file, not a c
source file. And as mentioned in [1] it is also very useful to notice
incorrectly (eg uppercase extension files) quickly by them being
white, so you can fix them.

I'm not sure if the suggestion in this bug (33123) is good though;
since ls --color=auto-case will just error on older releases, you
can't easily sync your aliases. Unfortunately ls also ignores all of
LS_COLORS if it contains any unknown values (why??) so we can't add a
"CS" field there for case sensitiveness either (unless there's some
extra options field already I don't know about?).

If anyone has some ideas I would be happy to produce a patch (assuming
you don't need copyright assignment).

[1] https://debbugs.gnu.org/cgi/bugreport.cgi?bug=9086
[2] https://lists.gnu.org/archive/html/coreutils/2018-06/msg00011.html


Thanks for taking the time to tie all these threads together.

Yes we forgot about [1] when implementing [2].

The ideas in [1] are sound. I.e. we shouldn't need a new option.
We just need to honor any capitalized extension entries,
and fall back to case insensitive for lower case extensions.
We do need to be cognizant of performance though.
We could sort the list of extensions, which would auto give
precedence to capitalized entries, and we might also use that
sorting to break out of the match loop early.


The attached will honor different sequences defined for
separate extension letter cases, by operating case sensitively
for those extensions.

cheers,
Pádraig
From 2c1daa5dbe226852417b9721f799e0939d640ea0 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= 
Date: Sun, 4 Sep 2022 19:59:25 +0100
Subject: [PATCH] ls: --color: honor separate sequences for extension cases

Following on from commit v8.29-45-g24053fbd8 which unconditionally
used case insensitive extension matching, support selective
case sensitive matching when there are separate extension cases
defined with different display sequences.

* src/ls.c (parse_ls_color): Postprocess the list to
mark entries for case sensitive matching, or to
quickly ignore unmatchable entries.
* tests/ls/color-ext.sh: Add test cases.
* NEWS: Mention the change in behavior.
Adressess https://bugs.gnu.org/33123
---
 NEWS  |  3 ++
 src/ls.c  | 66 +++
 tests/ls/color-ext.sh | 46 +-
 3 files changed, 109 insertions(+), 6 deletions(-)

diff --git a/NEWS b/NEWS
index db5150824..496d7a4da 100644
--- a/NEWS
+++ b/NEWS
@@ -40,6 +40,9 @@ GNU coreutils NEWS-*- outline -*-
   reverting to the behavior in coreutils-9.0 and earlier.
   This behavior is now documented.
 
+  ls --color now matches a file extension case sensitively
+  if there are different sequences defined for separate cases.
+
   runcon now exits with status 125 for internal errors.  Previously upon
   internal errors it would exit with status 1, which was less distinguishable
   from errors from the invoked command.
diff --git a/src/ls.c b/src/ls.c
index 2960f6d38..5a83415c6 100644
--- a/src/ls.c
+++ b/src/ls.c
@@ -612,6 +612,7 @@ struct color_ext_type
   {
 struct bin_str ext;		/* The extension we're looking for */
 struct bin_str seq;		/* The sequence to output when we do */
+bool   exact_match;		/* Whether to compare case insensitively */
 struct color_ext_type *next;	/* Next in list */
   };
 
@@ -643,7 +644,7 @@ static struct bin_str color_indicator[] =
 { LEN_STR_PAIR ("\033[K") },	/* cl: clear to end of line */
   };
 
-/* FIXME: comment  */
+/* A list mapping file extensions to corresponding display sequence.  */
 static struct color_ext_type *color_ext_list = NULL;
 
 /* Buffer for color sequences */
@@ -2775,6 +2776,7 @@ parse_ls_color (void)
   ext = xmalloc (sizeof *ext);
   ext->next = color_ext_list;
   color_ext_list = ext;
+  ext->exact_match = false;
 
   ++p;
   ext->ext.string = buf;
@@ -2860,6 +2862,49 @@ parse_ls_color (void)
 }
   print_with_color = false;
 }
+  else
+{
+  /* Postprocess list to set EXACT_MATCH on entries where there are
+ different cased extensions with separate sequences defined.
+ Also set ext.len to SIZE_MAX on any entries that can't
+ match due to precedence, to avoid redundant string compares.  */
+  struct color_ext_type *e1;
+
+  for (e1 = color_ext_list; e1 != NULL; e1 = e1->next)
+{
+  struct color_ext_type *e2;
+   

bug#57237: b2sum does not support '-a' options found at https://www.blake2.net/

2022-08-16 Thread Pádraig Brady

On 16/08/2022 04:15, Robert E. Novak wrote:

as a result, the Gnu version does not support blake2s (for smaller
digests) and blakes2bp for higher performance on multicore systems.

Yes, I know about Blake3, but there are many reasons to support older
hash algorithms.

The real problem is that you have co-opted the b2sum binary so that the
testing required to find out if is system's b2sum application is the
open source b2sum from https://www.blake2.net/ or the Gnu coreutils.  I
am hopeful that a similar problem is not introduced with a coreutils
version of b3sum?  Will you implement the C language single thread
version (lower performance) or the parallel merkle tree rust implementation?

Since you introduce incompatible differences in the implementation, the
least that you could do is to rename the Gnu Coreutils b2sum to gnub2sum
so that applications that require different command line semantics do
NOT Have to go through machinations to find all installed versions of
b2sum on a system in order to select the correct invocation semantics.
Just my $0.02 worth, but I am trying to rationalize the world of
cryptographic hash algorithms.  I have two blogs that reference this on
linkedin ( * & ** ) so that you can understand part of the reason why
this will become more important over time.

I realize that Gnu has a long tradition of implementing the Gnu version
of commands and that in many cases the Gnu versions have become the "de
facto" standards.  However this does not happen if you don't support the
semantics of the commands that you are replacing.

I have been using Unix/Linux since 1974 (Arpanet node #6 at Urbana,
Illinois) and I would never have made the transition to Linux if there
were less semantic consistency over time.

If indeed you had implemented a superset of the Blake2 semantics, there
would be no cause for concern.

*
https://www.linkedin.com/pulse/canonical-cryptographic-hash-encoding-robert-e-novak/?trackingId=gjy%2FJwsjnJaviUN2ZYtuqw%3D%3D

This is a preliminary version and I hope to release a second version
after further development.

**
https://www.linkedin.com/pulse/thoughts-pragmatic-one-time-pads-encryption-robert-e-novak/?trackingId=3AUdLAGWSsDMYYuCINZuVA%3D%3D



Discussion on the initial GNU coreutils implementation,
including dropping of the -a interface was discussed at:

  https://lists.gnu.org/archive/html/coreutils/2016-10/msg7.html
  https://lists.gnu.org/archive/html/coreutils/2016-11/msg0.html

We broached keeping coreutils simpler using just blake2b with blake2 folks,
and there was general agreement.  So essentially there were design negotiations
in the threads above to present the interface appropriate to most users
in the GNU coreutils b2sum implementation,
which would become the standard variant available.

Which systems are installing the reference version?
I presume this is an infrequent issue?
I would think the onus on the above systems would be to install the reference
version under a different name.

Note the GNU coreutils digest tools were refactored recently
around a single `cksum -a ...` tool, and there will be no
new separate digest specific tools in future (like b3sum etc.).

thanks,
Pádraig





bug#57132: bug report

2022-08-11 Thread Pádraig Brady

tag 57132 notabug
close 57132
stop

On 11/08/2022 04:05, lingzhiyuan (袁凌志) wrote:

# tail -f log_error.log
tail: unrecognized file system type 0x794c7630 for ‘log_error.log’. please 
report this to bug-coreutils@gnu.org. reverting to polling

[cid:image001.jpg@01D8AD72.4C974A50]


This was addressed in coreutils release 8.25 (2016-01-20).
Please update to something newer than that.

thank you.





bug#55401: date man page

2022-07-24 Thread Pádraig Brady

On 14/05/2022 01:42, Pádraig Brady wrote:

On 13/05/2022 20:10, t0th wrote:

Man page of date command should make explicit that -d and -r options are
mutually exclusive.


Right.  More accurately, we might have a sentence to say that:
"All options to specify the date to display are mutually exclusive.
I.e.: --date, --file, --reference, --resolution".


I've pushed the attached 2 patches
to document the current situation at least.

cheers,
Padraig
From b09a23ec3f20003729b652ff1141f76de5e29e7f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= 
Date: Sun, 24 Jul 2022 20:49:29 +0100
Subject: [PATCH] doc: date: clarify which options are mutually exclusive

* src/date.c (usage): Specify that --date, --file, --reference,
and --resolution are mutually exclusive.
* doc/coreutils.texi (Options for date): Likewise.
Addresses https://bugs.gnu.org/55401
---
 doc/coreutils.texi | 4 
 src/date.c | 5 +
 2 files changed, 9 insertions(+)

diff --git a/doc/coreutils.texi b/doc/coreutils.texi
index e0c87d1ad..53257f7d9 100644
--- a/doc/coreutils.texi
+++ b/doc/coreutils.texi
@@ -16426,6 +16426,10 @@ relative to Universal Time rather than to the local time zone.
 The program accepts the following options.  Also see @ref{Common options}.
 Except for @option{-u}, these options are all GNU extensions to POSIX.
 
+All options that specify the date to display are mutually exclusive.
+I.e.: @option{--date}, @option{--file}, @option{--reference},
+@option{--resolution}.
+
 @table @samp
 
 @item -d @var{datestr}
diff --git a/src/date.c b/src/date.c
index 7f2ac801d..ff5c548c0 100644
--- a/src/date.c
+++ b/src/date.c
@@ -183,6 +183,11 @@ With -s, or with [MMDDhhmm[[CC]YY][.ss]], set the date and time.\n\
   fputs (VERSION_OPTION_DESCRIPTION, stdout);
   fputs (_("\
 \n\
+All options that specify the date to display are mutually exclusive.\n\
+I.e.: --date, --file, --reference, --resolution.\n\
+"), stdout);
+  fputs (_("\
+\n\
 FORMAT controls the output.  Interpreted sequences are:\n\
 \n\
   %%   a literal %\n\
-- 
2.26.2

From 854e0351216869b8e79391e08b156127d1508beb Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= 
Date: Sun, 24 Jul 2022 19:24:18 +0100
Subject: [PATCH] date: --debug: diagnose discarded -d or -s options

* src/date.c: (main): Track and diagnose whether any
-d or -s options are dropped, as users may think
multiple options are supported, given they can be relative.
* tests/misc/date-debug.sh: Add a test case.
* NEWS: Mention the improvement.
---
 NEWS |  5 +
 src/date.c   | 12 
 tests/misc/date-debug.sh | 10 ++
 3 files changed, 27 insertions(+)

diff --git a/NEWS b/NEWS
index b5b8990f8..3113d4236 100644
--- a/NEWS
+++ b/NEWS
@@ -36,6 +36,11 @@ GNU coreutils NEWS-*- outline -*-
   factor now accepts the --exponents (-h) option to print factors
   in the form p^e, rather than repeating the prime p, e times.
 
+** Improvements
+
+  date --debug now diagnoses if multiple --date or --set options are
+  specified, as only the last specified is significant in that case.
+
 
 * Noteworthy changes in release 9.1 (2022-04-15) [stable]
 
diff --git a/src/date.c b/src/date.c
index 9a282e2f5..7f2ac801d 100644
--- a/src/date.c
+++ b/src/date.c
@@ -403,6 +403,8 @@ main (int argc, char **argv)
   char *reference = NULL;
   struct stat refstats;
   bool ok;
+  bool discarded_datestr = false;
+  bool discarded_set_datestr = false;
 
   initialize_main (, );
   set_program_name (argv[0]);
@@ -420,6 +422,8 @@ main (int argc, char **argv)
   switch (optc)
 {
 case 'd':
+  if (datestr)
+discarded_datestr = true;
   datestr = optarg;
   break;
 case DEBUG_DATE_PARSING_OPTION:
@@ -469,6 +473,8 @@ main (int argc, char **argv)
   new_format = rfc_email_format;
   break;
 case 's':
+  if (set_datestr)
+discarded_set_datestr = true;
   set_datestr = optarg;
   set_date = true;
   break;
@@ -511,6 +517,12 @@ main (int argc, char **argv)
   usage (EXIT_FAILURE);
 }
 
+  if (discarded_datestr && (parse_datetime_flags & PARSE_DATETIME_DEBUG))
+error (0, 0, _("only using last of multiple -d options"));
+
+  if (discarded_set_datestr && (parse_datetime_flags & PARSE_DATETIME_DEBUG))
+error (0, 0, _("only using last of multiple -s options"));
+
   if (optind < argc)
 {
   if (optind + 1 < argc)
diff --git a/tests/misc/date-debug.sh b/tests/misc/date-debug.sh
index 0b5217611..5c38dee41 100755
--- a/tests/misc/date-debug.sh
+++ b/tests/misc/date-debug.sh
@@ -298,4 +298,14 @@ sed '1s/(Y-M-D) [0-9][0-9][0-9][0-9]-/(Y-M-D) -/' out9_t > out9 \
 compare exp9 out9 || fail=1
 
 
+# Diagnose discarded -d arguments
+echo 'date: only using last of multiple -d options' > exp

bug#56710: ls vs. stat display of st_size

2022-07-24 Thread Pádraig Brady

On 24/07/2022 17:18, Paul Eggert wrote:

On 7/24/22 01:48, Pádraig Brady wrote:


Well ls(1) was explicitly changed to assuming only positive,
citing POSIX (though I can't see it in POSIX myself):
https://github.com/coreutils/coreutils/commit/67ba4ac01


I vaguely recall being involved with that decades-old change. The POSIX
requirement is here:

https://pubs.opengroup.org/onlinepubs/9699919799/utilities/ls.html#tag_20_73_10

(look for "%u").


Right, that's fairly conclusive for ls.


Also ls(1) can sort by size, which gives a little more
credence to assuming positive only size.


I don't see why; negative sizes sort just as well as positive ones do.


Fair enough.


For these reasons I would keep ls(1) as is (assuming positive).

As for stat(1), it's now consistent with ls(1) which has some benefit.
It is lower level though, so in my mind it might be better
to output the raw value, especially since it's such an edge case.

So I'd leave ls(1) as is, and I'll leave it up to you
how to handle stat(1) given the above points.


Consistency is reasonably important here (as per the original bug
report), so if those are the choices let's leave things as-is.


Cool.

For reference stat(1) on FreeBSD takes the lower level approach,
outputting signed by default (I presume from looking at the man page),
and allowing the user to override that.  I.e. it defaults
to `stat -f %z` but the user can override to `stat -f %Uz`.
We don't have many letters left to play with but I suppose
we could default to unsigned (as we now are) and support %Is etc.
for signed integer quantities. I'm not suggesting we need this,
just thinking out loud.

cheers,
Pádraig





bug#56710: ls vs. stat display of st_size

2022-07-24 Thread Pádraig Brady

On 23/07/2022 21:07, Paul Eggert wrote:

On 7/23/22 05:17, Pádraig Brady wrote:


BTW I see we've code in cache_fstatat() that assumes
st_size can't have such large values, which contradicts a bit.


Good catch. I installed the first attached patch.


  > This is only a real consideration for virtual files I think
  > since off_t is signed, and so impractical for a real file system
  > to support files > OFF_T_MAX.

Yes, that sounds right.

You've convinced me that 'ls' should switch to the way 'stat' behaves
rather than vice versa; that's more useful anyway. How about the
attached second patch, which I haven't installed? (I was actually
inclined this way originally but got lazy.)


Well ls(1) was explicitly changed to assuming only positive,
citing POSIX (though I can't see it in POSIX myself):
https://github.com/coreutils/coreutils/commit/67ba4ac01

Also ls(1) can sort by size, which gives a little more
credence to assuming positive only size.

Also ls(1) is a bit higher level, more human facing than stat(1).

For these reasons I would keep ls(1) as is (assuming positive).

As for stat(1), it's now consistent with ls(1) which has some benefit.
It is lower level though, so in my mind it might be better
to output the raw value, especially since it's such an edge case.

So I'd leave ls(1) as is, and I'll leave it up to you
how to handle stat(1) given the above points.

cheers,
Pádraig





bug#56710: ls vs. stat display of st_size

2022-07-23 Thread Pádraig Brady

On 22/07/2022 21:52, Paul Eggert wrote:

Thanks for reporting that. I installed the attached.


Playing devil's advocate, this takes the stance that
st_size should always be treated as unsigned
(given that stat(1) is a lower level util than ls(1)).

This is only a real consideration for virtual files I think
since off_t is signed, and so impractical for a real file system
to support files > OFF_T_MAX.
In this case /proc/kcore is a virtual file, with the
size representing the VM size (guessing riscv64 in this case).
But other virtual files may set st_size = -1,
to represent an unknown file size, which with the change,
scripts using stat(1) can no longer rely on?
Perhaps the "-1" case could be specialized for this.

BTW I see we've code in cache_fstatat() that assumes
st_size can't have such large values, which contradicts a bit.

BTW assuming that st_size is unsigned, reminds me of this change where
we cast all st_size to unsigned, which also allowed us to enable -Wsign-compare:
https://lists.gnu.org/archive/html/bug-coreutils/2009-01/msg00050.html

cheers,
Pádraig





bug#56520: Security vulnerabilities at coreutils version for CentOS 7.9

2022-07-12 Thread Pádraig Brady

On 12/07/2022 13:43, Meirav Rath via GNU coreutils Bug Reports wrote:

Hello,

My name is Meirav Rath, I'm a software developer and security champion at 
Imperva.
As part of our effort to map security risks in our products I've been scanning our 
3rd party rpms for vulnerabilities. It looks like coreutils available rpm for CentOS 
7.9 (8.22) has the vulnerability 
CVE-2017-18018.

When can we expect an updated RPM of a more advanced version with fixes for 
this issues, aimed for CentOS7.9?


This was previously discussed at:
https://lists.gnu.org/archive/html/coreutils/2017-12/msg00045.html
With corresponding doc patch at:
https://git.sv.gnu.org/gitweb/?p=coreutils.git;a=commitdiff;h=bc2fd9796

cheers,
Pádraig





bug#56391: `cp --reflink=always` creates empty file on failure

2022-07-07 Thread Pádraig Brady

forcemerge 56391 56392
close 56391
stop

On 06/07/2022 20:48, Paul Eggert wrote:

On 7/6/22 06:17, Pádraig Brady wrote:

This will usually work, but there are cases where this may lose data,
as previously discussed at:

https://bugzilla.redhat.com/show_bug.cgi?id=921708
http://lists.gnu.org/archive/html/coreutils/2013-03/msg00056.html

I'm not sure cp can robustly clean up in this situation?


Thanks for pointing me to those old discussions. As I understand it, the
worry is that FICLONE will only partly succeed, causing the destination
file to contain some (but not all) the input data, and then if we remove
the output file we'll lose the newly-made partial clone. I don't know
whether FICLONE can do that, but it sounds like a reasonable worry.

If that understanding is correct, then the attached further patch should
suffice, so I boldly installed it.


Yes that's better thanks.
The code looks good, and should cater for any issues
within a single cp instance.

As for overlapping processes accessing a file, this may introduce
new edge cases for where a file will be deleted. But I don't see
a specific issue with that, given there are existing truncation
races etc. with overlapping access to a file.  I haven't thought
through all scenarios TBH.

marking this as done.

thanks!
Pádraig





bug#56391: `cp --reflink=always` creates empty file on failure

2022-07-06 Thread Pádraig Brady

On 05/07/2022 16:04, Paul Eggert wrote:

Thanks for reporting that. I installed the attached patch.


This will usually work, but there are cases where this may lose data,
as previously discussed at:

https://bugzilla.redhat.com/show_bug.cgi?id=921708
http://lists.gnu.org/archive/html/coreutils/2013-03/msg00056.html

I'm not sure cp can robustly clean up in this situation?

cheers,
Pádraig





bug#55748: df: wrong column is checked in condition in total-verify.sh

2022-06-01 Thread Pádraig Brady

tag 55748 notabug
close 55748
stop

On 01/06/2022 09:01, Daniel Hofstetter wrote:

Hi,

While looking at
https://github.com/coreutils/coreutils/blob/master/tests/df/total-verify.sh
I noticed the following lines and I think "$5" in the last line (line
38 in the source code) should be "$6" because there are six columns in
the df output and '-' is in the sixth column of the row starting with
"total".

# Recognize df output lines like these:
# /dev/sdc1 0 0 0 - /c
# tmpfs 1536000 12965 1523035 1% /tmp
# total 5285932 787409 4498523 15% -
/^(.*?) +(-?\d+|-) +(-?\d+|-) +(-?\d+|-) +(?:-|[0-9]+%) (.*)$/
or die "$0: invalid input line\n: $_";
if ($1 eq 'total' && $5 eq '-')


Notice the non capturing group, i.e. (?:)
This is because the percentage column is not needed
in the subsequent processing.
I.e. the following would work, but isn't needed.

cheers,
Pádraig

diff --git a/tests/df/total-verify.sh b/tests/df/total-verify.sh
index 87589d23d..d3f109d97 100755
--- a/tests/df/total-verify.sh
+++ b/tests/df/total-verify.sh
@@ -33,9 +33,9 @@ while (<>)
 # /dev/sdc1  0   0   0-  /c
 # tmpfs1536000   12965 15230351% /tmp
 # total5285932  787409 4498523   15% -
-/^(.*?) +(-?\d+|-) +(-?\d+|-) +(-?\d+|-) +(?:-|[0-9]+%) (.*)$/
+/^(.*?) +(-?\d+|-) +(-?\d+|-) +(-?\d+|-) +(-|[0-9]+%) (.*)$/
   or die "$0: invalid input line\n: $_";
-if ($1 eq 'total' && $5 eq '-')
+if ($1 eq 'total' && $6 eq '-')
   {
 $total == $2 or die "$total != $2";
 $used  == $3 or die "$used  != $3";






bug#55724: cp --reflink=always failing when --reflink=auto reflinks successfully on OpenZFS

2022-05-30 Thread Pádraig Brady

On 30/05/2022 10:13, Rich wrote:

Hi!

So, OpenZFS is adding reflink support Soon(tm), including across
filesystems on a pool, which is nice.

Unfortunately, Linux's VFS returns EXDEV for trying FICLONE or
FICLONERANGE (but not copy_file_range) cross-filesystem before you ever ask
the filesystem-specific code, so currently, the following strange behavior
occurs:

On coreutils 8.30 or 8.32, cp --reflink=always across filesystems will fail
with EXDEV and --reflink=auto will not reflink (because they're not trying
copy_file_range as a fallback).
On coreutils git, as of b3331d59e, cp --reflink=always across filesystems
will fail with EXDEV without ever getting out of Linux's VFS code, cp
--reflink=auto will reflink silently (since it falls back to
copy_file_range after getting EXDEV), cp --reflink=never will not reflink.

(On the same filesystem, in all of the above versions, cp --reflink=always
and =auto do the same thing and reflink correctly.)

I'm not sure what the "correct" behavior here should be, but at least =auto
working and =always failing seems like a surprising and incorrect outcome
to me, though it's not readily obvious to me how the code "should" flow
instead to avoid that - and since the failure cases happen before calling
into OpenZFS, I don't see any way it could be handled better there.

Happy to point people at the WIP code being used to demonstrate this if it
would be helpful, but this seems like it's only OpenZFS specific in that
nobody else has this functionality but would hit this case (because IIUC
btrfs avoids clone_file failing with EXDEV by pretending they're not
distinct filesystems, and there's not many other FSes where reflink across
filesystems would make sense).

Thanks for any insights,
- Rich


Thanks for the clear info.
Yes this is an awkward one, which I'm not sure cp can do anything about.
`cp --reflink=always` => ensure we can reflink or otherwise fail.
Really the kernel has to behave appropriately there
and not do the blanket assumption with EXDEV.
cp can't determine from copy_file_range() whether a reflink
was performed or not, so wouldn't be appropriate to use with --reflink=always.

cheers,
Pádraig





bug#55622: Bug in sort with keys and reverse, and version-sort and reverse

2022-05-25 Thread Pádraig Brady

On 25/05/2022 03:00, Larry Ploetz wrote:

I think I've found a bug in sort (git branch master). The --reverse flag
seems to be ignored when --keys are supplied.

 larryp-MBP:bin larry$ ./sort --version
 sort (GNU coreutils) 9.1.17-a351f
 Copyright (C) 2022 Free Software Foundation, Inc.
 License GPLv3+: GNU GPL version 3 or 
later.
 This is free software: you are free to change and redistribute it.
 There is NO WARRANTY, to the extent permitted by law.

 Written by Mike Haertel and Paul Eggert.
 larryp-MBP:bin larry$ ./sort -r <<< $'a\nb'
 b
 a
 larryp-MBP:bin larry$ ./sort -rk1,1 <<< $'a\nb'
 a
 b


Thanks for the report.
I pushed the following to address this
https://git.sv.gnu.org/gitweb/?p=coreutils.git;a=commitdiff;h=4e0167ea1

The existing tests are sufficient to catch this.

thanks!
Pádraig






bug#55401: date man page

2022-05-13 Thread Pádraig Brady

On 13/05/2022 20:10, t0th wrote:

Man page of date command should make explicit that -d and -r options are
mutually exclusive.


Right.  More accurately, we might have a sentence to say that:
"All options to specify the date to display are mutually exclusive.
I.e.: --date, --file, --reference, --resolution".
However...


date -d -3 minutes -r tmp.txt "+%Y%m%d_%H%M"
date: the options to specify dates for printing are mutually exclusive


As you've seen, one might expect to be able to combine, as -d can be relative.
So theoretically we could support this (with the attached),
to honor the relative adjustment, but give precedence to a non relative date.

  $ src/date -r src/ls.c -d '-3 minutes'
  Fri 15 Apr 2022 16:30:53 IST
  $ src/date -r src/ls.c -d '1/1/2022'
  Sat 01 Jan 2022 00:00:00 GMT

In fact touch(1) behaves like this, which suggests date(1) should also.
From the info docs for the touch --reference option:

 "--reference
  Use the times of the reference FILE instead of the current time.
  If this option is combined with the --date=TIME
  (-d TIME) option, the reference FILES's time is
  the origin for any relative TIMEs given, but is otherwise ignored."

BTW, one might also expect that multiple -d options might combine like this,
however currently we silently ignore multiple -d (or -s) options.
The attached also at least warns about this with --debug:

  $ date --debug -d '15/4/2022' -d '-3 minutes'
  date: discarding previous -d: ‘15/4/2022’
  date: parsed relative part: -3 minutes
  ...

cheers,
Pádraigdiff --git a/doc/coreutils.texi b/doc/coreutils.texi
index 7bca37b71..ae6ec4def 100644
--- a/doc/coreutils.texi
+++ b/doc/coreutils.texi
@@ -16058,7 +16058,8 @@ is not set.  @xref{TZ Variable,, Specifying the Time Zone with
 @cindex formatting times
 If given an argument that starts with a @samp{+}, @command{date} prints the
 current date and time (or the date and time specified by the
-@option{--date} option, see below) in the format defined by that argument,
+@option{--date}, @option{--file}, or @option{--reference} options, see below)
+in the format defined by that argument,
 which is similar to that of the @code{strftime} function.  Except for
 conversion specifiers, which start with @samp{%}, characters in the
 format string are printed unchanged.  The conversion specifiers are
@@ -16502,6 +16503,9 @@ for the @option{--date} (@option{-d}) and @option{--file}
 @opindex --reference
 Display the date and time of the last modification of @var{file},
 instead of the current date and time.
+If this option is combined with the @option{--date} or @option{--file}
+options, the reference @var{file}'s time is
+the origin for any relative @var{time}s given, but is otherwise ignored.
 
 @item --resolution
 @opindex --resolution
diff --git a/src/date.c b/src/date.c
index 9a282e2f5..de94136fe 100644
--- a/src/date.c
+++ b/src/date.c
@@ -420,6 +420,8 @@ main (int argc, char **argv)
   switch (optc)
 {
 case 'd':
+  if (datestr && (parse_datetime_flags & PARSE_DATETIME_DEBUG))
+error (0, 0, _("discarding previous -d: %s"), quote (datestr));
   datestr = optarg;
   break;
 case DEBUG_DATE_PARSING_OPTION:
@@ -469,6 +471,8 @@ main (int argc, char **argv)
   new_format = rfc_email_format;
   break;
 case 's':
+  if (set_datestr && (parse_datetime_flags & PARSE_DATETIME_DEBUG))
+error (0, 0, _("discarding previous -s: %s"), quote (set_datestr));
   set_datestr = optarg;
   set_date = true;
   break;
@@ -494,8 +498,7 @@ main (int argc, char **argv)
 }
 }
 
-  int option_specified_date = (!!datestr + !!batch_file + !!reference
-   + get_resolution);
+  int option_specified_date = (!!datestr + !!batch_file + get_resolution);
 
   if (option_specified_date > 1)
 {
@@ -511,6 +514,9 @@ main (int argc, char **argv)
   usage (EXIT_FAILURE);
 }
 
+  /* Use as base for other "relative" specified dates.  */
+  option_specified_date += !!reference;
+
   if (optind < argc)
 {
   if (optind + 1 < argc)
@@ -597,7 +603,8 @@ main (int argc, char **argv)
 die (EXIT_FAILURE, errno, "%s", quotef (reference));
   when = get_stat_mtime ();
 }
-  else if (get_resolution)
+
+  if (get_resolution)
 {
   long int res = gettime_res ();
   when.tv_sec = res / TIMESPEC_HZ;
@@ -605,9 +612,10 @@ main (int argc, char **argv)
 }
   else
 {
+  struct timespec *now = reference ?  : NULL;
   if (set_datestr)
 datestr = set_datestr;
-  valid_date = parse_datetime2 (, datestr, NULL,
+  valid_date = parse_datetime2 (, datestr, now,
 parse_datetime_flags,
 tz, tzstring);
 }


bug#33123: ls: add case-sensitive --color option

2022-05-10 Thread Pádraig Brady

On 10/05/2022 09:45, Mikael Magnusson wrote:

Hi,

In bug #9086 [1] there was a lot of good discussion about case
insensitive matching for dircolors. This was all then summarily
ignored in [2] which made the behaviour unconditional. This breaks
coloring of uppercase files that should have a different color from
lowercase files, for example .C which is a c++ source file, not a c
source file. And as mentioned in [1] it is also very useful to notice
incorrectly (eg uppercase extension files) quickly by them being
white, so you can fix them.

I'm not sure if the suggestion in this bug (33123) is good though;
since ls --color=auto-case will just error on older releases, you
can't easily sync your aliases. Unfortunately ls also ignores all of
LS_COLORS if it contains any unknown values (why??) so we can't add a
"CS" field there for case sensitiveness either (unless there's some
extra options field already I don't know about?).

If anyone has some ideas I would be happy to produce a patch (assuming
you don't need copyright assignment).

[1] https://debbugs.gnu.org/cgi/bugreport.cgi?bug=9086
[2] https://lists.gnu.org/archive/html/coreutils/2018-06/msg00011.html


Thanks for taking the time to tie all these threads together.

Yes we forgot about [1] when implementing [2].

The ideas in [1] are sound. I.e. we shouldn't need a new option.
We just need to honor any capitalized extension entries,
and fall back to case insensitive for lower case extensions.
We do need to be cognizant of performance though.
We could sort the list of extensions, which would auto give
precedence to capitalized entries, and we might also use that
sorting to break out of the match loop early.

I'll look at this soon.

cheers,
Pádraig






bug#25999: SHA1SUM: please switch to sha1dc to warn of attempted hash collision attacks

2022-05-09 Thread Pádraig Brady

On 08/05/2022 21:19, Christoph Anton Mitterer wrote:

Hey.

Is this still being considered?


Yes it still does make sense,
especially as sha1 becomes less popular over time,
in which case the extra processing would matter less.

thanks,
Pádraig






bug#55325: (sha|md5)*sum on UNIX should default to binary mode

2022-05-09 Thread Pádraig Brady

tag 55325 wontfix
close 55325
stop

On 08/05/2022 21:18, Christoph Anton Mitterer wrote:

Hey.

Since on UNIX/Linux it is effectively always binary mode, shouldn't the
(sha|md5)*sum tools default to that, and especially also generate
output with the "*" flag set?


Well they do default to binary mode,
but don't use an explicit flag on UNIX systems
(unless -b is specified).

For compatibility reasons we can't really change thing now,
but note the newer `cksum -a ...` interface which uses
a different format without binary tag.

thanks,
Pádraig






bug#55304: build: INSTALL file is missing from .tar.gz snapshots

2022-05-07 Thread Pádraig Brady

tag 55304 notabug
close 55304
stop

On 07/05/2022 20:10, dg1727 via GNU coreutils Bug Reports wrote:

In the following snapshot:

http://git.savannah.gnu.org/cgit/coreutils.git/snapshot/coreutils-9.1.tar.gz

... the file README says the following a few paragraphs from the top:


If this file came to you as part of a tar archive, then see the file INSTALL
for compilation and installation instructions.


The file INSTALL doesn't seem to be in the tar archive.

Can this file be added to the snapshot, to assist users compiling the
coreutils?

I'm having an issue attempting to compile; if the INSTALL file helps me with
this issue, then I won't need to ask on the coreut...@gnu.org mailing list.

Many thanks in advance.


snapshots of a git repo are generally treated differently from a "dist" 
tarballs.
Specifically to build from a git snapshot, you would be acting like a developer,
and so would follow README-hacking instructions.

Note INSTALL _is_ present in the dist tarball, but not in the snapshot,
as that file comes from the gnulib submodule, and following README-hacking
instructions should populate all gnulib files.

If you only want to build, then it's best to go from the distribution tarballs
available at https://ftp.gnu.org/pub/gnu/coreutils/

cheers,
Pádraig





bug#55261: file:// links geneated by --hyperlink

2022-05-05 Thread Pádraig Brady

On 04/05/2022 14:27, Stephen Eglen wrote:

hi,

With ls (GNU coreutils) 9.0 if I generate a hyperlink e.g.:

$ ls --hyperlink /etc/anacrontab

it generates the following filename (after removing the markup)

file://light/etc/anacrontab

where 'light' is the name of my laptop (running arch linux).
browse-url-xdg-open is my browser-function, and

$ xdg-open file://light/etc/anacrontab

generates the error:

xdg-open: file 'file://light/etc/anacrontab' does not exist

As the file URI is local, I think it might be a mistake to include the
machine name, light, unless it is a FQDN.  If I edit the URL to remove
'light' the link works as expected.

This was reported for kitty terminal last year and fixed by dropping the
hostname: https://github.com/kovidgoyal/kitty/issues/2970

However, kitty seems to be able to connect to machines remotely via
ssh  ( https://download.calibre-ebook.com/videos/kitty.mp4 ) and so
maybe having the domain name in the hyperlink is useful.

Stephen


The scheme we're following for this is described at:
https://gist.github.com/egmontkob/eb114294efbcd5adb1944c9f3cb5feda#file-uris-and-the-hostname

What is $HOSTNAME for you?
Is it perhaps a FQDN and that might work better?
I guess if we went with $HOSTNAME if set, it would allow one to override
the hostname used by ls like: HOSTNAME=blah ls --hyper

For reference, on my system `xdg-open file://blah/etc/hostname`
ignores "blah" and opens the local file, while gnome-terminal
opening such a link will refuse with:
"file" scheme with remote hostname not supported.

cheers,
Pádraig






bug#55212: GNU Linux "sort -g" can hang indefinitely when run on standard input if NaNs are involved

2022-05-02 Thread Pádraig Brady

On 02/05/2022 07:03, Paul Eggert wrote:

Thanks for the bug report. This bug is entertaining, as it comes from
GCC now being so smart that it optimizes away a memset that cleared
padding bits. We added the memset in coreutils 8.14 (2011) to try to fix
the sort -g infinite loop bug (introduced in 1999), but the memset isn't
guaranteed to fix the bug because the memset can be optimized away.

If the padding bits happen to be clear already sort is OK, but if not
the results can be inconsistent when you compare two NaNs to each other,
and inconsistent results can make sort infloop.

The C standard allows this level of intelligence in the compiler, so
it's a bug in GNU 'sort'.

I installed the attached patch; please give it a try. For now I'll
boldly close the bug report; we can easily reopen it if this patch
doesn't actually fix the problem.


This is a bit slower of course, but since an edge case not a big concern:

  $ time yes nan | head -n128095 | timeout 10 sort -g >/dev/null
  real  0m0.693s

  $ time yes nan | head -n128095 | timeout 10 src/sort -g >/dev/null
  real  0m0.924s

I'll add the test case I think (attached)
since the existing one didn't trigger this.

thanks!
PádraigFrom bf12c9128523631d7f0ec251734fa05405f70c1c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= 
Date: Mon, 2 May 2022 14:27:34 +0100
Subject: [PATCH] tests: sort-NaN-infloop: augment testing for recent fix

* tests/misc/sort-NaN-infloop.sh: Add test case from
https://unix.stackexchange.com/a/700967/37127
* src/sort.c: Avoid syntax-check failure.
---
 src/sort.c | 2 +-
 tests/misc/sort-NaN-infloop.sh | 3 +++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/sort.c b/src/sort.c
index b2a465cf5..8af356c66 100644
--- a/src/sort.c
+++ b/src/sort.c
@@ -2006,7 +2006,7 @@ numcompare (char const *a, char const *b)
 static int
 nan_compare (long double a, long double b)
 {
-  char buf[2][sizeof "-nan()" + CHAR_BIT * sizeof a];
+  char buf[2][sizeof "-nan""()" + CHAR_BIT * sizeof a];
   snprintf (buf[0], sizeof buf[0], "%Lf", a);
   snprintf (buf[1], sizeof buf[1], "%Lf", b);
   return strcmp (buf[0], buf[1]);
diff --git a/tests/misc/sort-NaN-infloop.sh b/tests/misc/sort-NaN-infloop.sh
index 93cf9bd77..3e459884e 100755
--- a/tests/misc/sort-NaN-infloop.sh
+++ b/tests/misc/sort-NaN-infloop.sh
@@ -23,6 +23,9 @@ echo nan > F || framework_failure_
 printf 'nan\nnan\n' > exp || framework_failure_
 timeout 10 sort -g -m F F > out || fail=1
 
+# This was seen to infloop on some systems until coreutils v9.2 (bug 55212)
+yes nan | head -n128095 | timeout 10 sort -g > /dev/null || fail=1
+
 compare exp out || fail=1
 
 Exit $fail
-- 
2.26.2



bug#55183: date

2022-04-29 Thread Pádraig Brady

tag 55183 notabug
close 55183
stop

On 29/04/2022 13:59, danilopereira82--- via GNU coreutils Bug Reports wrote:

%r has a space at the end that makes me insanely angry fix it


What's happening is there is no "am_pm" item defined for your locale.
Running this with a locale that does define it, you can see:

  $ LC_TIME=C date +%r
  05:00:50 PM

  $ LC_TIME=zh_CN.utf8 date +%r
  下午 05时00分50秒

You can look up these values with:

  $ LC_TIME=C locale t_fmt_ampm t_fmt am_pm
  %I:%M:%S %p
  %H:%M:%S
  AM;PM

Note "t_fmt_ampm" if defined, takes precedence over "t_fmt ampm".

You could argue that an empty ampm should not output the space,
but that would have to be addressed in the libc strftime() implementation.

Another option is to use the more abstract %X format
to get the time in the most appropriate for one's locale,
which should be setup appropriately for locales that don't use AM/PM.

  $ LC_ALL=C date +%X
  17:00:50
  $ LC_ALL=en_US.utf8 date +%X
  05:00:50 PM
  $ LC_ALL=pt_PT.utf8 date +%X
  17:00:50

cheers,
Pádraig





bug#55023: Issue with CP empty folder after y2038 on 32-bits Kernel

2022-04-29 Thread Pádraig Brady

On 29/04/2022 00:56, Paul Eggert wrote:

On 4/27/22 09:42, Pádraig Brady wrote:

Marking this as done in the coreutils bug tracker,
now that this is being tracked in glibc.


This could also be worked around Gnulib for the benefit of 32-bit apps
running with unpatched glibc 2.34 and 2.35, or glibc older than 2.34.
Not sure it's worth the trouble, though, as the fixed glibc should be
universal pretty much everywhere before the year 2038 rolls around.


Yes I was thinking about that,
and also thought it not necessary to deal with in gnulib.

thanks,
Pádraig.





bug#55023: Issue with CP empty folder after y2038 on 32-bits Kernel

2022-04-27 Thread Pádraig Brady

Marking this as done in the coreutils bug tracker,
now that this is being tracked in glibc.

thank you!

On 27/04/2022 13:46, Adhemerval Zanella wrote:


On 21/04/2022 14:36, Pádraig Brady wrote:

That suggests the kernel (statx) returns fine,
but glibc's fstatat64 is returning the EOVERFLOW.



It seems to be a glibc missing support indeed.  The coreutils issues indicates
that lchmodat failed somehow:

   if (lchmodat (dst_dirfd, dst_relname, dst_mode | S_IRWXU) != 0)
 {
   error (0, errno, _("setting permissions for %s"),
  quoteaf (dst_name));
   goto un_backup;
 }

And lchmodat is a gnulib wrapper for fchmodat:

CHMODAT_INLINE int
lchmodat (int fd, char const *file, mode_t mode)
{
   return fchmodat (fd, file, mode, AT_SYMLINK_NOFOLLOW);
}

And since Linux fchmodat syscall does not provide a 'flag' argument (to
handle AT_SYMLINK_NOFOLLOW), glibc emulates it through opening a procfs file
descriptor, issuing fstatat to check if it is link (since some kernels and
filesystem it returns in inconsistent results), and then issue chmod.

However, the glibc internal fstat does not use the 64-bit version, which
then results in EOVERFLOW.

I have opened https://sourceware.org/bugzilla/show_bug.cgi?id=29097 and I will
fix it upstream and backport to 2.34 and 2.35.







bug#55029: Simple backup swaps source and destination files

2022-04-23 Thread Pádraig Brady

On 21/04/2022 03:53, Paul Eggert wrote:

On 4/19/22 16:05, Steve Ward wrote:

When doing mv or cp with --backup=simple, if an existing file in
DIRECTORY has the same name as SOURCE, the files appear to be swapped
instead of an in-place backup of the original file in DIRECTORY being
made.


Thanks for the bug report. That's new to coreutils 9.1, and is a big
enough fail that it suggests we'll need a 9.2 sooner rather than later.
I introduced the bug when fixing an earlier bug (sorry).

I installed the attached Gnulib patch, which should fix the bug in
Coreutils, with the attached two Coreutils patches to update to the
latest Gnulib, and to add a test case for the bug.


Thanks for the fix.

For the record, another failure mode is with different file systems:
  $ echo 1 > /tmp/a
  $ echo 2 > a
  $ echo 3 > a~
  $ src/mv -v --backup=simple a /tmp
  src/mv: cannot backup '/tmp/a': Invalid cross-device link
In this case the failure is not silent, and no changes are made.
This is a bit better, in that there is no possibility of silent data loss.

For the same file system case you might lose data in a setup like:
  $ cd workdir
  $ gen_data1 > data
  $ mv --backup=simple data1/
  $ gen_data2 > data
  $ mv --backup=simple data2/
In which case you'd lose the data~ backups in each of the data?/ dirs.

Another case which might be more problematic is with install(1).
  $ install --backup=simple blah /usr/local/dir
In which case we'd lose the blah~ backup upon removal of the build dir.

Note --backup without a specific type will default to simple
or numbered as appropriate, but this does _not_ have the
issue when defaulting to simple.

I've applied the fix to Fedora rawhide which had already moved to 9.1.
I've reported git issue at https://bugs.archlinux.org/task/74544

This should induce a coreutils 9.2 sooner,
but I don't think we need an immediate release just yet.

thanks,
Pádraig





bug#55023: Issue with CP empty folder after y2038 on 32-bits Kernel

2022-04-21 Thread Pádraig Brady

On 21/04/2022 18:41, Adhemerval Zanella wrote:



On 21/04/2022 12:25, Pádraig Brady wrote:

On 21/04/2022 15:53, Arnaud Panaïotis wrote:


On 21/04/2022 14:36, Pádraig Brady wrote:

On 19/04/2022 16:01, Arnaud Panaïotis wrote:

On 19/04/2022 16:00, Pádraig Brady wrote:

On 19/04/2022 08:47, Arnaud Panaïotis wrote:

Hello,

I did not received any feedback from this request right now. Have you
made any progress on this subject ?

Please let me know the progress for this, or contact me for additional
information if needed.

I'd like to have a ticket link to follow the advancement of this issue
(if possible). I'm available to test a patch if you are able to provide
me one.

Best regards,

On 01/04/2022 15:55, Arnaud Panaïotis wrote:


Hello,

I'm working for a client to generate embedded 32-bits Linux Kernel
working after y2038 issue.

I generated a 5.15 Kernel thought Buildroot with Coreutils 9.0, GCC
11.2.0, Binutils 2.37 and CFLAGS  -D_LARGEFILE_SOURCE
-D_FILE_OFFSET_BITS=64  -D_TIME_BITS=64

The Kernel pass y2038 but I found an issue with cp:
After analysis, the error occurs when trying to move an empty folder
without all user mode rights.

Here how to reproduce:

# mkdir -p test/test1 folder
# chmod u-w test/test1
# date -s "2040-04-02"
# cp -a test/* folder/
cp: setting permissions for 'folder/test1' : Value too large for defined data 
type

Note: The folder is copied before the error occurs. The copy works
fine before y2038.


The issue comes from coreutils-9.0/src/cp.c

Line 512 : if (lchmod (dir, stats.st_mode | S_IRWXU) != 0)

FYI I had a previous issue while calling lstat function from
 which is included in lib/lchmod.c. I used /usr/bin/stat
as a workaround.


Keep me in touch if you need more information.


The original mail seems to not have hit the lists, sorry.

The error suggests that the fstatat() done within lchmod()
is using a 32 bit time_t component of the stat structure.
Your kernel is new enough to support the 64 bit equivalent,
but you don't mention glibc. Can you ensure you're using
at least glibc 2.34, which added support for the 64 bit variants.

coreutils is configured by default to enable use
of the 64 bit variants where available, and you've confirmed
this as you say both -D_FILE_OFFSET_BITS=64 and -D_TIME_BITS=64 are defined.

An strace of the cp command would be useful to confirm the problematic syscall.


That suggests the kernel (statx) returns fine,
but glibc is returning the EOVERFLOW.
That suggests fstatat() rather than fstatat64() is being used
(inferring that by comparing
https://sourceware.org/git/?p=glibc.git;a=history;f=sysdeps/unix/sysv/linux/fstatat.c
https://sourceware.org/git/?p=glibc.git;a=history;f=sysdeps/unix/sysv/linux/fstatat64.c)

So why is fstatat() being used if compiling with
-D_FILE_OFFSET_BITS=64 and -D_TIME_BITS=64

You might confirm what statat is being called with:
nm cp | grep U.*statat

The redirect from fstatat() in code to the appropriate 64 bit interface
should be done with asm redirects and so immune to any undef etc.
that gnulib may be doing.

So in summary please look at how fstatat() is being referenced
on your system (by gnulib).

thanks,
Pádraig


Hi,

I had to add -D option to nm to avoid "no symbols" error.

  U __fstatat64_time64@GLIBC_2.34

All fstat, fstatat, lstat and stat are 64_time64 with the same GLIBC.


That looks correct.

So perhaps the EOVERFLOW is within your glibc's fstatat64 implementation?
You might get to the issue more quickly by installing debug symbols for
your coreutils and/or glibc and using: gdb -tui -args cp rest of cp args



Assuming a default config option (without --enable-kernel) the
_fstatat64_time64 should first try statx and then the old fstatat64
if statx fails with ENOSYS (on kernel older than 4.11).  The EOVERFLOW
only happens for later, assuming kernel does not returns anything
bogus.

What strace shows in this scenario?


strace shows statx() returning non error on this 5.15 kernel





bug#55023: Issue with CP empty folder after y2038 on 32-bits Kernel

2022-04-21 Thread Pádraig Brady

On 21/04/2022 15:53, Arnaud Panaïotis wrote:


On 21/04/2022 14:36, Pádraig Brady wrote:

On 19/04/2022 16:01, Arnaud Panaïotis wrote:

On 19/04/2022 16:00, Pádraig Brady wrote:

On 19/04/2022 08:47, Arnaud Panaïotis wrote:

Hello,

I did not received any feedback from this request right now. Have you
made any progress on this subject ?

Please let me know the progress for this, or contact me for additional
information if needed.

I'd like to have a ticket link to follow the advancement of this issue
(if possible). I'm available to test a patch if you are able to provide
me one.

Best regards,

On 01/04/2022 15:55, Arnaud Panaïotis wrote:


Hello,

I'm working for a client to generate embedded 32-bits Linux Kernel
working after y2038 issue.

I generated a 5.15 Kernel thought Buildroot with Coreutils 9.0, GCC
11.2.0, Binutils 2.37 and CFLAGS  -D_LARGEFILE_SOURCE
-D_FILE_OFFSET_BITS=64  -D_TIME_BITS=64

The Kernel pass y2038 but I found an issue with cp:
After analysis, the error occurs when trying to move an empty folder
without all user mode rights.

Here how to reproduce:

# mkdir -p test/test1 folder
# chmod u-w test/test1
# date -s "2040-04-02"
# cp -a test/* folder/
cp: setting permissions for 'folder/test1' : Value too large for defined data 
type

Note: The folder is copied before the error occurs. The copy works
fine before y2038.


The issue comes from coreutils-9.0/src/cp.c

Line 512 : if (lchmod (dir, stats.st_mode | S_IRWXU) != 0)

FYI I had a previous issue while calling lstat function from
 which is included in lib/lchmod.c. I used /usr/bin/stat
as a workaround.


Keep me in touch if you need more information.


The original mail seems to not have hit the lists, sorry.

The error suggests that the fstatat() done within lchmod()
is using a 32 bit time_t component of the stat structure.
Your kernel is new enough to support the 64 bit equivalent,
but you don't mention glibc. Can you ensure you're using
at least glibc 2.34, which added support for the 64 bit variants.

coreutils is configured by default to enable use
of the 64 bit variants where available, and you've confirmed
this as you say both -D_FILE_OFFSET_BITS=64 and -D_TIME_BITS=64 are defined.

An strace of the cp command would be useful to confirm the problematic syscall.


That suggests the kernel (statx) returns fine,
but glibc is returning the EOVERFLOW.
That suggests fstatat() rather than fstatat64() is being used
(inferring that by comparing
https://sourceware.org/git/?p=glibc.git;a=history;f=sysdeps/unix/sysv/linux/fstatat.c
https://sourceware.org/git/?p=glibc.git;a=history;f=sysdeps/unix/sysv/linux/fstatat64.c)

So why is fstatat() being used if compiling with
-D_FILE_OFFSET_BITS=64 and -D_TIME_BITS=64

You might confirm what statat is being called with:
nm cp | grep U.*statat

The redirect from fstatat() in code to the appropriate 64 bit interface
should be done with asm redirects and so immune to any undef etc.
that gnulib may be doing.

So in summary please look at how fstatat() is being referenced
on your system (by gnulib).

thanks,
Pádraig


Hi,

I had to add -D option to nm to avoid "no symbols" error.

     U __fstatat64_time64@GLIBC_2.34

All fstat, fstatat, lstat and stat are 64_time64 with the same GLIBC.


That looks correct.

So perhaps the EOVERFLOW is within your glibc's fstatat64 implementation?
You might get to the issue more quickly by installing debug symbols for
your coreutils and/or glibc and using: gdb -tui -args cp rest of cp args

thanks,
Pádraig.





bug#55023: Issue with CP empty folder after y2038 on 32-bits Kernel

2022-04-21 Thread Pádraig Brady

On 19/04/2022 16:01, Arnaud Panaïotis wrote:

On 19/04/2022 16:00, Pádraig Brady wrote:

On 19/04/2022 08:47, Arnaud Panaïotis wrote:

Hello,

I did not received any feedback from this request right now. Have you
made any progress on this subject ?

Please let me know the progress for this, or contact me for additional
information if needed.

I'd like to have a ticket link to follow the advancement of this issue
(if possible). I'm available to test a patch if you are able to provide
me one.

Best regards,

On 01/04/2022 15:55, Arnaud Panaïotis wrote:


Hello,

I'm working for a client to generate embedded 32-bits Linux Kernel
working after y2038 issue.

I generated a 5.15 Kernel thought Buildroot with Coreutils 9.0, GCC
11.2.0, Binutils 2.37 and CFLAGS  -D_LARGEFILE_SOURCE
-D_FILE_OFFSET_BITS=64  -D_TIME_BITS=64

The Kernel pass y2038 but I found an issue with cp:
After analysis, the error occurs when trying to move an empty folder
without all user mode rights.

Here how to reproduce:

# mkdir -p test/test1 folder
# chmod u-w test/test1
# date -s "2040-04-02"
# cp -a test/* folder/
cp: setting permissions for 'folder/test1' : Value too large for defined data 
type

Note: The folder is copied before the error occurs. The copy works
fine before y2038.


The issue comes from coreutils-9.0/src/cp.c

Line 512 : if (lchmod (dir, stats.st_mode | S_IRWXU) != 0)

FYI I had a previous issue while calling lstat function from
 which is included in lib/lchmod.c. I used /usr/bin/stat
as a workaround.


Keep me in touch if you need more information.


The original mail seems to not have hit the lists, sorry.

The error suggests that the fstatat() done within lchmod()
is using a 32 bit time_t component of the stat structure.
Your kernel is new enough to support the 64 bit equivalent,
but you don't mention glibc. Can you ensure you're using
at least glibc 2.34, which added support for the 64 bit variants.

coreutils is configured by default to enable use
of the 64 bit variants where available, and you've confirmed
this as you say both -D_FILE_OFFSET_BITS=64 and -D_TIME_BITS=64 are defined.

An strace of the cp command would be useful to confirm the problematic syscall.


That suggests the kernel (statx) returns fine,
but glibc is returning the EOVERFLOW.
That suggests fstatat() rather than fstatat64() is being used
(inferring that by comparing
https://sourceware.org/git/?p=glibc.git;a=history;f=sysdeps/unix/sysv/linux/fstatat.c
https://sourceware.org/git/?p=glibc.git;a=history;f=sysdeps/unix/sysv/linux/fstatat64.c)

So why is fstatat() being used if compiling with
-D_FILE_OFFSET_BITS=64 and -D_TIME_BITS=64

You might confirm what statat is being called with:
nm cp | grep U.*statat

The redirect from fstatat() in code to the appropriate 64 bit interface
should be done with asm redirects and so immune to any undef etc.
that gnulib may be doing.

So in summary please look at how fstatat() is being referenced
on your system (by gnulib).

thanks,
Pádraig





bug#55023: Issue with CP empty folder after y2038 on 32-bits Kernel

2022-04-19 Thread Pádraig Brady

On 19/04/2022 08:47, Arnaud Panaïotis wrote:

Hello,

I did not received any feedback from this request right now. Have you
made any progress on this subject ?

Please let me know the progress for this, or contact me for additional
information if needed.

I'd like to have a ticket link to follow the advancement of this issue
(if possible). I'm available to test a patch if you are able to provide
me one.

Best regards,

On 01/04/2022 15:55, Arnaud Panaïotis wrote:


Hello,

I'm working for a client to generate embedded 32-bits Linux Kernel
working after y2038 issue.

I generated a 5.15 Kernel thought Buildroot with Coreutils 9.0, GCC
11.2.0, Binutils 2.37 and CFLAGS  -D_LARGEFILE_SOURCE
-D_FILE_OFFSET_BITS=64  -D_TIME_BITS=64

The Kernel pass y2038 but I found an issue with cp:
After analysis, the error occurs when trying to move an empty folder
without all user mode rights.

Here how to reproduce:

# mkdir -p test/test1 folder
# chmod u-w test/test1
# date -s "2040-04-02"
# cp -a test/* folder/
cp: setting permissions for 'folder/test1' : Value too large for defined data 
type

Note: The folder is copied before the error occurs. The copy works
fine before y2038.


The issue comes from coreutils-9.0/src/cp.c

Line 512 : if (lchmod (dir, stats.st_mode | S_IRWXU) != 0)

FYI I had a previous issue while calling lstat function from
 which is included in lib/lchmod.c. I used /usr/bin/stat
as a workaround.


Keep me in touch if you need more information.


The original mail seems to not have hit the lists, sorry.

The error suggests that the fstatat() done within lchmod()
is using a 32 bit time_t component of the stat structure.
Your kernel is new enough to support the 64 bit equivalent,
but you don't mention glibc. Can you ensure you're using
at least glibc 2.34, which added support for the 64 bit variants.

coreutils is configured by default to enable use
of the 64 bit variants where available, and you've confirmed
this as you say both -D_FILE_OFFSET_BITS=64 and -D_TIME_BITS=64 are defined.

An strace of the cp command would be useful to confirm the problematic syscall.

thanks,
Pádraig





bug#50889: Please improve the documentation for install --compare (-C)

2022-04-09 Thread Pádraig Brady

On 29/09/2021 12:09, Ian Cross via GNU coreutils Bug Reports wrote:

Hi,

A while back a request was made to improve the documentation of the
`install --compare` cmd:

https://lists.gnu.org/archive/html/bug-coreutils/2009-09/msg00029.html

It seems the wording in `coreutils.texi` has been updated since
(https://github.com/coreutils/coreutils/commit/c8ac385299950ba84eb8c33f7e32e4d85b18e3ff
) but the manpage and `--help` as of coreutils v8.32 still leave things
vague:


compare each pair of source and destination files, and in some cases,

do not modify the destination at all

Florain Schlichting at the time of the request suggested this wording
instead:


compare each pair of source and destination files, and if identical

(by content, ownership and mode), do not copy to preserve mtime

It looks like a patch was made but somehow never made it into the
codebase. Please could the manpage and help be updated with this
wording so users can understand more readily what compare will do?


Both the --help and texinfo was incomplete / confusing.
I've installed the attached to address this.

Marking this as done.

thanks,
Pádraig
From 6d246fb68b7d8fcd6e7b3da88bd786b305f40f32 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= 
Date: Sat, 9 Apr 2022 12:42:37 +0100
Subject: [PATCH] doc: install --compare: clarify mode of operation

* doc/coreutils.texi (install invocation): For the --compare option,
clarify that the ownership or permissions of the source files don't
matter.  Also don't imply --owner or --group need to be specified
for --compare to be effective.
* src/install.c (usage): Add more detail on what's being compared.
---
 doc/coreutils.texi | 4 ++--
 src/install.c  | 5 +++--
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/doc/coreutils.texi b/doc/coreutils.texi
index c9243c683..8cfa698a1 100644
--- a/doc/coreutils.texi
+++ b/doc/coreutils.texi
@@ -9796,8 +9796,8 @@ The program accepts the following options.  Also see @ref{Common options}.
 @itemx --compare
 @opindex -C
 @opindex --compare
-Compare each pair of source and destination files, and if the destination has
-identical content and any specified owner, group, permissions, and possibly
+Compare content of source and destination files, and if there would be no
+change to the destination content, owner, group, permissions, and possibly
 SELinux context, then do not modify the destination at all.
 Note this option is best used in conjunction with @option{--user},
 @option{--group} and @option{--mode} options, lest @command{install}
diff --git a/src/install.c b/src/install.c
index 57a877f4a..079ce1f70 100644
--- a/src/install.c
+++ b/src/install.c
@@ -592,8 +592,9 @@ In the 4th form, create all components of the given DIRECTORY(ies).\n\
   --backup[=CONTROL]  make a backup of each existing destination file\n\
   -b  like --backup but does not accept an argument\n\
   -c  (ignored)\n\
-  -C, --compare   compare each pair of source and destination files, and\n\
-in some cases, do not modify the destination at all\n\
+  -C, --compare   compare content of source and destination files, and\n\
+if no change to content, ownership, and permissions,\n\
+do not modify the destination at all\n\
   -d, --directory treat all arguments as directory names; create all\n\
 components of the specified directories\n\
 "), stdout);
-- 
2.26.2



bug#54521: [PATCH] dircolors: colorize backup files with bright black

2022-04-03 Thread Pádraig Brady

On 23/03/2022 23:53, Pádraig Brady wrote:

On 22/03/2022 18:24, Ville Skyttä wrote:

Useful as it makes them stand out less than non-backup files.
---
   src/dircolors.hin | 19 +++
   1 file changed, 19 insertions(+)

diff --git a/src/dircolors.hin b/src/dircolors.hin
index 673835201..6dc5a2d74 100644
--- a/src/dircolors.hin
+++ b/src/dircolors.hin
@@ -211,5 +211,24 @@ EXEC 01;32
   .spx 00;36
   .xspf 00;36
   
+# backup files

+*~ 00;90
+*# 00;90
+.bak 00;90
+.old 00;90
+.orig 00;90
+.part 00;90
+.rej 00;90
+.swp 00;90
+.tmp 00;90
+.dpkg-dist 00;90
+.dpkg-old 00;90
+.ucf-dist 00;90
+.ucf-new 00;90
+.ucf-old 00;90
+.rpmnew 00;90
+.rpmorig 00;90
+.rpmsave 00;90
+
   # Subsequent TERM or COLORTERM entries, can be used to add / override
   # config specific to those matching environment variables.


I quite like that actually.
The default coloring works well on both dark and light backgrounds.
Coloring this class of files does seem useful.

+1 from me anyway.


I've pushed this after some perf testing showing it had negligible perf impact.
https://git.sv.gnu.org/cgit/coreutils.git/commit/?id=52aeae2c3

Here is a small example of the output:
https://www.pixelbeat.org/patches/coreutils/ls-backup-colors.html

cheers,
Pádraig





bug#54681: timeout: Program called by timeout cannot interact with tty stdin when timeout is called by exec()

2022-04-02 Thread Pádraig Brady

tag 54681 notabug
close 54681
stop

On 02/04/2022 11:22, Zhaofeng Yang wrote:

Hi GNU Team,

I found that program called by timeout cannot interact with tty stdin
when timeout is called by exec().

A simplest example is `timeout 10 timeout 5 cat`. cat cannot read input
from tty stdin.

I also tried to run `timeout 5 cat` in other programs by exec(), and
all of them cannot read tty stdin. For example,

import subprocess
subprocess.run(f"timeout 5 cat", shell=True)


timeout has a --foreground option to support this case,
with the caveat that if the program forks children,
they'll not be timed out.

See https://www.gnu.org/software/coreutils/timeout for details.

cheers,
Pádraig






bug#54625: Why do I need to specify `-o auto` for join's `-e` option to work?

2022-03-29 Thread Pádraig Brady

On 29/03/2022 09:07, Iago-lito wrote:

Hello,

      This is a follow-up to this (yet) unanswered post on SE U:
https://unix.stackexchange.com/q/696259/87656, which I'm cross-posting
here. Don't hesitate to fire if it's the wrong place.

With the following two simple files:

|a.txt|

|1 a 2 b 5 c |

|b.txt|

|2 x 4 y 5 z |

The following command does not behave like expected:

|$ join -a 1 -a 2 -e 0 a.txt b.txt 1 a 2 b x 4 y 5 c z |

I would expect the option |-e 0| to fill up missing values with zeroes.
However, the following does work:

|$ join -a 1 -a 2 -e 0 -o auto a.txt b.txt 1 a 0 2 b x 4 0 y 5 c z |

Reading documentation from |$ man join|, I see no connection between
|-o| and |-e| that would make the above behaviour meaningful. Instead, I
find it misleading that a useless |-o auto| needs to be inserted into my
command for |-e 0| to work..

Is there an explanation, to be clarified in the manpage? Or is it a bug?



The info doc is clearer that the output of missing fields need to be
explicitly enabled, https://www.gnu.org/software/coreutils/join :

  ‘-e STRING’
 Replace those output fields that are missing in the input with
 STRING.  I.e., missing fields specified with the ‘-12jo’ options.
I agree that the man page should mention this also,
which I've done in the attached.

Marking this as done.
thanks,
Pádraig

From 120ce321d5f768c00b9e193af4edd5ccceaa45e0 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= 
Date: Tue, 29 Mar 2022 16:34:10 +0100
Subject: [PATCH] doc: join: clarify that -e only effective for -12jo fields

* src/join.c (usage): Clarify that -e is not sufficient
to enable output of missing fields from one of the inputs.
Rather the -12jo options are required to explicitly
enable output of those fields.
Fixes https://bugs.gnu.org/54625
---
 src/join.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/src/join.c b/src/join.c
index 3d62ca43a..f2fd1727b 100644
--- a/src/join.c
+++ b/src/join.c
@@ -206,7 +206,11 @@ When FILE1 or FILE2 (not both) is -, read standard input.\n\
 \n\
   -a FILENUM also print unpairable lines from file FILENUM, where\n\
FILENUM is 1 or 2, corresponding to FILE1 or FILE2\n\
-  -e EMPTY   replace missing input fields with EMPTY\n\
+"), stdout);
+  fputs (_("\
+  -e STRING  replace missing (empty) input fields with STRING;\n\
+   I.e., missing fields specified with '-12jo' options\
+\n\
 "), stdout);
   fputs (_("\
   -i, --ignore-case  ignore differences in case when comparing fields\n\
-- 
2.26.2



bug#54521: [PATCH] dircolors: colorize backup files with bright black

2022-03-23 Thread Pádraig Brady

On 22/03/2022 18:24, Ville Skyttä wrote:

Useful as it makes them stand out less than non-backup files.
---
  src/dircolors.hin | 19 +++
  1 file changed, 19 insertions(+)

diff --git a/src/dircolors.hin b/src/dircolors.hin
index 673835201..6dc5a2d74 100644
--- a/src/dircolors.hin
+++ b/src/dircolors.hin
@@ -211,5 +211,24 @@ EXEC 01;32
  .spx 00;36
  .xspf 00;36
  
+# backup files

+*~ 00;90
+*# 00;90
+.bak 00;90
+.old 00;90
+.orig 00;90
+.part 00;90
+.rej 00;90
+.swp 00;90
+.tmp 00;90
+.dpkg-dist 00;90
+.dpkg-old 00;90
+.ucf-dist 00;90
+.ucf-new 00;90
+.ucf-old 00;90
+.rpmnew 00;90
+.rpmorig 00;90
+.rpmsave 00;90
+
  # Subsequent TERM or COLORTERM entries, can be used to add / override
  # config specific to those matching environment variables.


I quite like that actually.
The default coloring works well on both dark and light backgrounds.
Coloring this class of files does seem useful.

+1 from me anyway.

thanks,
Pádraig





bug#54388: printf doesn't handle multi-byte values

2022-03-18 Thread Pádraig Brady

On 18/03/2022 14:59, Pádraig Brady wrote:

The attached should fix this up.


The following should make this more efficient for the normal unibyte case,
as one can't have NUL chars in any multi-byte encodings.

-  if (MB_CUR_MAX > 1)   \
+  if (MB_CUR_MAX > 1 && *(s + 1))   \






bug#54388: printf doesn't handle multi-byte values

2022-03-18 Thread Pádraig Brady

On 14/03/2022 15:38, Pádraig Brady wrote:

On 14/03/2022 03:27, Christoph Anton Mitterer wrote:

Hey Pádraig.

I just wanted to ask, whether the following could be a bug in printf:

POSIX says[0], that e.g.:
 printf '%d\n' \"3
should give the numeric value of the character, and that "in a locale
with multi-byte characters, the value of a character is intended to be
the value of the equivalent of the wchar_t representation of the
character".

In bash:
$ printf '%d\n' $'"\u2208'
8712

here the printf is bash's built-in printf, and there it works.


But using GNU coreutils' printf (version 8.32):
$ /usr/bin/printf '%d\n' $'"\u2208'
/usr/bin/printf: warning: ��: character(s) following character constant have 
been ignored
226


Do I have some wrong assumptions or should I report that as a bug?


Thanks,
Chris.


[0] https://pubs.opengroup.org/onlinepubs/9699919799/utilities/printf.html


This is a limitation of current coreutils printf that only handles single byte 
chars currently.
This email will open an issue in our bug tracker.

To summarize:
$ ord() { printf "0x%x\n" "'$1"; }  # bash's printf
$ ord 3
0x33
$ ord $'\u2208'
0x2208

$ ord() { env printf "0x%x\n" "'$1"; }  # coreutils' printf
$ ord 3
0x33
$ ord $'\u2208'
0xprintf: warning: ��: character(s) following character constant have been 
ignored
e2


The attached should fix this up.

Marking this as done.

cheers,
Pádraig
From 3730d6f212dcb667594bb1be9fcb28dd419915f0 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= 
Date: Fri, 18 Mar 2022 14:52:36 +
Subject: [PATCH] printf: support printing the numeric value of multi-byte
 chars

* src/printf.c (STRTOX): Update to support multi-byte chars.
* tests/misc/printf-mb.sh: Add a new test.
* tests/local.mk: Reference the new test.
* NEWS: Mention the bug fix.
Fixes https://bugs.gnu.org/54388
---
 NEWS|  3 +++
 src/printf.c| 16 +
 tests/local.mk  |  1 +
 tests/misc/printf-mb.sh | 52 +
 4 files changed, 72 insertions(+)
 create mode 100755 tests/misc/printf-mb.sh

diff --git a/NEWS b/NEWS
index fe66f496f..7eaa1d158 100644
--- a/NEWS
+++ b/NEWS
@@ -42,6 +42,9 @@ GNU coreutils NEWS-*- outline -*-
   for B when A is a directory, possibly inflooping.
   [bug introduced in coreutils-6.3]
 
+  printf now supports printing the numeric value of multi-byte characters.
+  [This bug was present in "the beginning".]
+
   AIX builds no longer fail because some library functions are not found.
   [bug introduced in coreutils-8.32]
 
diff --git a/src/printf.c b/src/printf.c
index 5f84475fd..a0e81c02e 100644
--- a/src/printf.c
+++ b/src/printf.c
@@ -53,6 +53,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include "system.h"
 #include "cl-strtod.h"
@@ -170,6 +171,21 @@ FUNC_NAME (char const *s)		 \
 {	 \
   unsigned char ch = *++s;		 \
   val = ch; \
+ \
+  if (MB_CUR_MAX > 1)		 \
+{ \
+  mbstate_t mbstate = { 0, };	 \
+  wchar_t wc;			 \
+  size_t slen = strlen (s);	 \
+  ssize_t bytes;		 \
+  bytes = mbrtowc (, s, slen, );			 \
+  if (0 < bytes)		 \
+{ \
+  val = wc;			 \
+  s += bytes - 1;		 \
+} \
+} \
+ \
   /* If POSIXLY_CORRECT is not set, then give a warning that there	 \
  are characters following the character constant and that GNU	 \
  printf is ignoring those characters.  If POSIXLY_CORRECT *is*	 \
diff --git a/tests/local.mk b/tests/local.mk
index f97ddcb98..0f7778619 100644
--- a/tests/local.mk
+++ b/tests/local.mk
@@ -344,6 +344,7 @@ all_tests =	\
   tests/misc/printf.sh\
   tests/misc/printf-cov.pl			\
   tests/misc/printf-hex.sh			\
+  tests/misc/printf-mb.sh			\
   tests/misc/printf-surprise.sh			\
   tests/misc/printf-quote.sh			\
   tests/misc/pwd-long.sh			\
diff --git a/tests/misc/printf-mb.sh b/tests/misc/printf-mb.sh
new file mode 100755
index 0..ad21dbe67
--- /dev/null
+++ b/tests/misc/printf-mb.sh
@@ -0,0 +1,52 @@
+#!/bin/sh
+# tests for printing multi-byte values of characters
+
+# Copyright (C) 2022 Free Software Foundation, Inc.
+
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR P

bug#54388: printf doesn't handle multi-byte values

2022-03-14 Thread Pádraig Brady

On 14/03/2022 03:27, Christoph Anton Mitterer wrote:

Hey Pádraig.

I just wanted to ask, whether the following could be a bug in printf:

POSIX says[0], that e.g.:
printf '%d\n' \"3
should give the numeric value of the character, and that "in a locale
with multi-byte characters, the value of a character is intended to be
the value of the equivalent of the wchar_t representation of the
character".

In bash:
$ printf '%d\n' $'"\u2208'
8712

here the printf is bash's built-in printf, and there it works.


But using GNU coreutils' printf (version 8.32):
$ /usr/bin/printf '%d\n' $'"\u2208'
/usr/bin/printf: warning: ��: character(s) following character constant have 
been ignored
226


Do I have some wrong assumptions or should I report that as a bug?


Thanks,
Chris.


[0] https://pubs.opengroup.org/onlinepubs/9699919799/utilities/printf.html


This is a limitation of current coreutils printf that only handles single byte 
chars currently.
This email will open an issue in our bug tracker.

To summarize:
$ ord() { printf "0x%x\n" "'$1"; }  # bash's printf
$ ord 3
0x33
$ ord $'\u2208'
0x2208

$ ord() { env printf "0x%x\n" "'$1"; }  # coreutils' printf
$ ord 3
0x33
$ ord $'\u2208'
0xprintf: warning: ��: character(s) following character constant have been 
ignored
e2

cheers,
Pádraig





bug#54338: enhancement (documentation): explain permission tests in "test"

2022-03-11 Thread Pádraig Brady

On 11/03/2022 10:42, Ulrich Windl wrote:

I noticed that "test -r file" returns success when called as root for a file
with these permissions:
"--w---" (see also https://stackoverflow.com/q/71435657/6607497)

The documentation simply states:
‘-r FILE’
  True if FILE exists and read permission is granted.

Doing an strace it seems stat() is used to check the permissions (well, what
about ACLs, just in case?)

I think there should be a better explanatiomn how the permission tests work,
especially when called as "root".

(Report based on coreutils-8.32-150300.3.5.1 from openSUSE Leap 15.3)


I agree the current docs are ambiguous.
I'll apply the attached later to address this.

thanks,
Pádraig
From 8d4a616d5abe8bcd8a1760654a8f23b08cba92f3 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= 
Date: Fri, 11 Mar 2022 12:47:05 +
Subject: [PATCH] doc: test: clarify that -rwx don't just check perm bits

* src/test.c (usage): State that -rwx is determined by
user access, rather than permission bits.
* doc/coreutils.texi (Access permission tests): Likewise.
* man/test.x [SEE ALSO]: access(2).
Fixes https://bugs.gnu.org/54338
---
 doc/coreutils.texi | 6 +++---
 man/test.x | 2 ++
 src/test.c | 6 +++---
 3 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/doc/coreutils.texi b/doc/coreutils.texi
index 05dc5ee21..c9243c683 100644
--- a/doc/coreutils.texi
+++ b/doc/coreutils.texi
@@ -13451,7 +13451,7 @@ True if @var{file} exists and has its @dfn{sticky} bit set.
 @item -r @var{file}
 @opindex -r
 @cindex readable file check
-True if @var{file} exists and read permission is granted.
+True if @var{file} exists and the user has read access.
 
 @item -u @var{file}
 @opindex -u
@@ -13461,12 +13461,12 @@ True if @var{file} exists and has its set-user-ID bit set.
 @item -w @var{file}
 @opindex -w
 @cindex writable file check
-True if @var{file} exists and write permission is granted.
+True if @var{file} exists and the user has write access.
 
 @item -x @var{file}
 @opindex -x
 @cindex executable file check
-True if @var{file} exists and execute permission is granted
+True if @var{file} exists and the user has execute access
 (or search permission, if it is a directory).
 
 @item -O @var{file}
diff --git a/man/test.x b/man/test.x
index 0adc35fee..61a2d0a2f 100644
--- a/man/test.x
+++ b/man/test.x
@@ -17,3 +17,5 @@ test \- check file types and compare values
 .I OPTION
 [DESCRIPTION]
 .\" Add any additional description here
+[SEE ALSO]
+access(2)
diff --git a/src/test.c b/src/test.c
index 6daad3b34..cea7dc10d 100644
--- a/src/test.c
+++ b/src/test.c
@@ -750,15 +750,15 @@ EXPRESSION is true or false and sets exit status.  It is one of:\n\
   -N FILE FILE exists and has been modified since it was last read\n\
   -O FILE FILE exists and is owned by the effective user ID\n\
   -p FILE FILE exists and is a named pipe\n\
-  -r FILE FILE exists and read permission is granted\n\
+  -r FILE FILE exists and the user has read access\n\
   -s FILE FILE exists and has a size greater than zero\n\
 "), stdout);
   fputs (_("\
   -S FILE FILE exists and is a socket\n\
   -t FD   file descriptor FD is opened on a terminal\n\
   -u FILE FILE exists and its set-user-ID bit is set\n\
-  -w FILE FILE exists and write permission is granted\n\
-  -x FILE FILE exists and execute (or search) permission is granted\n\
+  -w FILE FILE exists and the user has write access\n\
+  -x FILE FILE exists and the user has execute (or search) access\n\
 "), stdout);
   fputs (_("\
 \n\
-- 
2.26.2



bug#54287: [PATCH] Fix stat command triggering automount.

2022-03-07 Thread Pádraig Brady

New stat patch attached to apply AT_NO_AUTOMOUNT
unless --cached=never is specified.

See https://bugs.gnu.org/54286 for more general discussion.
Marking this as done.

cheers,
PádraigFrom 3fddd0f4eb0eb9b47294f951195233e1c5f12e72 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= 
Date: Mon, 7 Mar 2022 14:32:03 +
Subject: [PATCH] stat: only automount with --cached=never

Revert to the default behavior before the introduction of statx().

* src/stat.c (do_stat): Set AT_NO_AUTOMOUNT without --cached=never.
* doc/coreutils.texi (stat invocation): Mention the automount
behavior with --cached=never.
* NEWS: Mention the fix.

Fixes https://bugs.gnu.org/54287
---
 NEWS   | 5 +++--
 doc/coreutils.texi | 1 +
 src/stat.c | 3 +++
 3 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/NEWS b/NEWS
index 12051cd15..8e95be755 100644
--- a/NEWS
+++ b/NEWS
@@ -34,8 +34,9 @@ GNU coreutils NEWS-*- outline -*-
   and the documentation has been clarified for unusual cases.
   [bug introduced in coreutils-7.0]
 
-  ls no longer tries to automount files, reverting to the behavior
-  before the statx() call was introduced.
+  ls and stat no longer try to automount files, reverting to the behavior
+  before the statx() call was introduced.  Only `stat --cached=never`
+  will continue to automount files.
   [bug introduced in coreutils-8.32]
 
   On macOS, 'mv A B' no longer fails with "Operation not supported"
diff --git a/doc/coreutils.texi b/doc/coreutils.texi
index e9be0993a..05dc5ee21 100644
--- a/doc/coreutils.texi
+++ b/doc/coreutils.texi
@@ -12608,6 +12608,7 @@ Always read the already cached attributes if available.
 
 @item never
 Always sychronize with the latest file system attributes.
+This also mounts automounted files.
 
 @item default
 Leave the caching behavior to the underlying file system.
diff --git a/src/stat.c b/src/stat.c
index edafd0285..3765a8f65 100644
--- a/src/stat.c
+++ b/src/stat.c
@@ -1394,6 +1394,9 @@ do_stat (char const *filename, char const *format, char const *format2)
   else if (force_sync)
 flags |= AT_STATX_FORCE_SYNC;
 
+  if (! force_sync)
+flags |= AT_NO_AUTOMOUNT;
+
   fd = statx (fd, pathname, flags, format_to_mask (format), );
   if (fd < 0)
 {
-- 
2.26.2



bug#54286: [PATCH] Fix ls -l triggering automounts.

2022-03-07 Thread Pádraig Brady

Updated patch for ls attached as per discussion.
Added a NEWS entry.From e4c7fccc0c057a772b6be3f002003ba005cc89d7 Mon Sep 17 00:00:00 2001
From: Rohan Sable 
Date: Mon, 7 Mar 2022 14:14:13 +
Subject: [PATCH] ls: avoid triggering automounts

statx() has different defaults wrt automounting
compared to stat() or lstat(), so explicitly
set the AT_NO_AUTOMOUNT flag to suppress that behavior,
and avoid unintended operations or potential errors.

* src/ls.c (do_statx): Pass AT_NO_AUTOMOUNT to avoid this behavior.
* NEWS: Mention the bug fix.

Signed-off-by: Rohan Sable 
---
 NEWS | 4 
 src/ls.c | 2 +-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/NEWS b/NEWS
index af6596b06..12051cd15 100644
--- a/NEWS
+++ b/NEWS
@@ -34,6 +34,10 @@ GNU coreutils NEWS-*- outline -*-
   and the documentation has been clarified for unusual cases.
   [bug introduced in coreutils-7.0]
 
+  ls no longer tries to automount files, reverting to the behavior
+  before the statx() call was introduced.
+  [bug introduced in coreutils-8.32]
+
   On macOS, 'mv A B' no longer fails with "Operation not supported"
   when A and B are in the same tmpfs file system.
   [bug introduced in coreutils-9.0]
diff --git a/src/ls.c b/src/ls.c
index 1930e4abb..255789061 100644
--- a/src/ls.c
+++ b/src/ls.c
@@ -1177,7 +1177,7 @@ do_statx (int fd, char const *name, struct stat *st, int flags,
 {
   struct statx stx;
   bool want_btime = mask & STATX_BTIME;
-  int ret = statx (fd, name, flags, mask, );
+  int ret = statx (fd, name, flags | AT_NO_AUTOMOUNT, mask, );
   if (ret >= 0)
 {
   statx_to_stat (, st);
-- 
2.26.2



bug#54286: [PATCH] fcntl-h: add AT_NO_AUTOMOUNT

2022-03-07 Thread Pádraig Brady
* lib/fcntl.in.h: Define AT_NO_AUTOMOUNT to 0 where not defined.
This is available on Linux since 2.6.38.
---
 ChangeLog  | 6 ++
 lib/fcntl.in.h | 4 
 2 files changed, 10 insertions(+)

diff --git a/ChangeLog b/ChangeLog
index c5a80fd3f3..e3f0ed216c 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,9 @@
+2022-03-07  Pádraig Brady  
+
+   fcntl-h: add AT_NO_AUTOMOUNT
+   * lib/fcntl.in.h: Define AT_NO_AUTOMOUNT to 0 where not defined.
+   This is available on Linux since 2.6.38.
+
 2022-03-01  Paul Eggert  
 
Create lib/Makefile.am after gnulib-comp.m4
diff --git a/lib/fcntl.in.h b/lib/fcntl.in.h
index 3e0c302af3..9270ced897 100644
--- a/lib/fcntl.in.h
+++ b/lib/fcntl.in.h
@@ -435,6 +435,10 @@ _GL_WARN_ON_USE (openat, "openat is not portable - "
 # define AT_EACCESS 4
 #endif
 
+/* Ignore this flag if not supported.  */
+#ifndef AT_NO_AUTOMOUNT
+# define AT_NO_AUTOMOUNT 0
+#endif
 
 #endif /* _@GUARD_PREFIX@_FCNTL_H */
 #endif /* _@GUARD_PREFIX@_FCNTL_H */
-- 
2.26.2






bug#54286: [PATCH] Fix ls -l triggering automounts.

2022-03-07 Thread Pádraig Brady

On 07/03/2022 07:54, Rohan Sable wrote:

Running ls -l on a path that has autofs mounts,
triggers a mount or in case of unmountable shares,
triggers errors :
~~~
[root@rsablerhel85 mnt2]# ll
ls: cannot access 'testshare2': No such file or directory < Error
total 0
drwxrwxrwx. 3 1000 1000 15 Jan 17 12:08 testshare < mount is 
triggerd for testshare
d?? ? ?? ?? testshare2< Path we 
know that is inaccessible throws an error

[root@rsablerhel85 mnt2]# mount | grep -i test
rsable76server:/testshare on /mnt2/testshare type nfs 
(rw,relatime,vers=3,rsize=32768,wsize=32768,namlen=255,hard,proto=tcp,timeo=600,retrans=6,sec=sys,mountaddr=192.168.122.58,mountvers=3,mountport=20048,mountproto=tcp,local_lock=none,addr=192.168.122.58)
~~~

Added AT_NO_AUTOMOUNT flag to do_lstat to fix this behavior.


Yes we should handle this, but a bit differently I think.

In this and the stat(1) patch you're only doing the adjustment for lstat().
I'd be more inclined to do this for all statx() uses because:
 - stat() and lstat() behave as if AT_NO_AUTOMOUNT is set
   - stat can be significant here for ondemand dirs, bind mounted entries, or 
network contexts etc.
 - ls(1) and stat(1) are operating on meta-data so should avoid mounts by 
default

Also we have to provide some ifdef protection around AT_NO_AUTOMOUNT use.
That's best done in gnulib in fcntl.in.h

Also we have related control in the --cached=never option in stat(1),
which is meant to ensure we go the whole way to the source file system,
which maps well with _not_ setting AT_NO_AUTOMOUNT for that case only.

I'll follow up with some patches along those lines.

thanks,
Pádraig





bug#54189: [PATCH] dircolors: add hterm

2022-02-27 Thread Pádraig Brady

On 27/02/2022 21:08, Mike Frysinger wrote:

* src/dircolors.hin: Add hterm*.
---
  src/dircolors.hin | 1 +
  1 file changed, 1 insertion(+)

diff --git a/src/dircolors.hin b/src/dircolors.hin
index 673835201311..fa01192bf7e5 100644
--- a/src/dircolors.hin
+++ b/src/dircolors.hin
@@ -23,6 +23,7 @@ TERM cygwin
  TERM *direct*
  TERM dtterm
  TERM gnome
+TERM hterm*
  TERM hurd
  TERM jfbterm
  TERM konsole


Do you have more info on the source of this terminal
and how popular it is?

Note we've recently enabled colors for any terminal that sets
the COLORTERM env var to anything, so if hterm sets that
it would already be catered for.

cheers,
Pádraig





bug#45648: `dd` seek/skip which way is up?

2022-02-24 Thread Pádraig Brady

On 22/02/2022 17:12, Paul Eggert wrote:

On 1/4/21 20:08, Paul Eggert wrote:

On 1/4/21 7:44 PM, Bela Lubkin wrote:

TLDR: *huge* existing presence of 'iseek' and 'oseek'; most OSes document
them as pure synonyms for 'skip' and 'seek'.


Thanks for doing all that research. It's compelling, and I think your
patch (or something like it) should go in. I'll wait for a bit to hear
other opinions.


After thinking about the patch a bit more, let's omit the part about
adding new conversions iseek_bytes etc., as I think there's a better way
to address that issue. I proposed something in .

So instead of your patch, I installed the attached patches. The first
one adds the iseek and oseek operands that you suggested; the second one
clarifies dd documentation, as I found several things were confusing
when rereading it carefully. Something like these patches should appear
in the next coreutils release.


+1

The aliases are useful.
I always remembered it like skIp for Input,
but that is awkward.

As for the overlap in solaris with disabling reading,
I think that would be better as a flag, like "seek_only",
if deemed useful.

thanks,
Pádraig





bug#54124: fmt inserts garbage in certain cases?

2022-02-23 Thread Pádraig Brady

On 23/02/2022 17:55, Pádraig Brady wrote:


I think isspace(x85) returning true on macOS is a bug,


Bug is a bit of a strong word here.

A digression into why 0x85 is being treated specially here.
Note Cyrillic kha "х" is encoded in UTF-8 as:
 $ printf '\u0445' | od -tx1
 000 d1 85

What I think is happening is \u0085 represents "Next Line" in unicode.
This is present in unicode to support mapping to/from the corresponding char in 
EBCDIC,
which had a distinct char for this in addition to CR and LF.
Given isspace('\n') returns true, then it makes some sense that isspace("Next 
Line")
would return true, and I guess through implementation details
isspace(int) is operating on utf32 on macOS in UTF-8 locales
and this returning true for this value.

BTW 0xA0 is the only other value that isspace() returns true for
(other than the standard c_isspace() values of course).
This is non breaking space, so it's best we don't split on it anyway.
I.e. this is another benefit to the change.

I still think using c_isspace() to avoid this issue is best,
and intend to push the change tomorrow.

cheers,
Pádraig





bug#54124: fmt inserts garbage in certain cases?

2022-02-23 Thread Pádraig Brady

On 23/02/2022 10:58, JD wrote:

Hi!

I have fmt from coreutils 8.32.1 installed via MacPorts.

If I run the following command: `echo х х х х х х х х х х х х х х х х х х х х х 
х х х х х | gfmt -sw 10` (which is just echoing 26 Cyrillic 'х' ('kha') 
letters), I get the following results:

https://i.imgur.com/yRx7uuz.png (iTerm2)
https://i.imgur.com/7oQ0UPz.png (iTerm2 if passed via `more`)
https://i.imgur.com/UlLrEMy.png (Alacritty)

And if I delete just two 'х' letters, like this: `echo х х х х х х х х х х х х 
х х х х х х х х х х х х | gfmt -sw 10`, evertyhitng shows just fine: 
https://i.imgur.com/DwuWxyx.png

Would be grateful for any advice :)


The issue here is that (on macOS 10.15.7 at least),
isspace(0x85) returns true for UTF-8 locales
(but not for "C" or "iso8859-1" locales).
BTW iscntrl() returns true for 0x85 on all non C locales
on both Linux and macOS.

Now gnulib says wrt isspace() that:

"This function's behaviour depends on the locale, but does not support
the multibyte characters that occur in strings in locales with
@code{MB_CUR_MAX > 1} (this includes all the common UTF-8 locales)."

I think isspace(x85) returning true on macOS is a bug,
but we should probably avoid isspace() in fmt altogether
given it's inconsistency with multibyte locales.
The attached uses c_isspace() instead.

cheers,
PádraigFrom 166b6783bc1a6e0ce206114c1d593c2528e3cfa1 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= 
Date: Wed, 23 Feb 2022 17:50:46 +
Subject: [PATCH] fmt: fix invalid multi-byte splitting on macOS

On macOS, isspace(0x85) returns true,
which results in splitting within multi-byte characters.

* src/fmt.c (get_line): s/isspace/c_isspace/.
* tests/fmt/non-space.sh: Add a new test.
* tests/local.mk: Reference new test.
* NEWS: Mention the fix.
Addresses https://bugs.gnu.org/54124
---
 NEWS   |  4 
 src/fmt.c  |  3 ++-
 tests/fmt/non-space.sh | 49 ++
 tests/local.mk |  3 ++-
 4 files changed, 57 insertions(+), 2 deletions(-)
 create mode 100755 tests/fmt/non-space.sh

diff --git a/NEWS b/NEWS
index ef65b4ab8..35d9a50dd 100644
--- a/NEWS
+++ b/NEWS
@@ -21,6 +21,10 @@ GNU coreutils NEWS-*- outline -*-
   and B is in some other file system.
   [bug introduced in coreutils-9.0]
 
+  On macOS, fmt no longer corrupts multi-byte characters
+  by misdetecting their component bytes as spaces.
+  [This bug was present in "the beginning".]
+
   'id xyz' now uses the name 'xyz' to determine groups, instead of xyz's uid.
   [bug introduced in coreutils-8.22]
 
diff --git a/src/fmt.c b/src/fmt.c
index 1eb7019b0..05bafabd6 100644
--- a/src/fmt.c
+++ b/src/fmt.c
@@ -26,6 +26,7 @@
it to be a type get syntax errors for the variable declaration below.  */
 #define word unused_word_type
 
+#include "c-ctype.h"
 #include "system.h"
 #include "error.h"
 #include "die.h"
@@ -702,7 +703,7 @@ get_line (FILE *f, int c)
   *wptr++ = c;
   c = getc (f);
 }
-  while (c != EOF && !isspace (c));
+  while (c != EOF && !c_isspace (c));
   in_column += word_limit->length = wptr - word_limit->text;
   check_punctuation (word_limit);
 
diff --git a/tests/fmt/non-space.sh b/tests/fmt/non-space.sh
new file mode 100755
index 0..b59838983
--- /dev/null
+++ b/tests/fmt/non-space.sh
@@ -0,0 +1,49 @@
+#!/bin/sh
+# Test fmt space handling
+
+# Copyright (C) 2022 Free Software Foundation, Inc.
+
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see .
+
+. "${srcdir=.}/tests/init.sh"; path_prepend_ ./src
+print_ver_ fmt printf
+
+# Before coreutils 9.1 macOS treated bytes like 0x85
+# as space characters in multi-byte locales (including UTF-8)
+
+check_non_space() {
+  char="$1"
+  test "$(env printf "=$char=" | fmt -s -w1 | wc -l)" = 1 || fail=1
+}
+
+export LC_ALL=en_US.iso8859-1  # only lowercase form works on macOS 10.15.7
+if test "$(locale charmap 2>/dev/null | sed 's/iso/ISO-/')" = ISO-8859-1; then
+  check_non_space '\xA0'
+fi
+
+export LC_ALL=en_US.UTF-8
+if test "$(locale charmap 2>/dev/null)" = UTF-8; then
+  check_non_space '\u00A0'  # No break space
+  check_non_space '\u2007'  # TODO: should probably split on figure space
+  check_non_space '\u202F'  # Narrow no break space
+  check_non_space '\u2060'  # zero-width no break space
+  check_non_space '\u0445'  # Cyrillic 

bug#54112: dd seek_bytes etc. is confusing

2022-02-22 Thread Pádraig Brady

On 22/02/2022 17:03, Paul Eggert wrote:

While looking into Bug#45648 I noticed that the GNU extensions
count_bytes, seek_bytes, and skip_bytes are confusing, and the proposed
fix to bug#45648 would make them even more confusing. To fix this
confusion, we should deprecate these options, and instead say that if
you want to use byte counts you should use a number string ending in "B".

Here's another way to put it.  Currently this:

 dd oseek=100KiB

means "seek 102,400 blocks". It should simply mean "seek 102,400 bytes",
which is what it says. And if we change oseek's meaning this way, we
don't need "oseek_bytes".

Although this is an incompatible change to GNU dd, I don't think it'll
affect real-world uses (who would use oseek in such a confusing way
now?) and overall it will be a win.


That is a more concise and direct way to achieve the same functionality.
+1

I guess we should remove docs for the other options,
but leave support there for backwards compat.

thanks,
Pádraig





bug#53977: Improve markup in man pages

2022-02-14 Thread Pádraig Brady

On 14/02/2022 13:52, Mario Blättermann wrote:

Hello Pádraig,

Am Mo., 14. Feb. 2022 um 13:15 Uhr schrieb Pádraig Brady :


On 13/02/2022 13:19, Mario Blättermann wrote:

Hello,

the SEE ALSO sections in the man pages contain links which will be
pulled in from *.x files by help2man. While help2man evaluates the
Groff markup from --help and --version output, it doesn't bother with
the markup in the *.x files. See the attached patch. The bold
formatting of the links is especially useful in HTML output (but also
in terminal output); the links become clickable and point to the
respective man page in online collections [1]. You can test the
behavior in the German version, where the links are already properly
formatted [2].

[1] https://man.archlinux.org/man/cat.1
[2] https://man.archlinux.org/man/cat.1.de


Sorry. I'm still not convinced on this.
It seems like a layering violation to stipulate a style here.
The renderer should have enough context to highlight appropriately.
See for example:

https://man7.org/linux/man-pages/man1/sort.1.html



Maybe some renderers are smart enough to highlight this. But it
shouldn't be up to the developers of such renderers to apply missing
formattings virtually.


Thanks for the consideration on this.
I'm not strongly against hardcoding the formatting,
but I do think it's worth discussing the need.

Note we discussed this previously at:
https://lists.gnu.org/archive/html/coreutils/2021-01/msg8.html

The summary there was things have trended over time
from a mixture of none,italic,bold style references
to mostly bold.


Note the man7.org renderer only highlights the SEE ALSO references,
when ideally it would highlight all instances of this pattern.
Anyway handling references outside of the SEE ALSO section,
is another reason to have the renderer do this consistently.
See for example all the appropriately highlighted references in:

http://man.he.net/?topic=sort=all
https://man.cx/sort


Yes, but this doesn't work in all imaginable cases, because the
renderer needs to be able to evaluate if it is a link or not. In your
example, "shuf" is a link because it is tagged with the section
number, but this wouldn't, then "shuf" would be as plain as other
words. See the "diff3.1" man page [1]: > > -e, --ed
output ed script incorporating changes from OLDFILE to YOURFILE into MYFILE

If "ed" would be "ed(1)" then it would be detected as a command name
here, but it isn't.


But it isn't bold either.
I suggest this page should be updated to use "ed(1)".


Well, the best solution would be to dig in the
Help2man code and try to improve the detection of parts worth to be
formatted - provided solid Perl skills. But this would be outside of
the topic of this bug report;


Yes a `help2man --bold-refs` option seems useful.
It give the option to apply the formatting,
and more centrally, rather than inconsistently sprinkled
through all man pages.
Also this would be the only way to consistently
style all references in coreutils, since some are output
to the terminal through --help.


let's go back to the SEE ALSO links. The
man page man-pages(7) says [2]:

»The name of the command, and its options, should always be formatted in bold.«
 
In fact, the SEE ALSO links are also command names, although not the
command the current man page describes. 


The bold here is pertaining to styling of the headers of the page,
rather than to any command references in general IMHO.
In general bold text interspersed in other text on the terminal
can be quite distracting to read. Some terminals can make it
bigger, brighter, blurrier, ...


Let's have a look at other projects [that hardcode bold references]

> grub, grep, kernel.

I see Paul added the grep markup recently in a seemingly unrelated change:
https://git.savannah.gnu.org/gitweb/?p=grep.git;a=commit;h=fe630c9f


I could mention lots of similar examples, but just open an arbitrary
man page in your terminal with "man", not in Vim; you will see that
(almost …) all SEE ALSO links are formatted bold.


I did check also, and see lots of inconsistencies.
BTW re vim, the match for references is:
  syn match manReference "\f\+([1-9][a-z]\=)"


With the bold
formatting (and correct placing of the section number), you make sure
that *all* renderers, both the simple ones like "man" in the terminal
and the advanced HTML/DVI/PDF/whatever renderers, format the links
correctly.


"correctly" is a strong word here.


[1] https://man.cx/diff3#heading3
[2] 
https://man.archlinux.org/man/man-pages.7.en#Formatting_conventions_for_manual_pages_describing_commands
[3] https://git.savannah.gnu.org/cgit/grub.git/tree/docs/man/grub-editenv.h2m
[4] https://git.savannah.gnu.org/cgit/grep.git/tree/doc/grep.in.1#n1359
[5] 
https://git.kernel.org/pub/scm/docs/man-pages/man-pages.git/tree/man1/mtrace.1#n64
[6] https://github.com/freebsd/freebsd-src/blob/main/usr.bin/sort/s

bug#53977: Improve markup in man pages

2022-02-14 Thread Pádraig Brady

On 13/02/2022 13:19, Mario Blättermann wrote:

Hello,

the SEE ALSO sections in the man pages contain links which will be
pulled in from *.x files by help2man. While help2man evaluates the
Groff markup from --help and --version output, it doesn't bother with
the markup in the *.x files. See the attached patch. The bold
formatting of the links is especially useful in HTML output (but also
in terminal output); the links become clickable and point to the
respective man page in online collections [1]. You can test the
behavior in the German version, where the links are already properly
formatted [2].

[1] https://man.archlinux.org/man/cat.1
[2] https://man.archlinux.org/man/cat.1.de


Sorry. I'm still not convinced on this.
It seems like a layering violation to stipulate a style here.
The renderer should have enough context to highlight appropriately.
See for example:

https://man7.org/linux/man-pages/man1/sort.1.html

Note the man7.org renderer only highlights the SEE ALSO references,
when ideally it would highlight all instances of this pattern.
Anyway handling references outside of the SEE ALSO section,
is another reason to have the renderer do this consistently.
See for example all the appropriately highlighted references in:

http://man.he.net/?topic=sort=all
https://man.cx/sort

thanks,
Pádraig





bug#49239: Unexpected results with sort -V

2022-02-13 Thread Pádraig Brady

On 13/02/2022 05:31, Paul Eggert wrote:

On 6/28/21 10:54, Kamil Dudka wrote:

You are right.  The matching algorithm was not implemented correctly and
the patch you attached fixes it.


I looked into Bug#49239 and found some more places where the
documentation disagreed with the code. I installed the attached patches
into Gnulib and Coreutils, respectively, which should bring the two into
agreement and should fix the bugs that Michael reported albeit in a
different way than his proposed patch. Briefly:

* The code didn't allow file name suffixes to be the entire file name,
but the documentation did. Here I went with the documentation. I could
be talked into the other way; it shouldn't matter much either way.

* The code did the preliminary test (without suffixes) using strcmp, the
documentation said it should use version comparison. Here I went with
the documentation.

* As Michael mentioned, sort -V mishandled NUL. I fixed this by adding a
Gnulib function filenvercmp that treats NUL as just another character.

* As Michael also mentioned, filevercmp fell back on strcmp if version
sort found no difference, which meant sort's --stable flag was
ineffective. I fixed this by not having filevercmp fall back on strcmp.

* I fixed the two-consecutive dot and trailing-dot bugs Michael
mentioned, by rewriting the suffix finder to not have that confusing
READ_ALPHA state variable, and to instead implement the regular
expression's nested * operators in the usual way with nested loops.

Thanks, Michael, for reporting the problem. I'm boldly closing the
Coreutils bug report as fixed.


A very thorough analysis.
All looks good.

thank you!
Pádraig





bug#53631: coreutils id(1) incorrect behavior

2022-02-12 Thread Pádraig Brady

On 04/02/2022 22:59, Paul Eggert wrote:

Thanks for the bug report. I installed the attached patch, which I hope
fixes things for you, and am boldly closing the bug report.

This fix depends on the latest lib/userspec.c from Gnulib; see
.


I'll apply the following later to ensure
chown --verbose continues to list numeric ID info.

cheers,
PádraigFrom 8113e620d950cc4beab38dfab8354852b778c583 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= 
Date: Sat, 12 Feb 2022 21:20:21 +
Subject: [PATCH] chown,chgrp: reinstate numeric id output in -v messages

since gnulib commit ff208d546a,
related to coreutils commit v9.0-143-gabde15969
we no longer maintain numeric IDs through chopt->{user,group}_name.
Therefore we need to adjust to ensure tests/chown/basic.sh passes.

* src/chown-core.c (uid_to_str, gid_to_str): New helper functions
to convert numeric id to string.
(change_file_owner): Use the above new functions to pass
numeric ids to describe_change().
---
 src/chown-core.c | 44 +++-
 1 file changed, 35 insertions(+), 9 deletions(-)

diff --git a/src/chown-core.c b/src/chown-core.c
index f7d03297e..428c96de9 100644
--- a/src/chown-core.c
+++ b/src/chown-core.c
@@ -73,6 +73,28 @@ chopt_free (struct Chown_option *chopt)
   free (chopt->group_name);
 }
 
+/* Convert the numeric user-id, UID, to a string stored in xmalloc'd memory,
+   and return it.  Use the decimal representation of the ID.  */
+
+static char *
+uid_to_str (uid_t uid)
+{
+  char buf[INT_BUFSIZE_BOUND (intmax_t)];
+  return xstrdup (TYPE_SIGNED (uid_t) ? imaxtostr (uid, buf)
+  : umaxtostr (uid, buf));
+}
+
+/* Convert the numeric group-id, GID, to a string stored in xmalloc'd memory,
+   and return it.  Use the decimal representation of the ID.  */
+
+static char *
+gid_to_str (gid_t gid)
+{
+  char buf[INT_BUFSIZE_BOUND (intmax_t)];
+  return xstrdup (TYPE_SIGNED (gid_t) ? imaxtostr (gid, buf)
+  : umaxtostr (gid, buf));
+}
+
 /* Convert the numeric group-id, GID, to a string stored in xmalloc'd memory,
and return it.  If there's no corresponding group name, use the decimal
representation of the ID.  */
@@ -80,11 +102,8 @@ chopt_free (struct Chown_option *chopt)
 extern char *
 gid_to_name (gid_t gid)
 {
-  char buf[INT_BUFSIZE_BOUND (intmax_t)];
   struct group *grp = getgrgid (gid);
-  return xstrdup (grp ? grp->gr_name
-  : TYPE_SIGNED (gid_t) ? imaxtostr (gid, buf)
-  : umaxtostr (gid, buf));
+  return grp ? xstrdup (grp->gr_name) : gid_to_str (gid);
 }
 
 /* Convert the numeric user-id, UID, to a string stored in xmalloc'd memory,
@@ -94,11 +113,8 @@ gid_to_name (gid_t gid)
 extern char *
 uid_to_name (uid_t uid)
 {
-  char buf[INT_BUFSIZE_BOUND (intmax_t)];
   struct passwd *pwd = getpwuid (uid);
-  return xstrdup (pwd ? pwd->pw_name
-  : TYPE_SIGNED (uid_t) ? imaxtostr (uid, buf)
-  : umaxtostr (uid, buf));
+  return pwd ? xstrdup (pwd->pw_name) : uid_to_str (uid);
 }
 
 /* Allocate a string representing USER and GROUP.  */
@@ -484,11 +500,21 @@ change_file_owner (FTS *fts, FTSENT *ent,
  : CH_SUCCEEDED);
   char *old_usr = file_stats ? uid_to_name (file_stats->st_uid) : NULL;
   char *old_grp = file_stats ? gid_to_name (file_stats->st_gid) : NULL;
+  char *new_usr = chopt->user_name
+  ? chopt->user_name : uid != -1
+   ? uid_to_str (uid) : NULL;
+  char *new_grp = chopt->group_name
+  ? chopt->group_name : gid != -1
+   ? gid_to_str (gid) : NULL;
   describe_change (file_full_name, ch_status,
old_usr, old_grp,
-   chopt->user_name, chopt->group_name);
+   new_usr, new_grp);
   free (old_usr);
   free (old_grp);
+  if (new_usr != chopt->user_name)
+free (new_usr);
+  if (new_grp != chopt->group_name)
+free (new_grp);
 }
 }
 
-- 
2.26.2



bug#53946: Errors in man pages of GNU_coreutils

2022-02-12 Thread Pádraig Brady

On 12/02/2022 05:57, Helge Kreutzmann wrote:

Dear coreutils maintainer,
the manpage-l10n project maintains a large number of translations of
man pages both from a large variety of sources (including coreutils) as
well for a large variety of target languages.

During their work translators notice different possible issues in the
original (english) man pages. Sometimes this is a straightforward
typo, sometimes a hard to read sentence, sometimes this is a
convention not held up and sometimes we simply do not understand the
original.

We use several distributions as sources and update regularly (at
least every 2 month). This means we are fairly recent (some
distributions like archlinux also update frequently) but might miss
the latest upstream version once in a while, so the error might be
already fixed. We apologize and ask you to close the issue immediately
if this should be the case, but given the huge volume of projects and
the very limited number of volunteers we are not able to double check
each and every issue.

Secondly we translators see the manpages in the neutral po format,
i.e. converted and harmonized, but not the original source (be it man,
groff, xml or other). So we cannot provide a true patch (where
possible), but only an approximation which you need to convert into
your source format.

Finally the issues I'm reporting have accumulated over time and are
not always discovered by me, so sometimes my description of the
problem my be a bit limited - do not hesitate to ask so we can clarify
them.

I'm now reporting the errors for your project. If future reports
should use another channel, please let me know.

Man page: arch.1
Issue:missing markup (B<>)

"uname(1), uname(2)"


I'm not sure about marking up references like this.
There should be enough context from the reference
for the man page reader to display appropriately.
For e.g. I use vim to display man pages, and references
like this are highlighted, and can be followed.
Also looking at other projects, I don't see explicitly
highlighted referenced man pages.


Man page: expand.1
Issue:Missing full stop

"use comma separated list of tab positions  The last specified position can"


Already fixed upstream.


Man page: ls.1
Issue:seems to be an unnecessary split of paragraphs; join with next one

"can be augmented with a B<--sort> option, but any use of B<--sort>=I<\\,none"
"\\/> (B<-U>) disables grouping"
--
Man page: ls.1
Issue:   seems to be an unnecessary split of paragraphs; join with next one

msgid "that points to a directory"
--


Yes good point.
The issue here is those newlines are generated due to inconsistent indentation.
Attached is a patch to ls to make the indentation consistent.


Man page: ls.1
Issue 1:  LS_COLORS → B


There are other env vars mentioned.
I'm not sure why this one, or any should be highlighted.


Issue 2:  dircolors → B(1)


Good point re the (1). Added in second patch.


Man page: rmdir.1
Issue:join this string with the next one "is non-empty"

"ignore each failure that is solely because a directory"


Added in third patch.
I also noticed an erroneous blank line added in the man page
after the first option. I adjusted the column of the description
to 21 to avoid that, and in a further (4th) patch I adjusted
the description column of --help and --version is all commands
to use column 21 also.


Man page: test.1
Issue:[ →  test

"Full documentation Ehttps://www.gnu.org/software/coreutils/[E"


This is subtle. The link above (with [) does actually work.
Though I notice it's not highlighted appropriately by gnome-terminal
at least, so would be better to use "test" instead.
This is done in patch 5.


Man page: unexpand.1
Issue:Missing full stop

"use comma separated list of tab positions  The last specified position can"


Already fixed upstream.

Marking this as done.

cheers,
PádraigFrom 195e943be732d1d1fe5576786596b49be409f1aa Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= 
Date: Sat, 12 Feb 2022 18:43:25 +
Subject: [PATCH 5/5] doc: avoid using "[" is URLS in --help output

* src/system.h (emit_ancillary_info): While supported if entered
  manually, the "[" character is not highlighted as part of a
  URL by default in terminals, so avoid using it.
  Addresses https://bugs.gnu.org/53946
---
 src/system.h | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/src/system.h b/src/system.h
index 08cfd3bc0..09498a172 100644
--- a/src/system.h
+++ b/src/system.h
@@ -716,8 +716,12 @@ emit_ancillary_info (char const *program)
   fputs (_("Report any translation bugs to "
"\n"), stdout);
 }
+  /* .htaccess on the coreutils web site maps programs to the appropriate page,
+ however we explicitly handle "[" -> "test" here as the "[" is not
+ recognized as part of a URL by default in terminals.  */
+  char const *url_program = STREQ (program, "[") ? "test" : program;
   printf (_("Full 

bug#53574: csplit: help document: +/- is not required before OFFSET

2022-01-27 Thread Pádraig Brady

On 27/01/2022 07:28, Jun-T wrote:

'csplit --help' (and 'man csplit') says:
A line OFFSET is a required '+' or '-' followed by a positive integer.

But I think the '+' or '-' is optional, not required.

'info csplit' just says:
The optional OFFSET is an integer

I think this is correct since 'an integer' can be 3, +3 or -3.


Good point. Pushed the following:
https://git.sv.gnu.org/gitweb/?p=coreutils.git;a=commitdiff;h=4a0a8fdbe

Marking this as done.

thanks,
Pádraig





bug#53400: base64: decode Base64URL

2022-01-21 Thread Pádraig Brady

On 20/01/2022 21:31, Sergey Ponomarev wrote:

Package: coreutils

Currently the base64 util can't  parse base64 url safe.
RFC 3548 Base 64 Encoding with URL and Filename Safe Alphabet
https://datatracker.ietf.org/doc/html/rfc3548#page-6

But basically this is the same encoding: just two symbols differ and
no padding. So it's easy and safe just to parse both forms.
The coreutils have a separate command `basenc --base64url` to parse
the b64u but actually there is no any sense to make it as a separate
command.
Having this as a separate command makes it unusable for small Linux
distros like OpenWrt where the device's disk is very small.


Note coreutils can be built like busybox to include multiple commands in a 
single binary



Currently **many** shell scripts uses additional transformation like:

```sh
base64_padding()
{
   local len=$(( ${#1} % 4 ))
   local padded_b64=''
   if [ ${len} = 2 ]; then
 padded_b64="${1}=="
   elif [ ${len} = 3 ]; then
 padded_b64="${1}="
   else
 padded_b64="${1}"
   fi
   echo -n "$padded_b64"
}

base64url_to_b64()
{
   base64_padding "${1}" | tr -- '-_' '+/'
}
```


I'm not sure about base64 accepting _either_ encoding without options.
There would be robustness issues with that.

Also blinding assuming padding may introduce robustness issues with truncated 
data.

It might be useful to have a "--decode-relaxed" option or something
to support decoding any b64 variant and assume no padding,
though preprocessing isn't that onerous, so this is marginal.


But this over complicates everything, reduces performance and is error prone.

I created a similar ticked for OpenSSL base64
https://github.com/openssl/openssl/issues/17559


Also as an additional request: can we add the new param `-D` in upper
case which will be an alias to `-d`.
The issue is that on MacOS the small -d was used for debug and they
used the -D in upper case. This creates a lot of problems for scripts
that should work on both Linux and MacOS. So most people use the long
--decode. But many users just don't know about this interop problem
and once I faced a problem when my college gave me a script that
didn't work for me.
This will prevent unexpected compatibility problems.


macos using -D is unfortunate.
It's surprising they diverged from FreeBSD here.
The fact there is a cross platform --decode option,
suggests we don't need to add -D support,
as it doesn't help folks using -d on other systems.

cheers,
Pádraig





bug#53262: chmod 9.0 failing, where 8.29 succeeds - but no error message

2022-01-15 Thread Pádraig Brady

On 15/01/2022 01:09, Paul Eggert wrote:

Thanks for reporting that. This is a duplicate of bug#50784[1], which
was fixed[2] in September.

Perhaps we should generate a new Coreutils soon, since this bug has been
reported three times now.

[1]: https://bugs.gnu.org/50784
[2]:
https://git.savannah.gnu.org/cgit/coreutils.git/commit/?id=e8b56ebd536e82b15542a00c888109471936bfda


Yes I'm planning a new release in a few weeks.

cheers,
Pádraig






bug#53025: Encouragement to go back to *dis*abled quotation marks in ls output as *default* behavior

2022-01-05 Thread Pádraig Brady

On 05/01/2022 08:44, Joerg M. Sigle wrote:

Dear coreutils developers

Thank you very much for your efferts in trying to provide and maintain
fine tools that help millions or billions of users every day.
I'm one who appreciates that (and I try to do something similar as well).

Still, sometimes there have been changes introduced over the years,
that may have been well intended, but still do more harm than good,
even if not visible to everyone at first glance.

Introduction of quotation marks placed selectively around ls output
containing spaces is an example for that.

I'd like to encourage you to make this behaviour an option again,
and put the default setting back to leaving these quotation marks away.
Whoever wants them, can activate them - and who doesn't, won't be
bothered by an inconsistent and somewhat annoying behavior that disrupts
the visual processing of every single ls output where it becomes apparent -
and requires the addition of an option line for every single existing(!)
.bashrc file on systems where the "new" behaviour is not desired.

And, given the change was introduced in 2016, and re-introduced in 2019,
and annoyance is still caused in 2021 - well, whenever it has been fixed
in existing systems, the problem still turns up in each and every newly
set up system since then, in a delayed manner because naturally ls output
with spaces is not the first thing that will appear, and therefore requires
a lookup of its cause and remedy, and then the manual addition of fixes to
each already existing user account set up until then.


upstream has been consistent in maintaining the behavior since it was 
introduced.



As somebody put it on publicly available pages:

"When this many people consider a thing a bug, then it's a bug whether maintainers 
disagree or not."
Quoted from:

https://unix.stackexchange.com/questions/258679/why-is-ls-suddenly-wrapping-items-with-spaces-in-single-quotes/262162#262162


The bigger problem is, that it's not only ls which is affected like that.
Similar changes, instead, occur in more and more places - in browsers, in
the X11 environment (now even losing it's network transparency etc.), in the
design of GUIs with inconsistant apprance and unrecognizable operating elements
and so on. It's like if somebody has a bad idea, they're not held back by
traditions or established standards or knowledgeable people around them any 
more -
but on the contrary, that bad idea is immediately implemented, distributed, and
then: even copied over and over by other people who think things must constantly
be "renewed" even if the existing version was perfect and simple, and "new" 
usually
means "more complex" and "obviously worse".

Anyway. Thank you very much for consideration of this message,
including its philosophical background, and kind regards!


Reasoning for the default behavior is detailed at:
https://www.gnu.org/software/coreutils/quotes.html

thanks,
Pádraig.





bug#52800: [PATCH] maint: fix prctl arguments

2021-12-26 Thread Pádraig Brady

On 26/12/2021 05:17, Max Filippov wrote:

When configured with --enable-single-binary tools issue incorrect prctl:

   prctl(PR_SET_KEEPCAPS, 1071091381)  = -1 EINVAL (Invalid argument)

PR_SET_MM_ARG_START is not a prctl 'option' parameter, it's 'arg2'
parameter for the option PR_SET_MM. It also has to have 'arg4' and
'arg5' set to 0 explicitly, otherwise the kernel also returns -EINVAL.

* src/coreutils.c (launch_program): Fix prctl arguments.
---
  src/coreutils.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/coreutils.c b/src/coreutils.c
index 6db5a8f05ce3..d8f6e97a6d2f 100644
--- a/src/coreutils.c
+++ b/src/coreutils.c
@@ -121,7 +121,7 @@ launch_program (char const *prog_name, int prog_argc, char 
**prog_argv)
  #if HAVE_PRCTL && defined PR_SET_MM_ARG_START
/* Shift the beginning of the command line to prog_argv[0] (if set) so
   /proc/pid/cmdline reflects the right value.  */
-  prctl (PR_SET_MM_ARG_START, prog_argv[0]);
+  prctl (PR_SET_MM, PR_SET_MM_ARG_START, prog_argv[0], 0, 0);
  #endif
  
exit (prog_main (prog_argc, prog_argv));


Nice one.

This functionality allows programs like ps to attribute
to more specific "utilities", rather than the general "coreutils" binary.

However this is only available with CAP_SYS_RESOURCE or root.
Note CAP_SYS_RESOURCE wouldn't be appropriate to set on the coreutils binary in 
general.
I confirmed your patch is effective for coreutils invoked by root with:

$ src/sleep 5 & { sleep 1; tr '\0' '\n' < /proc/$!/cmdline; }
/home/padraig/git/coreutils/src/coreutils
--coreutils-prog-shebang=sleep
src/sleep
5

# src/sleep 5 & { sleep 1; tr '\0' '\n' < /proc/$!/cmdline; }
sleep
5

I'll apply later (with some adjustments to NEWS)
Marking this as done.

thanks!
Pádraig





bug#52343: Sentry kafka bug

2021-12-08 Thread Pádraig Brady

tag 52343 notabug
close 52343
stop

On 07/12/2021 03:13, 许慧良 wrote:

tail: unrecognized file system type 0x794c7630 for 
‘kafkaServer-gc.log.0.current’. please report this to bug-coreutils@gnu.org. 
reverting to polling

2021-12-07T03:03:51Z [relay_server::endpoints::common] ERROR: error handling 
request: failed to queue envelope
   caused by: Too many envelopes (event_buffer_size reached)


That was fixed nearly 6 years ago in coreutils release 8.25

thanks,
Pádraig





bug#52077: tail: a case where -n is not taken as implicit

2021-11-24 Thread Pádraig Brady

tag 52077 notabug
close 52077
stop

On 24/11/2021 07:38, Shehu Dikko wrote:

Package: coreutils
Version: 9.0

Do please take a look at this example:

% printf '%s\n' just three lines | tee >f1 f2
% tail -n1 f1
lines
% tail -n1 f*
==> f1 <==
lines

==> f2 <==
lines
% tail -1 f1
lines
% tail -1 f*
tail: option used in invalid context -- 1
% tail --version | head -1
tail (GNU coreutils) 9.0

Note that with head and busybox tail, the outputs are unsurprising:

% head -1 f*
==> f1 <==
just

==> f2 <==
just
% head --version | head -1
head (GNU coreutils) 9.0
% busybox tail -1 f*
==> f1 <==
lines

==> f2 <==
lines
%
% uname -v
#1 SMP Debian 4.19.194-3 (2021-07-18)


The compat for that older syntax is not extended to multiple files.
From the info docs:

  For compatibility ‘tail’ also supports an obsolete usage ‘tail
  -[NUM][bcl][f] [FILE]’, which is recognized only if it does not conflict
  with the usage described above.  This obsolete form uses exactly one
  option and at most one file.

cheers,
Pádraig





bug#51857: cross-filesystem copying broken on macOS with coreutils >= 9.0

2021-11-15 Thread Pádraig Brady

On 15/11/2021 18:37, Sudhip Nashi via GNU coreutils Bug Reports wrote:



On Nov 15, 2021, at 11:33, Paul Eggert  wrote:

Is the source file on a ZFS file system by any chance? See my lseek comment 
below.


On 11/15/21 07:48, Cameron Katri via GNU coreutils Bug Reports wrote:

stat64("/tmp/test\0", 0x16DDC36C0, 0x0) = 0 0
fstatat64(0xFFFE, 0x16DDC3C21, 0x16DDC2BA0) = 0 0
fstatat64(0xFFFE, 0x16DDC3C30, 0x16DDC2B10) = 0 0
open("/usr/bin/clear\0", 0x0, 0x0) = 3 0
fstat64(0x3, 0x16DDC2C30, 0x0) = 0 0
open("/tmp/test\0", 0x401, 0x0) = 4 0
fstat64(0x4, 0x16DDC2CC0, 0x0) = 0 0
fstat64(0x4, 0x16DDC2D50, 0x0) = 0 0
fcntl(0x3, 0x32, 0x16DDC3200) = 0 0
fcntl(0x4, 0x32, 0x16DDC2E00) = 0 0
unlink("/private/tmp/test\0", 0x0, 0x0) = 0 0


What's causing this use of "/private/tmp"? I don't see that in the GNU cp 
source code. Can you put a breakpoint on clonefileat and see what's calling it and what 
its arguments are?


clonefileat(0xFFFE, 0x16DDC3200, 0xFFFE) = -1 
Err#18
open("/private/tmp/test\0", 0x601, 0x81ED) = 5 0
close(0x5) = 0 0
open("/private/tmp/test\0", 0x2, 0x0) = 5 0
dup2(0x5, 0x4, 0x0) = 4 0
close(0x5) = 0 0
fchmod(0x4, 0x81ED, 0x0) = 0 0
fchown(0x4, 0x0, 0x0) = 0 0
futimes(0x4, 0x16DDC2DE0, 0x0) = 0 0
sysctl([CTL_HW, 7, 0, 0, 0, 0] (2), 0x207EC4068, 0x16DDC2A30, 0x0, 0x0) 
= 0 0
lseek(0x3, 0x0, 0x4) = -1 Err#6


That lseek call looks like OpenZFS bug 11900 
. If you're using ZFS, the bug really 
should be fixed in your ZFS implementation as it can affect programs other than coreutils 
and there's no easy workaround (other than to disable efficient copying). Is this something 
you can look into, or ask someone with macOS and/or ZFS expertise to look into? For more, 
see .


ftruncate(0x4, 0x1D770, 0x0) = 0 0
close(0x4) = 0 0
close(0x3) = 0 0


Turns out lseek is broken (or at least works differently) on macOS as well 
(https://lists.gnu.org/archive/html/bug-gnulib/2018-09/msg00054.html). Funny 
coincidence! I’ll take a better look later this week if I can and try to see 
what the exact problem is.


I saw on other report of failure on macOS.
I think we should disable the SEEK_DATA optimization there for now.
The attached does that.

I'll apply that later.
I also have access to a macOS system, so I'll also test out
if there are ways to use SEEK_DATA there.

thanks,
Pádraig
From e82670759e7eea3fb9faec99fcd94ad9f2fe03b6 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= 
Date: Mon, 15 Nov 2021 23:15:07 +
Subject: [PATCH] copy: avoid SEEK_DATA on macOS

* src/copy.c (infer_scantype): Don't probe for SEEK_DATA support
on __APPLE__ systems, as SEEK_DATA was seen to return ENXIO
inappropriately there.
* init.cfg (seek_data_capable_): Return failure on Darwin systems.
Addresses https://bugs.gnu.org/51857
---
 init.cfg   | 6 ++
 src/copy.c | 2 +-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/init.cfg b/init.cfg
index b92f717f5..2ab607cf3 100644
--- a/init.cfg
+++ b/init.cfg
@@ -533,6 +533,12 @@ require_kill_group_()
 # which SEEK_DATA support exists.
 seek_data_capable_()
 {
+  # The SEEK_DATA implementation on macOS is not supported
+  if test "$(uname)" = Darwin; then
+warn_ 'seek_data_capable_: Darwin detected: assuming not SEEK_DATA capable'
+return 1
+  fi
+
   { python3 < /dev/null && PYTHON_=python3; } ||
   { python  < /dev/null && PYTHON_=python; }
 
diff --git a/src/copy.c b/src/copy.c
index f88bf3ed3..ecb37a02c 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -1100,7 +1100,7 @@ infer_scantype (int fd, struct stat const *sb,
  && ST_NBLOCKS (*sb) < sb->st_size / ST_NBLOCKSIZE))
 return PLAIN_SCANTYPE;
 
-#ifdef SEEK_HOLE
+#if defined SEEK_HOLE && !defined __APPLE__
   scan_inference->ext_start = lseek (fd, 0, SEEK_DATA);
   if (0 <= scan_inference->ext_start || errno == ENXIO)
 return LSEEK_SCANTYPE;
-- 
2.26.2



bug#51857: cross-filesystem copying broken on macOS with coreutils >= 9.0

2021-11-15 Thread Pádraig Brady

On 15/11/2021 04:02, Sudhip Nashi via GNU coreutils Bug Reports wrote:

Hello,

Cross-filesystem copying seems to have been broken in the latest coreutils 
release on macOS. Running a command like ‘cp /usr/bin/clear /tmp/test’ appears 
to return successfully, but if one analyzes /tmp/test, it’s filled with NULL 
characters. However, copying works fine when the source and destination file 
are on the same filesystem. Do you know what might be causing this?

Thanks in advance,
Sudhip Nashi




What are the source and dest file system types?
Could you send the output of `sudo dtruss cp /usr/bin/clear /tmp/test`?
I suspect SEEK_DATA may have issues on nacOS,
as usage of that is new in coreutils 9.0.

thanks,
Pádraig





bug#51793: FAIL: tests/misc/env-signal-handler

2021-11-13 Thread Pádraig Brady

On 13/11/2021 09:46, Andreas Schwab wrote:

On Nov 12 2021, Pádraig Brady wrote:


I've never seen this, and I can't see the race.


There is an obvious race: if env needs more than .1 seconds to set the
SIGINT handler.


Indeed.

That fits the pattern where we need a certain delay to pass.
So we can use our retry_delay_ helper here.
I'll apply the attached later.

thanks,
Pádraig
From dd460b0a7ac029a1809ca2ef363701b431e613be Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= 
Date: Sat, 13 Nov 2021 12:15:17 +
Subject: [PATCH] tests: avoid rare false failure in env-signal-handler.sh

* tests/misc/env-signal-handler.sh: Use retry_delay_ to
avoid a false failure under load, where env hasn't setup
the SIGINT handling before timeout(1) sends the SIGINT.
Fixes https://bugs.gnu.org/51793
---
 tests/misc/env-signal-handler.sh | 64 +---
 1 file changed, 26 insertions(+), 38 deletions(-)

diff --git a/tests/misc/env-signal-handler.sh b/tests/misc/env-signal-handler.sh
index aa6ec8dac..c4179fe9e 100755
--- a/tests/misc/env-signal-handler.sh
+++ b/tests/misc/env-signal-handler.sh
@@ -82,62 +82,50 @@ compare /dev/null err4 || fail=1
 # env test - block signal handler
 env --block-signal true || fail=1
 
+env_ignore_delay_()
+{
+  local delay="$1"
+
+  # The first 'env' is just to ensure timeout is not a shell built-in.
+  env timeout --verbose --kill-after=.1 --signal=INT $delay \
+env $env_opt \
+sleep 10 > /dev/null 2>outt
+  # check only the first two lines from stderr, which are printed by timeout.
+  # (operating systems might add more messages, like "killed").
+  sed -n '1,2p' outt > out || framework_failure_
+  compare exp out
+}
+
 # Baseline test - ignore signal handler
 # -
-# Kill 'sleep' after 1 second with SIGINT - it should terminate (as SIGINT's
-# default action is to terminate a program).
-# (The first 'env' is just to ensure timeout is not the shell's built-in.)
-env timeout --verbose --kill-after=.1 --signal=INT .1 \
-sleep 10 > /dev/null 2>err5
-
-printf "timeout: sending signal INT to command 'sleep'\n" > exp-err5 \
-|| framework_failure_
-
-compare exp-err5 err5 || fail=1
-
+# Kill 'sleep' after 1 second with SIGINT - it should terminate
+# (as SIGINT's default action is to terminate a program).
+cat <<\EOF >exp || framework_failure_
+timeout: sending signal INT to command 'env'
+EOF
+env_opt='' retry_delay_ env_ignore_delay_ .1 6 || fail=1
 
 # env test - ignore signal handler
 # 
-# Use env to silence (ignore) SIGINT - "seq" should continue running
+# Use env to silence (ignore) SIGINT - "sleep" should continue running
 # after timeout sends SIGINT, and be killed after 1 second using SIGKILL.
-
-cat>exp-err6 <exp || framework_failure_
 timeout: sending signal INT to command 'env'
 timeout: sending signal KILL to command 'env'
 EOF
-
-env timeout --verbose --kill-after=.1 --signal=INT .1 \
-env --ignore-signal=INT \
-sleep 10 > /dev/null 2>err6t
-
-# check only the first two lines from stderr, which are printed by timeout.
-# (operating systems might add more messages, like "killed").
-sed -n '1,2p' err6t > err6 || framework_failure_
-
-compare exp-err6 err6 || fail=1
-
+env_opt='--ignore-signal=INT' retry_delay_ env_ignore_delay_ .1 6 || fail=1
 
 # env test - ignore signal handler (2)
 # 
-# Repeat the previous test with "--ignore-signals" and no signal names,
+# Repeat the previous test with "--ignore-signal" and no signal names,
 # i.e., all signals.
-
-env timeout --verbose --kill-after=.1 --signal=INT .1 \
-env --ignore-signal \
-sleep 10 > /dev/null 2>err7t
-
-# check only the first two lines from stderr, which are printed by timeout.
-# (operating systems might add more messages, like "killed").
-sed -n '1,2p' err7t > err7 || framework_failure_
-
-compare exp-err6 err7 || fail=1
-
+env_opt='--ignore-signal' retry_delay_ env_ignore_delay_ .1 6 || fail=1
 
 # env test --list-signal-handling
 env --default-signal --ignore-signal=INT --list-signal-handling true \
   2> err8t || fail=1
 sed 's/(.*)/()/' err8t > err8 || framework_failure_
-env printf 'INT(): IGNORE\n' > exp-err8
+env printf 'INT(): IGNORE\n' > exp-err8 || framework_failure_
 compare exp-err8 err8 || fail=1
 
 
-- 
2.26.2



bug#51793: FAIL: tests/misc/env-signal-handler

2021-11-12 Thread Pádraig Brady

On 12/11/2021 19:25, Andreas Schwab wrote:

--- exp-err6   2021-11-11 22:58:04.360716802 +
+++ err6   2021-11-11 22:58:04.752716821 +
@@ -1,2 +1 @@
  timeout: sending signal INT to command 'env'
-timeout: sending signal KILL to command 'env'


The above is for the previous command not the following.


./tests/misc/env-signal-handler.sh: line 127: 26396 Killed  env 
timeout --verbose --kill-after=.1 --signal=INT .1 env --ignore-signal sleep 10 > 
/dev/null 2> err7t
FAIL tests/misc/env-signal-handler.sh (exit status: 1)


Could you paste a bit more context from the previous command.
Specifically I'm wondering if the "Killed" message was displayed
(i.e. if the SIGKILL was actually sent).

I presume this is a race that you're seeing rarely.
I've never seen this, and I can't see the race.

What might be happening is the "sending signal KILL..."
message sent to stderr, is in a buffer somewhere,
which is dropped upon processing of SIGKILL.

What file system, kernel, shell are you using.

cheers,
Pádraig






bug#51792: coreutils - csplit - feature request

2021-11-12 Thread Pádraig Brady

On 12/11/2021 17:05, Rodolfo Aramayo wrote:

Dear Coreutils Maintainers,

First, thank you for your work. I use coreutils daily both for my research
and teaching. It is a great set of tools.

Second, I recently needed to extract Coding Sequences information from a
GenBank file. GenBank files are used in Computational
Genomics/Bioinformatics extensively. I used csplit, and it works like a
charm.

The command I used is:

csplit -sz -n 5 --prefix=02_ 01_1
/[[:space:]][[:space:]][[:space:]][[:space:]][[:space:]]CDS[[:space:]][[:space:]][[:space:]][[:space:]][[:space:]][[:space:]][[:space:]][[:space:]][[:space:]][[:space:]][[:space:]][[:space:]][[:space:]]/
{*};

I was unable to declare: "[[:space:]]\+" as I expected for POSIX aware code.

My question is: Is csplit POSIX compatible? and if it is not, can we make
it POSIX compatible?



Well POSIX defines BRE and ERE, with csplit supporting the former.
From the code we have:

  re_syntax_options =
RE_SYNTAX_POSIX_BASIC & ~RE_CONTEXT_INVALID_DUP & ~RE_NO_EMPTY_RANGES;

Generally one can replace '+' functionality from ERE, with '\{1,\}' in BRE.
So you'd be using something like:

  [[:space:]]\{1,\}CDS[[:space:]]\{1,\}

We might add an option to use ERE, though there isn't a big need
for that I think for csplit use cases.

cheers,
Pádraig





bug#51433: cp 9.0 sometimes fails with SEEK_DATA/SEEK_HOLE

2021-11-03 Thread Pádraig Brady

On 27/10/2021 11:00, Janne Heß wrote:

Hi everyone,

I packaged coreutils 9.0 for NixOS and we found breakages that seemed to be 
very random during builds of packages
that use the updated coreutils in their build process. It's really hard to tell 
the main cause but it seems like the issues
are caused by binaries that are corrupted after cp copied them from /tmp to 
/nix. The issue arises both when the
directories are on the same filesystem and when /tmp is on tmpfs.
Upon further inspection/bisection we figured out these issues are caused by 
a6eaee501f6ec0c152abe88640203a64c390993e.
This seems to happen on ZFS and indeed on the main coreutils mailing list there 
is a ZFS issue linked [1].
The testsuite was patched in 61c81ffaacb0194dec31297bc1aa51be72315858 so it 
doesn't detect this issue anymore,
but the issue still very much happens in the real world.

We have found this to happen while building the completions for a go tool (jx) 
which seems to be the same
issue as [2]. The tool is built, copied using cp, and called which causes a 
segfault to happen.

Building another package (peertube) on x86_64-linux on ext4 also fails with 
strange errors in the
test suite, something about "Error: The service is no longer running". This 
does not happen when the mentioned
coreutils commit is undone by replacing #ifdef with #if 0 [3].

We have also seen this issue on Darwin when building Alacritty but only 
happening on some machines
but we were not able to pin it down any further there so this might be related 
or it might not.

Since the issue is so random, we started wondering if it might be related to 
-frandom-seed which changes in NixOS
when rebuilding a package [4]. A thing to note here is that Nix does a lot of 
sandboxing stuff during builds which
includes mount namespaces so a Kernel bug is not out of the question. All of 
these issues happened during Nix builds,
coreutils 9.0 never made it out of the NixOS staging environment due to the 
builds breaking. We will probably disable
the new code paths as outlined above so the issue is contained for NixOS users 
and does not hit any production environments.

[1]: https://github.com/openzfs/zfs/issues/11900
[2]: https://github.com/golang/go/issues/48636
[3]: 
https://raw.githubusercontent.com/NixOS/nixpkgs/bf0531b4f8a2de4ff2700797fb211a90c951786e/pkgs/tools/misc/coreutils/disable-seek-hole.patch
[4]: https://github.com/NixOS/nixpkgs/pull/141684#issuecomment-952339263


Looks like there is a WIP fix for OpenZFS mentioned at [1],
where mmap'd regions were not being flushed:
https://github.com/openzfs/zfs/commit/f2eebe07

So this should unblock enabling coreutils 9 at some stage at least.
I've asked at [1] now they know what's going on,
how programs might best distinguish buggy instances of openzfs.

cheers,
Pádraig





bug#51569: tail -f reports unrecognized file system type

2021-11-02 Thread Pádraig Brady

tag 51569 notabug
close 51569
stop

On 02/11/2021 16:01, Tom Barringer wrote:

Received this error:
tail: unrecognized file system type 0x53464846 for ‘ads0.exp’. please
report this to bug-coreutils@gnu.org. reverting to polling
Environment: Win10 with all updates
Using Windows Subsystem for Linux (WSL)
Target file is ASCII text, with CRLF, LF line terminators
Error is repeatable.

[image: image.png]
Drive C:
[image: image.png]



That was addressed in coreutils-8.26
(so updating wsl or coreutils therein should fix that).

cheers,
Pádraig





bug#51433: cp 9.0 sometimes fails with SEEK_DATA/SEEK_HOLE

2021-11-01 Thread Pádraig Brady

On 01/11/2021 05:52, Paul Eggert wrote:

On 10/31/21 09:13, Pádraig Brady wrote:


The attached uses statfs()->f_type which is usually available,
to avoid using SEEK_DATA on ZFS.  This should be fairly lightweight I
think,
and only used for files that might be sparse.


Couldn't we be even lazier about invoking fstatfs? It needs to be
invoked only if the file appears to be sparse (as you mentioned), and
also only if lseek+SEEK_DATA fails (with errno==ENXIO) at a position
less than the file's length, and also at most once per input file
descriptor.

When copying multiple files (e.g., cp -r) we could also cache the
previous file's st_dev and ZFS status, so in the typical case we could
skip fstatfs entirely except for the first sparsish file that satisfies
the abovementioned criteria.


I was thinking that sparse may be enough of a filter,
but these are valid suggestions.


Also, as I understand it the problem occurs with OpenZFS but not on
Solaris or its descendants, so we don't need to do any of this if __sun
is defined.


Oh interesting.
If that was actually the case we could probably default
to allowing SEEK_DATA, and only disallow when f_type == ZFS,
i.e. allow SEEK_DATA on solaris.


Also, if we're going to use fstatfs it'd probably be better to hoist all
the fstatfs portability mess out of stat.c and into a new file (say,
gl/lib/pstatfs.c) that gives us a portable way of getting statfs-like
data out of filesystems, and using that new file in both stat.c and copy.c.


.. and tail.c. Good suggestion.


One more idea: I've seen reports that the problem goes away if you use
'read' to read just the first byte of the false hole; that might be a
simpler workaround than getting into the fstatfs portability mess.


Indeed. It would be really nice to get a solution supporting
existing, and future openzfs installations.


We could also disable SEEK_DATA for remote file systems,
but that would be a pity since SEEK_DATA seems especially useful there.


Absolutely.

Don't know if you're following
<https://github.com/openzfs/zfs/issues/11900> but rincebrain reports a
working workaround on the OpenZFS side tho more testing needs to be
done. This OpenZFS bugs affects GNU tar and star (and surely other
programs) as well as coreutils, and we may be better off overall if we
merely ask people to fix their ZFS implementation rather than our trying
to work around the bug in coreutils.


I see a slew of recent messages re workarounds in openzfs.
It's good to see some movement there, as it's definitely
best to address this in zfs.

cheers,
Pádraig





bug#51011: [PATCH] sort: --debug: add warnings about radix and grouping chars

2021-10-31 Thread Pádraig Brady

On 11/10/2021 02:54, Pádraig Brady wrote:

On 11/10/2021 00:34, Paul Eggert wrote:

The warnings look good, except that this one:


     $ printf '1.0\n0.9\n' | sort -s -k1,1g --debug
     sort: numbers use ‘.’ as a decimal point in this locale
     0.9
     ___
     1.0
     ___


seems overkill if we're in the C locale.

Also, shouldn't similar diagnostics be generated if the field separator
is '-', or '+', or a digit in the current locale?


Yes this is more informational than a warning.
As Bernhard mentioned it may be useful to tag
--debug messages as informational or warnings.

In this case it would be info:
but would change to warn: if (tab == decimal_point).

The reason for the info message is that it may not
be obvious to users that numeric comparison
depends on locale just like text,
and we already provide the informational
text comparison message indicating the current locale.
We would only show this info: message if doing numeric sorting.


Addressing your '+' and '-' comment.
Yes they may also be used as field separators and
so are worth explicitly warning about.

Re warning about using digits in --field-separator,
that would be extremely edge case, and anyway
the --debug key marking should make it apparent
the extent of the numbers being compared.
The same argument can be made for other characters possible in numbers like;
  1E+4 nan, Infinity, 0xabcde.fp-3, etc.

As a related issue, I also thought it appropriate to warn
when we're ignoring multi-byte grouping chars in the locale.

The new warnings in this update are:

  $ LC_ALL=fr_FR.utf8 sort -n --debug /dev/null
  sort: the multi-byte number group separator in this locale is not supported

  $ sort --debug -t- -k1n /dev/null
  sort: key 1 is numeric and spans multiple fields
  sort: field separator ‘-’ is treated as a minus sign in numbers

  $ sort --debug -t+ -k1g /dev/null
  sort: key 1 is numeric and spans multiple fields
  sort: field separator ‘+’ is treated as a plus sign in numbers

I'll apply this later.

cheers,
Pádraig
>From 9e89c5f3e9c5652df2cfbf46b3e1f9608cb0092f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= 
Date: Sun, 10 Oct 2021 18:35:59 +0100
Subject: [PATCH] sort: --debug: add warnings about sign, radix, and grouping
 chars
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

New warnings are added related to the handling
of thousands grouping characters, and decimal points.
Examples now diagnosed are:

$ printf '0,9\n1,a\n' | sort -nk1 --debug -t, -s
sort: key 1 is numeric and spans multiple fields
sort: field separator ‘,’ is treated as a group separator in numbers
1,a
_
0,9
___

$ printf '1,a\n0,9\n' | LC_ALL=fr_FR.utf8 sort -gk1 --debug -t, -s
sort: key 1 is numeric and spans multiple fields
sort: field separator ‘,’ is treated as a decimal point in numbers
0,9
___
1,a
__

$ printf '1.0\n0.9\n' | LC_ALL=fr_FR.utf8 sort -s -k1,1g --debug
sort: note numbers use ‘,’ as a decimal point in this locale
0.9
_
1.0
_

$ LC_ALL=fr_FR.utf8 sort -n --debug /dev/null
sort: text ordering performed using ‘fr_FR.utf8’ sorting rules
sort: note numbers use ‘,’ as a decimal point in this locale
sort: the multi-byte number group separator in this locale \
  is not supported

$ sort --debug -t- -k1n /dev/null
sort: key 1 is numeric and spans multiple fields
sort: field separator ‘-’ is treated as a minus sign in numbers
sort: note numbers use ‘.’ as a decimal point in this locale

$ sort --debug -t+ -k1g /dev/null
sort: key 1 is numeric and spans multiple fields
sort: field separator ‘+’ is treated as a plus sign in numbers
sort: note numbers use ‘.’ as a decimal point in this locale

* src/sort.c (key_warnings): Add the warnings above.
* tests/misc/sort-debug-warn.sh: Add test cases.
* NEWS: Mention the improvement.
Addresses https://bugs.gnu.org/51011
---
 NEWS  |  7 ++-
 src/sort.c| 90 ++-
 tests/misc/sort-debug-warn.sh | 51 +++-
 3 files changed, 144 insertions(+), 4 deletions(-)

diff --git a/NEWS b/NEWS
index 086da03ae..1097f0c32 100644
--- a/NEWS
+++ b/NEWS
@@ -11,10 +11,15 @@ GNU coreutils NEWS-*- outline -*-
 ** Changes in behavior
 
   timeout --foreground --kill-after=... will now exit with status 137
-  if the kill signal was sent, which is consistent with the behaviour
+  if the kill signal was sent, which is consistent with the behavior
   when the --foreground option is not specified.  This allows users to
   distinguish if the command was more forcefully terminated.
 
+** Improvements
+
+  sort --debug now diagnoses issues with --field-separator characters
+  that conflict with characters possibly used in numbers.
+
 
 * Noteworthy changes in release 9.0 (2021-09-24) [stable]
 
diff --git a/src/sort.c b/src/sort.c
index d32dcfb02..c75281661 100644
--- a/src/sort.c
+++ b/src/sort.c
@@ -156,6 +156,8 @@ static char decimal_po

bug#51433: cp 9.0 sometimes fails with SEEK_DATA/SEEK_HOLE

2021-10-31 Thread Pádraig Brady

On 30/10/2021 03:04, Paul Eggert wrote:

On 10/28/21 13:59, Pádraig Brady wrote:


I wonder after getting a SEEK_DATA ENXIO, might we correlate
we really are at end of file by checking SEEK_HOLE also returns ENXIO?


Wouldn't SEEK_HOLE return the current offset, instead of ENXIO? That is,
if the underlying data structure is wrong and claims that the rest of
the file is a hole, wouldn't SEEK_HOLE merely repeat that information?


What I was thinking is that SEEK_DATA returning ENXIO suggests end of file.
If we're not actually at EOF a SEEK_HOLE at SEEK_CUR+1 might not indicate ENXIO.
I which case we could do a fdatasync().
All wild speculation though depending on the ZFS bug.
If the bug is fixed and we have a good reproducer, we might
be able to do something like this to cater for existing buggy ZFS installations.

cheers,
Pádraig





bug#51433: cp 9.0 sometimes fails with SEEK_DATA/SEEK_HOLE

2021-10-31 Thread Pádraig Brady

On 28/10/2021 20:11, Paul Eggert wrote:

On 10/28/21 06:54, Pádraig Brady wrote:


Further debugging from Nix folks suggest ZFS was in consideration always,
as invalid artifacts were written to a central cache from ZFS backed hosts.
So we should at least change the comment in the patch to only mention ZFS.


Yes, that sounds reasonable.

This ZFS bug sounds pretty serious, though. Apparently it affects star
and other programs too. I'm not sure we should attempt to work around it
in coreutils, if the workarounds penalize everybody not using ZFS.


Yes this is an awkward situation.
Though given the frequency of the cp/zfs combo,
I think we should try to provide some mitigation.


Is it cheap to check whether a file is actually in a ZFS filesystem?
(Don't know how this'd work with loopback mounts, NFS, etc.) If so, it
might be better to simply fdatasync (or even fsync) every input file
that's on ZFS, until we know the ZFS bugs are fixed.


The attached uses statfs()->f_type which is usually available,
to avoid using SEEK_DATA on ZFS.  This should be fairly lightweight I think,
and only used for files that might be sparse.

There may still be issues with NFS backed by ZFS,
but that should be rarer and more centrally managed,
thus more amenable to possible future ZFS fixes, or mitigations.
We could also disable SEEK_DATA for remote file systems,
but that would be a pity since SEEK_DATA seems especially useful there.

cheers,
Pádraig
>From b7b4926b2f52cfa2ab7d33742355beaf9a08c695 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= 
Date: Sun, 31 Oct 2021 15:38:29 +
Subject: [PATCH] copy: avoid SEEK_DATA corruption on ZFS

Avoid corruption when using SEEK_DATA on ZFS, as discussed in:
https://github.com/openzfs/zfs/issues/11900

* src/copy.c (functional_seek_data): A new function that
returns true when we're sure we're not copying from ZFS.
Note systems like solaris which doesn't have statfs()->f_type
available, will be assumed to not have a usable SEEK_DATA.
Most systems in use should have this info available.
(infer_scantype): After a file is determined to perhaps be sparse,
call functional_seek_data to ensure it's not on ZFS.
* init.cfg (seek_data_capable_): Skip on ZFS.
* NEWS: Mention the bug avoidance.
Addresses https://bugs.gnu.org/51433
---
 NEWS   |  4 
 init.cfg   | 12 
 src/copy.c | 36 +++-
 3 files changed, 51 insertions(+), 1 deletion(-)

diff --git a/NEWS b/NEWS
index 086da03ae..78a73ab05 100644
--- a/NEWS
+++ b/NEWS
@@ -4,6 +4,10 @@ GNU coreutils NEWS-*- outline -*-
 
 ** Bug fixes
 
+  cp will avoid corruption bugs in the ZFS file system by avoiding use
+  of lseek(..., SEEK_DATA), thus using potentially slower hole detection.
+  [bug triggered in coreutils-9.0]
+
   chmod -R no longer exits with error status when encountering symlinks.
   All files would be processed correctly, but the exit status was incorrect.
   [bug introduced in coreutils-9.0]
diff --git a/init.cfg b/init.cfg
index b92f717f5..0f97a6712 100644
--- a/init.cfg
+++ b/init.cfg
@@ -541,6 +541,18 @@ seek_data_capable_()
   return 1
   fi
 
+  # Skip on zfs due to various SEEK_DATA issues in its implementation
+  fstype=$(stat -f -c%t "$@")  # Ensure we have f_type, as that's what copy uses
+  if test -z "$fstype" || test "$fstype" = '?'; then
+warn_ 'seek_data_capable_: stat(1) failed: assuming not SEEK_DATA capable'
+return 1
+  fi
+  fsname=$(stat -f -c%T "$@")
+  if test "$fsname" = 'zfs'; then
+warn_ 'seek_data_capable_: zfs detected: SEEK_DATA is disabled'
+return 1
+  fi
+
   # Use timeout if available to skip cases where SEEK_DATA takes a long time.
   # We saw FreeBSD 9.1 take 35s to return from SEEK_DATA for a 1TiB empty file.
   # Note lseek() is uninterruptible on FreeBSD 9.1, but it does eventually
diff --git a/src/copy.c b/src/copy.c
index a6523ed97..6e19b8740 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -1080,6 +1080,33 @@ union scan_inference
   off_t ext_start;
 };
 
+/* SEEK_DATA on ZFS has many issues as described at:
+   https://github.com/openzfs/zfs/issues/11900
+   so return FALSE for ZFS (or inability to detect fs type).  */
+
+#ifdef SEEK_HOLE
+# include "fs.h"
+# if HAVE_SYS_STATFS_H
+#  include 
+# elif HAVE_SYS_VFS_H
+#  include 
+# endif
+static bool
+functional_seek_data (int fd)
+{
+# if HAVE_FSTATFS && HAVE_STRUCT_STATFS_F_TYPE
+  struct statfs buf;
+  if (fstatfs (fd, ) == 0)
+{
+  if (buf.f_type != S_MAGIC_ZFS)
+return true;
+}
+# endif
+
+  return false;
+}
+#endif
+
 /* Return how to scan a file with descriptor FD and stat buffer SB.
Store any information gathered into *SCAN.  */
 static enum scantype
@@ -1092,7 +1119,14 @@ infer_scantype (int fd, struct stat const *sb,
 return PLAIN_SCANTYPE;
 
 #ifdef SEEK_HOLE
-  scan_inference->ext_start = lseek (fd, 

bug#51433: cp 9.0 sometimes fails with SEEK_DATA/SEEK_HOLE

2021-10-30 Thread Pádraig Brady

On 30/10/2021 02:24, Paul Eggert wrote:

On 10/28/21 12:11, Paul Eggert wrote:

Wait - lseek returns a number less than -1?! We could easily check for
that FreeBSD bug, perhaps as an independent patch; this shouldn't
require any extra syscalls.


I installed the attached patch to do this. This doesn't fix coreutils
bug#51433; it merely makes 'cp' and similar programs more likely to
detect and report the FreeBSD 9.1 bug you mentioned.


On further inspection it seems the bug is in truss :/
I.e. the only bug on FreeBSD 9.1 is that the SEEK_DATA takes ages,
but the returned values are correct.
I.e. SEEK_DATA was returning 1099511595008 in this case
which truss was reporting in 32 bit rather than 64 bit.
I see other inconsistencies in truss output,
so it can only be relied on for what syscall was called,
rather than what was passed/returned.

Given this patch doesn't change any operation,
it may be best to revert to avoid the complexity?

cheers,
Pádraig





bug#51345:

2021-10-29 Thread Pádraig Brady

On 28/10/2021 22:59, Sworddragon wrote:

Despite I'm not using Linux as main system anymore and wanted to avoid
getting into too much work I found some time to do some tests as this issue
bugs me just too much.


You could try running the following immediately after,
to see if it also returns quickly:

   blockdev --flushbufs /dev/sdb


Yes, this command also blocks for a bit over 1 minute when this issue
occurs.


Right, so that suggests conv=fsync on dd was ineffective.


Here is the output (I had to freely translate the strings since
this Knoppix instance is only in german so they might be slightly
inaccurate; Also I had to type all text since it was executed on a
different system but carefully checked to not introduce any typos):

root@Microknoppix:~# dd if=/dev/random of=/dev/sdb bs=1M conv=fsync
status=progress
1039138816 Bytes(1,0 GB, 991 MiB) copied, 56 s, 18,5 MB/s
dd: Error on writing '/dev/sdb': The device has not enough free space
999+0 records in
998+0 records out


Ah right. What's probably happening is that dd is not doing the fsync
because it's exiting early because of the ENOSPC from write(2).

To avoid needing the buffer drain (with fsync), you can use





bug#51345:

2021-10-29 Thread Pádraig Brady

On 28/10/2021 22:59, Sworddragon wrote:

Despite I'm not using Linux as main system anymore and wanted to avoid
getting into too much work I found some time to do some tests as this issue
bugs me just too much.


You could try running the following immediately after,
to see if it also returns quickly:

   blockdev --flushbufs /dev/sdb


Yes, this command also blocks for a bit over 1 minute when this issue
occurs.


Right that suggests the conv=fsync to dd was ineffective


Here is the output (I had to freely translate the strings since
this Knoppix instance is only in german so they might be slightly
inaccurate; Also I had to type all text since it was executed on a
different system but carefully checked to not introduce any typos):

root@Microknoppix:~# dd if=/dev/random of=/dev/sdb bs=1M conv=fsync
status=progress
1039138816 Bytes(1,0 GB, 991 MiB) copied, 56 s, 18,5 MB/s
dd: Error on writing '/dev/sdb': The device has not enough free space
999+0 records in
998+0 records out


Ah right. What's happening is dd is not doing the fsync()
as it's exiting early due to write(2) getting ENOSPC.

As you've seen you can avoid the need for fsync()
to flush buffers with oflag=direct.
A reason that might be faster also is that depending on your free mem,
you would avoid churning the kernel caches.

Another way to at least ensure the conv=fsync was effective,
would be to not write too much.  I.e. you could determine
the exact size of the disk (with `blockdev --getsize64 /dev/sdb` for e.g.)
and then use an appropriate bs= and count=. That's awkward though
and difficult to do with good performance due to larger block sizes
not generally aligning with the device size.

So this is a gotcha that should at least be documented.
Though I'm leaning towards improving this by always
doing an fsync on exit if we get a read or write error
and have successfully written any data, so that
previously written data is sync'd as requested.

cheers,
Pádraig





bug#51433: cp 9.0 sometimes fails with SEEK_DATA/SEEK_HOLE

2021-10-28 Thread Pádraig Brady

On 28/10/2021 20:11, Paul Eggert wrote:

On 10/28/21 06:54, Pádraig Brady wrote:


Further debugging from Nix folks suggest ZFS was in consideration always,
as invalid artifacts were written to a central cache from ZFS backed hosts.
So we should at least change the comment in the patch to only mention ZFS.


Yes, that sounds reasonable.

This ZFS bug sounds pretty serious, though. Apparently it affects star
and other programs too. I'm not sure we should attempt to work around it
in coreutils, if the workarounds penalize everybody not using ZFS.

Is it cheap to check whether a file is actually in a ZFS filesystem?
(Don't know how this'd work with loopback mounts, NFS, etc.) If so, it
might be better to simply fdatasync (or even fsync) every input file
that's on ZFS, until we know the ZFS bugs are fixed.

In theory we could fdatasync/fsync every input file on every platform.
It'd be a shame to do that, though; that would slow down everybody
merely to work around this ZFS bug.


Also it seems like fsync() does avoid the ZFS issue as mentioned in:
https://github.com/openzfs/zfs/issues/11900


Yes. I'm hoping that fdatasync suffices as it's lighter-weight. But if
fsync is needed we can use fsync.


BTW I'm slightly worried about retrying SEEK_DATA as
FreeBSD 9.1 has a bug with large sparse files at least
where it takes ages for SEEK_DATA to return:
    36.13290615 lseek(3,0x0,SEEK_DATA) = -32768 (0x8000)
If ENXIO is not set in that case, then there is no issue.


Wait - lseek returns a number less than -1?! We could easily check for
that FreeBSD bug, perhaps as an independent patch; this shouldn't
require any extra syscalls.

Also please see
<https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=256205>. It appears
that ZFS has significant bugs in this area on FreeBSD, bugs that haven't
been fixed yet. That bug report does suggest that an fsync (and I hope
fdatasync) works around the bugs.


Also I'm not sure restricting sync to ENXIO is general enough,
as an strace from a problematic cp, from the github issue above is:
    lseek(3, 0, SEEK_DATA)    = 0
    lseek(3, 0, SEEK_HOLE)    = 131072
    lseek(3, 0, SEEK_SET) = 0
    read(3, "\177ELF\2\1"..., 131072) = 131072
    write(4, "\177ELF\2\"..., 131072) = 131072
    lseek(3, 131072, SEEK_DATA)   = -1 ENXIO
    ftruncate(4, 3318813) = 0


How about if we also do an fdatasync+retry after that 2nd lseek yields
ENXIO? Would that suffice to work around the ZFS bug? Would it be too
much of a performance penalty for non-ZFS users?


I don't think there is anything special about first or second lseek().
ZFS just seems to be returning ENXIO depending on the write pattern of the file.
Since we currently always have an expected SEEK_DATA ENXIO at end of file,
doing a sync on any ENXIO would be best avoided.

I wonder after getting a SEEK_DATA ENXIO, might we correlate
we really are at end of file by checking SEEK_HOLE also returns ENXIO?
If not we could do the datasync and try SEEK_DATA again.
I've not seen enough syscall traces to be confident of that though.

BTW we only attempt the SEEK_HOLE scanning when our heuristic
of st_blocks being less than st_size indicates there may be holes.
It's a bit surprising that that is the case for these elf binaries.
Perhaps zfs is updating st_blocks async, or perhaps there are
runs of zeros in these files that a linker or whatever is sparsifying?

cheers,
Pádraig





bug#51433: cp 9.0 sometimes fails with SEEK_DATA/SEEK_HOLE

2021-10-28 Thread Pádraig Brady

On 28/10/2021 08:56, Paul Eggert wrote:

On 10/27/21 03:00, Janne Heß wrote:

Building another package (peertube) on x86_64-linux on ext4 also fails with 
strange errors in the
test suite, something about "Error: The service is no longer running". This 
does not happen when the mentioned
coreutils commit is undone by replacing #ifdef with #if 0 [3].


So the problem is not limited to ZFS? Which means that even if we
implemented Pádraig's suggestion and disabled SEEK_HOLE on zfs, we'd
still run into problems? That's really puzzling. Particularly since it's
not clear what program is generating the diagnostic "The service is no
longer running", or how it's related to GNU cp.

Anyway, the ZFS issue sounds like a serious bug in lseek+SEEK_DATA that
really needs to be fixed. This is not just a coreutils issue, as other
programs use SEEK_DATA.

I assume the ZFS bug (if the bug is related to ZFS, anyway) is a race
condition of some sort; at least, that's what the trace in
 suggests.

In particular, I was struck that the depthcharge.config file that 'cp'
was reading from was created by some other process, this way:

[pid 3014182] openat(AT_FDCWD,
"/build/guybrush/tmp/portage/sys-boot/depthcharge-0.0.1-r3237/image/firmware/guybrush/depthcharge/depthcharge.config",
O_WRONLY|O_CREAT|O_TRUNC|O_CLOEXEC, 0666) = 4
[pid 3014182] fstat(4, {st_mode=S_IFREG|0644, st_size=0, ...}) = 0
[pid 3014182] ioctl(4, TCGETS, 0x7ffd919d61c0) = -1 ENOTTY
(Inappropriate ioctl for device)
[pid 3014182] lseek(3, 0, SEEK_CUR) = 0
[pid 3014182] lseek(3, 0, SEEK_DATA)= 0
[pid 3014182] lseek(3, 0, SEEK_HOLE)= 9608
[pid 3014182] copy_file_range(3, [0], 4, [0], 9608, 0) = 9608
[pid 3014182] lseek(3, 0, SEEK_CUR) = 9608
[pid 3014182] lseek(3, 9608, SEEK_DATA) = -1 ENXIO (No such device or
address)
[pid 3014182] lseek(3, 0, SEEK_END) = 9608
[pid 3014182] ftruncate(4, 9608)= 0
[pid 3014182] close(4)  = 0

So, one hypothesis is that ZFS's implementation of copy_file_range does
not set up data structures appropriately for cp's later use of
lseek+SEEK_DATA when reading depthcharge.config. That is, from cp's
point of view, the ftruncate(4, 9608) has been executed but the
copy_file_range(3, [0], 4, [0], 9608, 0) has not been executed yet (it's
cached somewhere, no doubt).

If my guess is right, then an fdatasync or fsync on cp's input might
work around  common instances of this ZFS bug. Could you try the
attached coreutils patch, and see whether it works around the bug? Or
perhaps change 'fdatasync' with 'fsync' in the attached patch? Thanks.


Further debugging from Nix folks suggest ZFS was in consideration always,
as invalid artifacts were written to a central cache from ZFS backed hosts.
So we should at least change the comment in the patch to only mention ZFS.

Also it seems like fsync() does avoid the ZFS issue as mentioned in:
https://github.com/openzfs/zfs/issues/11900

BTW I'm slightly worried about retrying SEEK_DATA as
FreeBSD 9.1 has a bug with large sparse files at least
where it takes ages for SEEK_DATA to return:
  36.13290615 lseek(3,0x0,SEEK_DATA) = -32768 (0x8000)
If ENXIO is not set in that case, then there is no issue.

Also I'm not sure restricting sync to ENXIO is general enough,
as an strace from a problematic cp, from the github issue above is:
  lseek(3, 0, SEEK_DATA)= 0
  lseek(3, 0, SEEK_HOLE)= 131072
  lseek(3, 0, SEEK_SET) = 0
  read(3, "\177ELF\2\1"..., 131072) = 131072
  write(4, "\177ELF\2\"..., 131072) = 131072
  lseek(3, 131072, SEEK_DATA)   = -1 ENXIO
  ftruncate(4, 3318813) = 0

For completeness, there is another SEEK_DATA issue on ZFS,
but only a perf one, not a correctness one.
This was a perf issue we saw in our tests and handled with:
https://git.sv.gnu.org/cgit/coreutils.git/commit/?id=v9.0-3-g61c81ffaa
That case is where ZFS has an async operation that needs to complete
before SEEK_DATA can determine a sparse file is empty (return ENXIO).
That seems to be controlled with zfs-dmu-offset-next-sync
One can see that perf issue with:

   rm f3 f4 &&
   truncate -s 1T f3 &&
   sleep 2 &&
   timeout 1 src/cp --reflink=never f3 f4 &&
   echo ok

If you change the `sleep 2` to a `sleep 4`, then "ok" is echoed.
Sometimes `sleep 3` works, so this seems to be the zfs timer.
Changing to a smaller file doesn't seem to change anything.
Using `sync f3`, 'sync .`, or `sync` rather than `sleep 4` doesn't help.

cheers,
Pádraig





bug#51433: cp 9.0 sometimes fails with SEEK_DATA/SEEK_HOLE

2021-10-27 Thread Pádraig Brady

On 27/10/2021 11:00, Janne Heß wrote:

Hi everyone,

I packaged coreutils 9.0 for NixOS and we found breakages that seemed to be 
very random during builds of packages
that use the updated coreutils in their build process. It's really hard to tell 
the main cause but it seems like the issues
are caused by binaries that are corrupted after cp copied them from /tmp to 
/nix. The issue arises both when the
directories are on the same filesystem and when /tmp is on tmpfs.
Upon further inspection/bisection we figured out these issues are caused by 
a6eaee501f6ec0c152abe88640203a64c390993e.
This seems to happen on ZFS and indeed on the main coreutils mailing list there 
is a ZFS issue linked [1].
The testsuite was patched in 61c81ffaacb0194dec31297bc1aa51be72315858 so it 
doesn't detect this issue anymore,
but the issue still very much happens in the real world.

We have found this to happen while building the completions for a go tool (jx) 
which seems to be the same
issue as [2]. The tool is built, copied using cp, and called which causes a 
segfault to happen.

Building another package (peertube) on x86_64-linux on ext4 also fails with 
strange errors in the
test suite, something about "Error: The service is no longer running". This 
does not happen when the mentioned
coreutils commit is undone by replacing #ifdef with #if 0 [3].

We have also seen this issue on Darwin when building Alacritty but only 
happening on some machines
but we were not able to pin it down any further there so this might be related 
or it might not.

Since the issue is so random, we started wondering if it might be related to 
-frandom-seed which changes in NixOS
when rebuilding a package [4]. A thing to note here is that Nix does a lot of 
sandboxing stuff during builds which
includes mount namespaces so a Kernel bug is not out of the question. All of 
these issues happened during Nix builds,
coreutils 9.0 never made it out of the NixOS staging environment due to the 
builds breaking. We will probably disable
the new code paths as outlined above so the issue is contained for NixOS users 
and does not hit any production environments.

[1]: https://github.com/openzfs/zfs/issues/11900
[2]: https://github.com/golang/go/issues/48636
[3]: 
https://raw.githubusercontent.com/NixOS/nixpkgs/bf0531b4f8a2de4ff2700797fb211a90c951786e/pkgs/tools/misc/coreutils/disable-seek-hole.patch
[4]: https://github.com/NixOS/nixpkgs/pull/141684#issuecomment-952339263


We know about the ZFS issue with SEEK_HOLE:
https://lists.gnu.org/archive/html/coreutils/2021-10/msg00021.html

I've asked the user having nixos issues on darwin whether they're using the zfs 
on darwin port,
or at least what file system is being copied from there.

This is awkward to handle unfortunately.
All I can think of now is to identify the file system type for each source file,
and disable SEEK_HOLE on zfs at least.

thanks for the info,
Pádraig





bug#51345: dd with conv=fsync sometimes returns when its writes are still cached

2021-10-23 Thread Pádraig Brady

On 23/10/2021 09:18, Sworddragon wrote:

On Knoppix 9.1 with the Linux Kernel 5.10.10-64 x86_64 and GNU Coreutils
8.32 I wanted to overwrite my USB Thumb Drive a few times with random data
via "dd if=/dev/random of=/dev/sdb bs=1M conv=fsync". While it usually
takes ~2+ minutes to perform this action dd returned once after less than
60 seconds which made me a bit curious. On a later attempt dd returned with
this command after ~1 minute again but the LED on my USB Thumb Drive was
still blinking and an immediated executed sync on the terminal blocked for
~1 minute and once it returned the LED on my USB Thumb Drive also stopped
blinking.

Knoppix 9.1 was booted as a live system from a DVD and the only another
persistent storage attached to it was an internal HDD which was already
overwritten the same way via a past booting session.


Unfortunately Linux is not my main operating system anymore so I randomly
encountered this issue on a nearly 1 year old distribution which might be
slightly outdated. But the issue is pretty severe as it can lead to
significant data loss so it is worth at least reporting it (and having the
"bad" behavior to always call sync after dd nonetheless will probably now
continue to stay for quite a while).



Well we're relying on the kernel here to not return from fync()
until appropriate.  You could try running the following immediately after,
to see if it also returns quickly:

  blockdev --flushbufs /dev/sdb

Note a subsequent `sync /dev/sdb` would probably not be effective,
since that would only consider data written by theh sync process
(which would be nothing).

cheers,
Pádraig





bug#51311: [PATCH] echo: update --help to document edge cases

2021-10-22 Thread Pádraig Brady

On 21/10/2021 21:54, Bernhard Voelker wrote:

On 10/21/21 15:14, Florent Flament wrote:

Pádraig Brady  writes:

+NOTE: printf(1) is a preferred alternative, with more standard option 
handling.\



I believe that it misses the point. It is still not clear that the echo
command doesn't behave as one would expect for a few edge cases.

Maybe something like this would be closer to what I'm trying to express:

NOTE: printf(1) is a preferred alternative, which doesn't share echo's
inability to handle edge cases.


I'm not sure that just mentioning "edge cases" will remind people either
that they are falling into such particular edge case.

Therefore, I'd prefer Padraig's shorter sentence: it expresses the matter
positively while the latter proposal tries to explain via negative wording.

If we want to be more explicit, then we'd have to name examples where
printf(1) is superior to echo(1) - or the shell's echo builtin.
But IMO the whole point is two-fold: if someone doesn't have enough experience
to understand the edge cases, then eventually the usage of printf with the
often complex format specifiers is also too much.
Finally, I think Padraig's suggestion had the best tradeoff between pointing
out the matter and getting too much into details.


Thanks for all the input.
I've now pushed the following to address this:

https://git.sv.gnu.org/gitweb/?p=coreutils.git;a=commitdiff;h=f60a3981c

Notes:
 - Brevity in --help and man pages is a feature
 - I kept the existing NOTE: style in --help to avoid a separate [NOTES] in man
 - I only detailed the edge cases in the info manual,
 as directing users to printf(1) is the better most general option
 - It's easy to get to online info pages now by following the link
 at the bottom of all man pages

cheers,
Pádraig





bug#51311: [PATCH] echo: update --help to document edge cases

2021-10-21 Thread Pádraig Brady

On 20/10/2021 22:50, Florent Flament wrote:


* src/echo.c (usage): Document edge cases when displaying arbitrary
strings with the echo command.
---
  src/echo.c | 8 
  1 file changed, 8 insertions(+)

diff --git a/src/echo.c b/src/echo.c
index 18513398a..73b44660b 100644
--- a/src/echo.c
+++ b/src/echo.c
@@ -78,6 +78,14 @@ If -e is in effect, the following sequences are 
recognized:\n\
fputs (_("\
\\0NNN   byte with octal value NNN (1 to 3 digits)\n\
\\xHHbyte with hexadecimal value HH (1 to 2 digits)\n\
+"), stdout);
+  fputs (_("\
+\n\
+NOTE: The echo command doesn't behave gracefully when displaying\n\
+arbitrary strings. For example, it can't display the string \"-n\" and\n\
+requires the STRICTLY_POSIX flag to display \"-e\" or \"-E\". Therefore,\n\
+if you need to display arbitrary strings please use the printf command\n\
+instead.\n\
  "), stdout);
printf (USAGE_BUILTIN_WARNING, PROGRAM_NAME);
emit_ancillary_info (PROGRAM_NAME);


This is too verbose.
BTW the env var is POSIXLY_CORRECT, not STRICTLY_POSIX.
Anyway I don't think we should mention that in the man page anyway.
I'll push the attached later, which just says printf(1) is preferred.

cheers,
Pádraig

>From 0aaa7e4ad33f9cf89d525bda0f9bfe3f9b092288 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= 
Date: Thu, 21 Oct 2021 13:05:47 +0100
Subject: [PATCH] doc: say that printf(1) is preferred over echo(1)

* src/echo.c (usage): Say printf(1) is preferred
due to being more standard.
* man/echo.x [SEE ALSO]: Reference printf(1).
---
 man/echo.x | 2 ++
 src/echo.c | 4 
 2 files changed, 6 insertions(+)

diff --git a/man/echo.x b/man/echo.x
index 9c4fa8131..61a36706b 100644
--- a/man/echo.x
+++ b/man/echo.x
@@ -2,3 +2,5 @@
 echo \- display a line of text
 [DESCRIPTION]
 .\" Add any additional description here
+[SEE ALSO]
+printf(1)
diff --git a/src/echo.c b/src/echo.c
index 18513398a..d2262fa6b 100644
--- a/src/echo.c
+++ b/src/echo.c
@@ -80,6 +80,10 @@ If -e is in effect, the following sequences are recognized:\n\
   \\xHHbyte with hexadecimal value HH (1 to 2 digits)\n\
 "), stdout);
   printf (USAGE_BUILTIN_WARNING, PROGRAM_NAME);
+  fputs (_("\n\
+NOTE: printf(1) is a preferred alternative, with more standard option handling.\
+\n\
+"), stdout);
   emit_ancillary_info (PROGRAM_NAME);
   exit (status);
 }
-- 
2.26.2



bug#51128: timeout --kill-after=0 seems to not send a kill 0s after the initial signal

2021-10-12 Thread Pádraig Brady

On 12/10/2021 02:21, Christoph Anton Mitterer wrote:

Hey.

One more thing on this, since I've just read through:
https://www.gnu.org/software/coreutils/manual/html_node/timeout-invocation.html#timeout-invocation

That does IMO *not* document the behaviour:

--kill-after=duration says:

This option has no effect if timeout’s duration is 0 which disables
the associated timeout.


But that's about the timeout from the command itself (i.e. the 1st non-
option argument), isn't it?

So it means that if I have:

timeout --kill-after=10 0 ./command

There won't be a KILL after 10s, since the duration itself is 0.


Right. That is the behavior.


There's no word about that --kill-after=0 disabling the KILL in the
duration != 0 case as in:
timeout --kill-after=0 10 ./command


Well DURATIONs of 0 were mentioned a little below that.
But it it worth clarifying in the option description itself.
So I'll make the following change:

-This option has no effect if @command{timeout}'s duration is 0 which
-disables the associated timeout.
+This option has no effect if either the main @var{duration}
+of the @command{timeout} command, or the @var{duration} specified
+to this option, is 0.

cheers,
Pádraig





bug#51135: timeout: --foreground causes exit status 124, even if KILL was used

2021-10-12 Thread Pádraig Brady

On 12/10/2021 02:55, Christoph Anton Mitterer wrote:

Thinking again about this:

Don't you think one looses quite something if, with --foreground, one
cannot differ (via the exit status) between a timeout that allowed the
program to clean up and one (when KILLing) that didn't?


Even if the KILL happens via killing timeout itself, couldn't it just
return 128+9 in the case --foreground was enabled and the original
signal had already been sent?
Or is that technically not possible then?


That is a fair point.
If one is using --kill-after you have to
check for both 124 and 137 anyway to see if it timed out.
It is useful to know whether the command was forcably killed.
Using --foreground to avoid the 137 exit status upon --kill-after
is not until now documented, so we should probably adjust
the exit status to be always 137 if a SIGKILL is sent.

With the attached we now behave like:

  $ timeout -v -s0 --foreground -k2 1 sleep 3; echo $?
  timeout: sending signal EXIT to command ‘sleep’
  timeout: sending signal KILL to command ‘sleep’
  137

  $ timeout -v -s0 -k2 1 sleep 3; echo $?
  timeout: sending signal EXIT to command ‘sleep’
  timeout: sending signal KILL to command ‘sleep’
  Killed
  137

cheers,
Pádraig
>From 0750fcdf3447366b074cb47dd8cbe88c83ed984d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= 
Date: Tue, 12 Oct 2021 14:32:57 +0100
Subject: [PATCH] timeout: ensure --foreground -k exits with status 137

* src/timeout.c (main): Propagate the killed status from the child.
* doc/coreutils.texi (timeout invocation): Remove the
description of the --foreground specific handling of SIGKILL,
now that it's consistent with the default mode of operation.
* tests/misc/timeout.sh: Add a test case.
* NEWS: Mention the change in behavior.
Fixes https://bugs.gnu.org/51135
---
 NEWS  | 7 +++
 doc/coreutils.texi| 3 ---
 src/timeout.c | 5 +
 tests/misc/timeout.sh | 3 +++
 4 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/NEWS b/NEWS
index 1cb3c28a1..086da03ae 100644
--- a/NEWS
+++ b/NEWS
@@ -8,6 +8,13 @@ GNU coreutils NEWS-*- outline -*-
   All files would be processed correctly, but the exit status was incorrect.
   [bug introduced in coreutils-9.0]
 
+** Changes in behavior
+
+  timeout --foreground --kill-after=... will now exit with status 137
+  if the kill signal was sent, which is consistent with the behaviour
+  when the --foreground option is not specified.  This allows users to
+  distinguish if the command was more forcefully terminated.
+
 
 * Noteworthy changes in release 9.0 (2021-09-24) [stable]
 
diff --git a/doc/coreutils.texi b/doc/coreutils.texi
index e3ecbdcf8..02aae4ef4 100644
--- a/doc/coreutils.texi
+++ b/doc/coreutils.texi
@@ -18326,9 +18326,6 @@ exit status 137, regardless of whether that signal is sent to @var{command}
 or to @command{timeout} itself, i.e., these cases cannot be distinguished.
 In the latter case, the @var{command} process may still be alive after
 @command{timeout} has forcefully been terminated.
-However if the @option{--foreground} option is specified then
-@command{timeout} will not send any signals to its own process,
-and so it will exit with one of the other exit status values detailed above.
 
 Examples:
 
diff --git a/src/timeout.c b/src/timeout.c
index 34d792640..650563461 100644
--- a/src/timeout.c
+++ b/src/timeout.c
@@ -593,6 +593,11 @@ main (int argc, char **argv)
   unblock_signal (sig);
   raise (sig);
 }
+  /* Allow users to distinguish if command was forcably killed.
+ Needed with --foreground where we don't send SIGKILL to
+ the timeout process itself.  */
+  if (timed_out && sig == SIGKILL)
+preserve_status = true;
   status = sig + 128; /* what sh returns for signaled processes.  */
 }
   else
diff --git a/tests/misc/timeout.sh b/tests/misc/timeout.sh
index 44ca450d8..295a95773 100755
--- a/tests/misc/timeout.sh
+++ b/tests/misc/timeout.sh
@@ -42,7 +42,10 @@ returns_ 124 timeout --preserve-status .1 sleep 10 && fail=1
 # kill delay. Note once the initial timeout triggers,
 # the exit status will be 124 even if the command
 # exits on its own accord.
+# exit status should be 128+KILL
 returns_ 124 timeout -s0 -k1 .1 sleep 10 && fail=1
+# Ensure a consistent exit status with --foreground
+returns_ 124 timeout --foreground -s0 -k1 .1 sleep 10 && fail=1
 
 # Ensure 'timeout' is immune to parent's SIGCHLD handler
 # Use a subshell and an exec to work around a bug in FreeBSD 5.0 /bin/sh.
-- 
2.26.2



bug#51135: timeout: --foreground causes exit status 124, even if KILL was used

2021-10-11 Thread Pádraig Brady

On 11/10/2021 22:11, Christoph Anton Mitterer wrote:

On Mon, 2021-10-11 at 22:04 +0100, Pádraig Brady wrote:

+However if the @option{--foreground} option is specified then
+@command{timeout} will not send any signals to its own process,
+and so it will exit with one of the other exit status values
detailed above.


So 137 is only used when the signal was sent to timeout itself?

I'd have actually found it quite nice if 137 was used *even* with
--foreground.

That way one could differentiate between whether COMMAND had the chance
to do cleanups, or whether the calling process should take care on
that.


For example, I use timeout with a program that reads a phassphrase and
prints it to stdout.
It disables the terminal's ECHO, so when it has to be killed, the
calling process would need to re-enable that mnually.


For that use case it's probably best to use --preserve-status,
in which case the 137 from the child getting the SIGKILL
will be propagated through.





bug#51135: timeout: --foreground causes exit status 124, even if KILL was used

2021-10-11 Thread Pádraig Brady

On 11/10/2021 19:01, Christoph Anton Mitterer wrote:

Hey.


This time I've also checked the 9.0 documentation (hopefully I wasn't
just too blind).


I noticed that whenever --foreground is used, timeout exits with a 124
status (instead of the documented 128+9) regardless of whether the KILL
is sent because of --signal=KILL or --kill-after=n .


Yes --foreground gives timeout(1) more control over the exit status in this 
case.
I'll add some clarification to the texinfo manual along these lines:


-In case of the @samp{KILL(9)} signal, @command{timeout} returns with
+In the case of the @samp{KILL(9)} signal, @command{timeout} returns with
 exit status 137, regardless of whether that signal is sent to @var{command}
 or to @command{timeout} itself, i.e., these cases cannot be distinguished.
 In the latter case, the @var{command} process may still be alive after
 @command{timeout} has forcefully been terminated.
+However if the @option{--foreground} option is specified then
+@command{timeout} will not send any signals to its own process,
+and so it will exit with one of the other exit status values detailed above.

thanks,
Pádraig





bug#51128: timeout --kill-after=0 seems to not send a kill 0s after the initial signal

2021-10-11 Thread Pádraig Brady

On 11/10/2021 16:59, Christoph Anton Mitterer wrote:

On Mon, 2021-10-11 at 16:49 +0100, Pádraig Brady wrote:

Well that wouldn't be that useful functionality,
as why not just send a single kill signal in that case.


Well with the same argumentation one could say that timeout 0 command
doesn’t execute the command at all, since why should one call it with a
timeout, when there's anyway none?


It's more consistent for a duration of 0 to disable the associated timeout.


Also it may make "sense" if the value is configurable e.g. via some
user input made before,...


Exactly. A propagated user setting of 0 should be interpreted as
disabling the timeouts rather than the command itself,
as timing out immediately is not useful functionality.


And in my opinion, since this behaviour is really quite unexpected
(disabling an option, though explicitly setting it), it should at least
also be mentioned in the manpage, too.


The man page mentions this for DURATION (and thus for both timeouts mentioned 
above):
"A duration of 0 disables the associated timeout"

thanks,
Pádraig





bug#51128: timeout --kill-after=0 seems to not send a kill 0s after the initial signal

2021-10-11 Thread Pádraig Brady

tag 51128 notabug
close 51128
stop

On 11/10/2021 02:28, Christoph Anton Mitterer wrote:

Hey.

The timeout(1) manpage says:
-k, --kill-after=DURATION
   also send a KILL signal if COMMAND is still running
   this long after the initial signal was sent

My naive assumption would have been that --kill-after=0 means that a
signal is sent immediately after the original one.


Well that wouldn't be that useful functionality,
as why not just send a single kill signal in that case.


However, that doesn't seem to be the case.


If intended, this should perhaps at least be mentioned in the manpage
:-)


It is intended that a 0 duration disables that timeout.
It is mentioned in the full documentation at:
https://www.gnu.org/software/coreutils/timeout
The man pages don't try to document every nuance.

cheers,
Pádraig.





bug#51011: [PATCH] sort: --debug: add warnings about radix and grouping chars

2021-10-10 Thread Pádraig Brady

On 11/10/2021 00:34, Paul Eggert wrote:

The warnings look good, except that this one:


    $ printf '1.0\n0.9\n' | sort -s -k1,1g --debug
    sort: numbers use ‘.’ as a decimal point in this locale
    0.9
    ___
    1.0
    ___


seems overkill if we're in the C locale.

Also, shouldn't similar diagnostics be generated if the field separator
is '-', or '+', or a digit in the current locale?


Yes this is more informational than a warning.
As Bernhard mentioned it may be useful to tag
--debug messages as informational or warnings.

In this case it would be info:
but would change to warn: if (tab == decimal_point).

The reason for the info message is that it may not
be obvious to users that numeric comparison
depends on locale just like text,
and we already provide the informational
text comparison message indicating the current locale.
We would only show this info: message if doing numeric sorting.


+  if (numeric_field_span)
+{
+  char sep_string[2] = { 0, };
+  sep_string[0] = thousands_sep;
+  if ((tab == TAB_DEFAULT
+   && (isblank (to_uchar (thousands_sep
+  || tab == thousands_sep)
+{
+  error (0, 0,
+ _("field separator %s is treated as a "
+   "group separator in numbers"),
+ quote (sep_string));
+  number_locale_warned = true;
+}
+}


This code brought it to my attention that the GNU 'sort' has had a
longstanding bug (in code that I wrote long ago - sorry!) in that
thousands_sep is either -1 or an unsigned char converted to int, and
this doesn't work in some unusual cases. I installed the attached patch
to fix that bug, and I vaguely suspect that it fixes similar bugs in GNU
'test' and GNU 'expr'. Good thing you brought it to my attention.
(Sorry, I'm too lazy and/or time-pressed and/or overconfident to write
test cases)


I'd noted this and was going to follow up on it.
Thanks for sorting it out!


Anyway, with that patch installed, TAB and THOUSANDS_SEP can both be
CHAR_MAX + 1 so the above code needs to be twiddled. Also, we can assume
C99. So, something like following (pardon Thunderbird's line wrap):

if (numeric_field_span
&& (tab == TAB_DEFAULT
  ? thousands_char != NON_CHAR && isblank (to_uchar (thousands_sep))
  : tab == thousands_sep))
  {
error (0, 0,
 _("field separator %s is treated as a group separator in numbers"),
 quote (((char []) {thousands_sep, 0})));
number_locale_warned = true;
  }

with a similar replacement to the decimal_point code.


cheers,
Pádraig





bug#51011: [PATCH] sort: --debug: add warnings about radix and grouping chars

2021-10-10 Thread Pádraig Brady

On 10/10/2021 22:20, Bernhard Voelker wrote:

On 10/10/21 19:57, Pádraig Brady wrote:

sort: numbers use ‘.’ as a decimal point in this locale


What about adding the hint to that message that this an "ambiguity warning"?

 sort: ambiguity warning: numbers use ‘.’ as a decimal point in this locale

(Likewise for the other cases, of course.)

Most other --debug messages usually state how sort processes the options / the
input, while this one tells why the processing potentially does not work as the
user expected.

+1 otherwise.


Yes it may be useful to tag messages as "informational" or "problematic".
I've mentioned this also in a reply to Paul.

thanks for the review,
Pádraig





bug#51011: [PATCH] sort: --debug: add warnings about radix and grouping chars

2021-10-10 Thread Pádraig Brady

On 09/10/2021 23:29, Paul Eggert wrote:

On 10/9/21 5:00 AM, Pádraig Brady wrote:

On 09/10/2021 04:48, Paul Eggert wrote:



'sort' could determine the group sizes from the locale, and
reject digit strings that are formatted improperly according to the
group-size rules. (Not that I plan to write the code to do that)


Yes I agree that would be better, but not worth it I think
as there would still be ambiguity in what was a grouping char
and what was a field separator. Also that ambiguity would
now vary across locales.


I don't see the ambiguity problem. The field separator is used to
identify fields; once the fields are identified, the thousands
separator, decimal point, etc. contribute to numeric comparison in the
usual way. So it's OK (albeit confusing) for the field separator to be
'.' or ',' or '-' or '0' or any another character that could be part of
a number.

For example, with 'sort -t 0 -k 2,2n', the digit 0 is not part of the
numeric field that is compared, and there's no ambiguity about that even
though 0 is allowed in numbers. The same idea applies to 'sort -t , -k
2,2n'.


Indeed. I dropped -t, from my later examples and confused myself.

Attached is the proposed change to add appropriate warnings in this area.
Examples now diagnosed are:

  $ printf '0,9\n1,a\n' | sort -nk1 --debug -t, -s
  sort: key 1 is numeric and spans multiple fields
  sort: field separator ‘,’ is treated as a group separator in numbers
  1,a
  _
  0,9
  ___

  $ printf '1,a\n0,9\n' | LC_ALL=fr_FR.utf8 sort -gk1 --debug -t, -s
  sort: key 1 is numeric and spans multiple fields
  sort: field separator ‘,’ is treated as a decimal point in numbers
  0,9
  ___
  1,a
  __

  $ printf '1.0\n0.9\n' | sort -s -k1,1g --debug
  sort: numbers use ‘.’ as a decimal point in this locale
  0.9
  ___
  1.0
  ___

  $ printf '1.0\n0.9\n' | LC_ALL=fr_FR.utf8 sort -s -k1,1g --debug
  sort: numbers use ‘,’ as a decimal point in this locale
  0.9
  _
  1.0
  _

cheers,
Pádraig
>From 06410ad77fcd80010859586cc068d8931bcf74e6 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= 
Date: Sun, 10 Oct 2021 18:35:59 +0100
Subject: [PATCH] sort: --debug: add warnings about radix and grouping chars
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

New warnings are added related to the handling
of thousands grouping characters, and decimal points.
Examples now diagnosed are:

$ printf '0,9\n1,a\n' | sort -nk1 --debug -t, -s
sort: key 1 is numeric and spans multiple fields
sort: field separator ‘,’ is treated as a group separator in numbers
1,a
_
0,9
___

$ printf '1,a\n0,9\n' | LC_ALL=fr_FR.utf8 sort -gk1 --debug -t, -s
sort: key 1 is numeric and spans multiple fields
sort: field separator ‘,’ is treated as a decimal point in numbers
0,9
___
1,a
__

$ printf '1.0\n0.9\n' | sort -s -k1,1g --debug
sort: numbers use ‘.’ as a decimal point in this locale
0.9
___
1.0
___

$ printf '1.0\n0.9\n' | LC_ALL=fr_FR.utf8 sort -s -k1,1g --debug
sort: numbers use ‘,’ as a decimal point in this locale
0.9
_
1.0
_

* src/sort.c (key_warnings): Add the warnings above.
* tests/misc/sort-debug-warn.sh: Add test cases.
Addresses https://bugs.gnu.org/51011
---
 src/sort.c| 62 +--
 tests/misc/sort-debug-warn.sh | 33 ++-
 2 files changed, 92 insertions(+), 3 deletions(-)

diff --git a/src/sort.c b/src/sort.c
index 2979400e7..17bae2a57 100644
--- a/src/sort.c
+++ b/src/sort.c
@@ -2422,9 +2422,15 @@ key_warnings (struct keyfield const *gkey, bool gkey_only)
   struct keyfield const *key;
   struct keyfield ugkey = *gkey;
   unsigned long keynum = 1;
+  bool numeric_field = false;
+  bool numeric_field_span = false;
+  bool general_numeric_field_span = false;
 
   for (key = keylist; key; key = key->next, keynum++)
 {
+  if (key_numeric (key))
+numeric_field = true;
+
   if (key->traditional_used)
 {
   size_t sword = key->sword;
@@ -2479,8 +2485,14 @@ key_warnings (struct keyfield const *gkey, bool gkey_only)
   if (!sword)
 sword++;
   if (!eword || sword < eword)
-error (0, 0, _("key %lu is numeric and spans multiple fields"),
-   keynum);
+{
+  error (0, 0, _("key %lu is numeric and spans multiple fields"),
+ keynum);
+  if (key->general_numeric)
+general_numeric_field_span = true;
+  else
+numeric_field_span = true;
+}
 }
 
   /* Flag global options not copied or specified in any key.  */
@@ -2499,6 +2511,52 @@ key_warnings (struct keyfield const *gkey, bool gkey_only)
   ugkey.reverse &= !key->reverse;
 }
 
+  /* Explicitly warn if field delimiters in this locale
+ don't constrain numbers.  */
+  bool number_locale_warned = false;
+  if (numeric_field_span)
+{
+  cha

bug#51011: [GNU sort] Numerical sort with delimiter may be broken (bug)

2021-10-09 Thread Pádraig Brady

On 09/10/2021 04:48, Paul Eggert wrote:

On 10/8/21 7:32 PM, Pádraig Brady wrote:

it's not a thousands separator, rather a grouping
character,
and groups can be in 2, 3, 4, and even 5.


Sure, but 'sort' could determine the group sizes from the locale, and
reject digit strings that are formatted improperly according to the
group-size rules. (Not that I plan to write the code to do that)


Yes I agree that would be better, but not worth it I think
as there would still be ambiguity in what was a grouping char
and what was a field separator. Also that ambiguity would
now vary across locales.

Another possible change which I'd prefer TBH
would be to disable the grouping separator, or decimal point
if they overlapped with --field-separator.
Doing this would induce a warning from --debug also.

cheers,
Pádraig





bug#51011: [GNU sort] Numerical sort with delimiter may be broken (bug)

2021-10-08 Thread Pádraig Brady

On 08/10/2021 21:48, Paul Eggert wrote:

On 10/8/21 6:37 AM, Pádraig Brady wrote:


The difference here is due to ',' being treated as a thousands sep,
not a decimal point.


Oh, thanks. Of course! I should have figured that out myself.

It is unfortunate that "," is treated as a thousands seperator even
though it's obviously not one (as it's not followed by 3 decimal
digits). I don't think POSIX requires this behavior; it's not clear to
me that POSIX even allows it.


Well in general it's not a thousands separator, rather a grouping character,
and groups can be in 2, 3, 4, and even 5.  So I don't think we should
change the logic here.


This bug report suggests that we should alter the code so that 'sort -n'
acts more like common practice, and requires thousands separators to be
in the right places in order to treat nearby digits to be part of the
number. Alternatively, we could document the existing behavior (even if
it's not clear that it conforms to POSIX).


What we can do is have --debug warn when there is an overlap
in --field-separator and the grouping and decimal characters
when using numeric keys.  I'll have a look at that tomorrow.

cheers,
Pádraig





bug#51011: [GNU sort] Numerical sort with delimiter may be broken (bug)

2021-10-08 Thread Pádraig Brady

On 04/10/2021 21:01, Paul Eggert wrote:

On 10/4/21 08:58, Pádraig Brady wrote:

The --debug option points out the issue:

    $ printf '%s\n' 1,a 0,9 | sort --debug -nk1 -t ,
    sort: key 1 is numeric and spans multiple fields
    1,a
    _
    ___
    0,9
    ___
    ___


As Juncheng points out, it is a bit odd that -n and -g disagree here,
even in locales where ',' is not a decimal point. For example:

$ printf '1,a\n0,9\n' | sort -gk1 -t, --debug
sort: text ordering performed using ‘en_US.UTF-8’ sorting rules
sort: key 1 is numeric and spans multiple fields
0,9
_
___
1,a
_
___
$ printf '1,a\n0,9\n' | sort -nk1 -t, --debug
sort: text ordering performed using ‘en_US.UTF-8’ sorting rules
sort: key 1 is numeric and spans multiple fields
1,a
_
___
0,9
___
___


The difference here is due to ',' being treated as a thousands sep,
not a decimal point. So Juncheng to specifically answer your question,
0,9 is being interpreted as 9, which sorts after 1,a. For e.g. consider:

$ printf '%s\n' 1,a 0,900 | sort -s -k1,1g --debug
0,900
_
1,a
_

$ printf '%s\n' 1,a 0,900 | sort -s -k1,1n --debug
1,a
_
0,900
_


Given the various groupings possible (depending on locale
one can group in 2, 3, 4, 5 digits) we effectively just
ignore the grouping separator in numeric mode, hence the difference.

Note in locales where , is a decimal point we do get
consistent order between -g and -n as expected:

$ printf '%s\n' '1,a' '0,9' | LC_ALL=fr_FR.utf8 sort -s -k1,1n --debug
sort: tri du texte réalisé en utilisant les règles de tri « fr_FR.utf8 »
0,9
___
1,a
__
$ printf '%s\n' '1,a' '0,9' | LC_ALL=fr_FR.utf8 sort -s -k1,1g --debug
sort: tri du texte réalisé en utilisant les règles de tri « fr_FR.utf8 »
0,9
___
1,a
__

For completeness we do have another issue with grouping separators,
where we don't support multi-byte separators appropriately.
For e.g. fr_FR.utf8 uses "narrow non breaking space" as the separator,
which we don't support:

$ sep=$(LC_ALL=fr_FR.utf8 locale thousands_sep)
$ printf '%s\n' 0800 "0${sep}900" | LC_ALL=fr_FR.utf8 sort -s -k1,1n --debug
sort: tri du texte réalisé en utilisant les règles de tri « fr_FR.utf8 »
0 900
_
0800



cheers,
Pádraig





bug#51011: [GNU sort] Numerical sort with delimiter may be broken (bug)

2021-10-04 Thread Pádraig Brady

tag 51011 notabug
close 51011
stop

On 04/10/2021 15:36, Juncheng Yang wrote:

Hi coreutils developers,
 I have encountered a bug in GNU sort in which sort produces incorrect 
results when numerical sort with delimiters. For example,
sort -nk1 -t , file
cannot sort the a file with the following lines (sort by the first column 
numerically)
1,a
0,9

I have tried multiple version including the latest version, this problem still 
exists.


The --debug option points out the issue:

  $ printf '%s\n' 1,a 0,9 | sort --debug -nk1 -t ,
  sort: key 1 is numeric and spans multiple fields
  1,a
  _
  ___
  0,9
  ___
  ___


So you want -k1,1n

cheers,
Pádraig





bug#50972: src/ls.c fails to build when __GNUC_PREREQ is not defined (e.g., OpenBSD)

2021-10-03 Thread Pádraig Brady

On 03/10/2021 03:07, Paul Eggert wrote:

On 10/2/21 7:44 AM, Brian Callahan wrote:

Once this fix is done, the rest of coreutils-9.0 builds and works OK.


Thanks for letting us know, as it's been a while since I tried building
coreutils on OpenBSD. I just now tried it on OpenBSD 6.9 and found some
other problems that were easy to lose sight of in the blizzard of false
alarms that clang emits on OpenBSD 6.9. I fixed all the problems I
noticed and installed the following patches into coreutils:

https://git.savannah.gnu.org/cgit/coreutils.git/commit/?id=4cfd48481da0486e2bad193495bc38e7d5ead7e4
https://git.savannah.gnu.org/cgit/coreutils.git/commit/?id=b31a6a09ad0caddce29729f8c9bc4c36c073b7b6
https://git.savannah.gnu.org/cgit/coreutils.git/commit/?id=5b43b8e9ce215864001314f92097584bceb71b58

These rely on the following OpenBSD-related patches to Gnulib, which I
installed earlier today:

https://git.savannah.gnu.org/cgit/gnulib.git/commit/?id=9bcd248da1ef25b3ff3431248f53401e1123d74f
https://git.savannah.gnu.org/cgit/gnulib.git/commit/?id=dd0af10fa597a95ffe5f4f110ef5edefc2f680bc

The first patch listed above should fix the problem you mentioned; you
might want to look at the other patches as well.

By the way, how do you deal with all those false positives from clang?
Do you suppress them with CFLAGS? I'm referring to the many warnings
like "comparison is always true due to limited range of data type" and a
few "integer overflow in expression" warnings. With so many false alarms
I wouldn't be surprised if people stopped paying attention to compiler
warnings even if they're valid.


Thanks for doing all that.
I'm confused as to why some functions need _Noreturn and some don't.
For example why does cleanup_fatal() in csplit.c need it since it calls exit()?
Does GCC not propagate the noreturn from exit to cleanup_fatal()?

In general I see this may introduce warnings on older systems,
but that's fine as dev is generally done on newer compilers,
and non dev builds on older systems will not have -Werror set.

thanks,
Pádraig





  1   2   3   4   5   6   7   8   9   10   >