Re: gnulib-tool Python tracebacks after control-C

[Pádraig Brady]

On 13/05/2024 21:48, Collin Funk wrote:

Hi Paul,

On 5/13/24 9:39 AM, Paul Eggert wrote:

I realized that I invoked ' ./gnulib-tool --create-testdir --dir foo stdbit' 
with the wrong options so I interrupted it with control-C. It then gave me the 
following long traceback, which is not useful. I suggest shutting off the 
Python traceback info after control-C unless perhaps some new debugging option 
is used.


Thanks for the suggestion. I agree, but I need to find the correct way
to do that. IIRC some Python packages I have used do it properly but
others just do an ugly traceback.

I think that control-C raises a KeyboardInterrupt. The Python
documentation notes some special considerations that need to be made
there [1].

Collin

[1] https://docs.python.org/3/library/exceptions.html#KeyboardInterrupt


I've an 18 year old script template here that silences Ctrl-C spew:
https://www.pixelbeat.org/talks/python/cli_skel.py

You can see a more modern usage of that in crudini at:
https://github.com/pixelb/crudini/blob/cbb3b85e/crudini.py#L1134
Note that also handles a closed stdin (and stdout elsewhere in that util).

cheers,
Pádraig

Re: gnulib-tool: Use the Python implementation by default

[Pádraig Brady]

On 27/04/2024 14:32, Bruno Haible wrote:

Hi Pádraig,


+if (python3 --version) >/dev/null 2>/dev/null \
+   && case `python3 --version 2>&1` in
+Python\ 3.[0-6] | Python\ 3.[0-6].*) false ;;
+Python\ 3.*) true ;;
+*) false ;;
+  esac; then


It may be preferable to state the supported version directly,


I wanted a short warning message. In most cases the absence of python3
will be the problem. It should be rare than anyone still uses an old
version.


and replace the above 6 lines with:

if python3 -c 'import sys; sys.exit(not sys.version_info >= (3,7))' 
2>/dev/null; then


Thanks for the hint. I admit that I did not think at it. But is it
actually better?


Only that's it's shorter, and more directly states the supported version.
No biggie, just a suggestion.



Are there alternative Python implementations, which could be installed under
the name 'python3'?


So this is a general question, unrelated to my particular suggestion.
I think the version check is fine, and should be quite independent from python 
implementation.
Possible drop in replacements for the python3 command might be ...

... pypy, which separates its implementation version from python language 
support version:
$ dnf install pypy3
$ pypy3

 sys.pypy_version_info
sys.pypy_version_info(major=7, minor=3, micro=15, releaselevel='final', 
serial=0)
 sys.version_info
sys.version_info(major=3, minor=10, micro=13, releaselevel='final', serial=0)

... cinder, which is very close to cpython ...
$ dnf install docker
$ sudo systemctl start docker
$ sudo setfacl --modify user:$USER:rw /var/run/docker.sock
$ docker run -it --rm ghcr.io/facebookincubator/cinder-runtime:cinder-3.10

Python 3.10.5+cinder (main, Apr 26 2024, 22:00:58) [GCC 10.3.1 20210422 (Red 
Hat 10.3.1-1)] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import sys
>>> sys.version_info
sys.version_info(major=3, minor=10, micro=5, releaselevel='final', serial=0)


There are no other real contenders for drop in python 3 replacement.
For example codon or mojo are not designed as drop in replacements.
I expect any future drop in replacements would follow the model above.


Note also that your snippet would not replace the 6 lines of code, only the
last 5 lines, because with Solaris 10 /bin/sh your snippet produces the
diagnostic
   python3: not found


Out of interest, I dug out an old dsa ssh key from a backup, to get access to a 
solaris 10 machine.
Testing shows that a subshell does avoid that issue, so the following works:

  if (python3 -c 'import sys; sys.exit(not sys.version_info >= (3,7))') 
2>/dev/null; then

cheers,
Pádraig

Re: gnulib-tool: Use the Python implementation by default

[Pádraig Brady]

On 26/04/2024 10:48, Bruno Haible wrote:

I committed this patch, which activates the Python rewrite of gnulib-tool
for all users who have Python installed, without the need to set any
environment variable. I'll make announcements to info-gnu and planet.gnu.org
soon.



+if (python3 --version) >/dev/null 2>/dev/null \
+   && case `python3 --version 2>&1` in
+Python\ 3.[0-6] | Python\ 3.[0-6].*) false ;;
+Python\ 3.*) true ;;
+*) false ;;
+  esac; then


It may be preferable to state the supported version directly,
and replace the above 6 lines with:

  if python3 -c 'import sys; sys.exit(not sys.version_info >= (3,7))' 
2>/dev/null; then


+  exec "$gnulib_dir/gnulib-tool.py" "$@"
+else
+  echo "gnulib-tool: warning: python3 not found or too old, using the slow shell-based 
implementation" 1>&2
+  exec "$gnulib_dir/gnulib-tool.sh" "$@"
+fi
+;;


I've updated to the latest gnulib now in coreutils,
and the auto python selection works well.

thanks!
Pádraig

Re: beta-tester call draft

[Pádraig Brady]

On 20/04/2024 01:22, Bruno Haible wrote:

Hi,

It's now time to call for beta-testers of the Python gnulib-tool.
I plan to post the same text to info-gnu and to planet.gnu.org.

Here is a draft. Please comment!


coreutils results...


   4. Clean the built files of your package:
$ make -k distclean


I also needed to clean out old temp files / dirs that were otherwise gitignored,
which I did with: git clean -f -d -X
One can audit that command first by using -n instead of -f.

With that, the sh+py checks pass.

As for timings:

$ export GNULIB_TOOL_IMPL=sh
$ ./bootstrap
real2m17.872s
user1m24.687s
sys 1m20.877s

$ export GNULIB_TOOL_IMPL=py
$ ./bootstrap
real0m27.423s
user0m22.367s
sys 0m3.382s


$ time AUTORECONF=true ./bootstrap --skip-po
real0m7.302s
user0m5.172s
sys 0m2.755s

Amazing.

thank you!

Pádraig

Re: quotearg: shell escape issue with single quote

[Pádraig Brady]

On 03/04/2024 22:33, Bruno Haible wrote:

Hi Pádraig,


The attached should fix this issue.



   process_input:
+  bool pending_shell_escape_end = false;


It has a syntax error: With clang and with GCC versions < 11, it is invalid
to put a declaration after a label. See:

=== foo.c =
int main ()
{
   goto label;

  label:
   int ret = 0;
   return ret;
}
===
$ gcc -c foo.c
foo.c: In function ‘main’:
foo.c:6:3: error: a label can only be part of a statement and a declaration is 
not a statement
 6 |   int ret = 0;
   |   ^~~
$ clang -c foo.c
foo.c:6:3: error: expected expression
 6 |   int ret = 0;
   |   ^
foo.c:7:10: error: use of undeclared identifier 'ret'
 7 |   return ret;
   |  ^
2 errors generated.

The workaround is trivial: Add a semicolon after the colon:

process_input: ;
 bool pending_shell_escape_end = false;


Oh tricky!

Done and pushed.

thanks for the review,
Pádraig

Re: quotearg: shell escape issue with single quote

[Pádraig Brady]

On 31/03/2024 22:02, Bruno Haible wrote:

Pádraig Brady wrote:

If a string starts with a sequence that requires $'' quoting followed
by a \' and then another charactrer requiring $'' quoting, the '$' at
the start of the quoted string ends up missing:

  $ env printf '%q\n' $'\1\'\2'
  '\001'\'''$'\002'


Indeed that is a bug.
I'll be able to have a look at this tomorrow.


The '$' at the beginning is only missing in specific cases:

$ ./printf '%q\n' $'\1\'\2'
'\001'\'''$'\002'
$ ./printf '%q\n' $'a\1\'\2'
'''a'$'\001'\'''$'\002'
$ ./printf '%q\n' $'\1\'\2x'
''$'\001'\'''$'\002''x'

Also, let me apply some documentation improvement to the .h file (attached).


The attached should fix this issue.

cheers,
Pádraig
From c0962db4411fceeb9b5c170f567aa0d06718e335 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= 
Date: Wed, 3 Apr 2024 15:46:47 +0100
Subject: [PATCH] quotearg: fix shell-escape quoting with single quotes

With shell-escape quoting, we misquoted strings
where the first and last characters required escaping,
while the string also contained single quotes.

* lib/quotearg.c (quotearg_buffer_restyled): Ensure that
pending_shell_escape_end is reset to the initial state
when reprocessing input due to single quotes.
* tests/test-quotearg-simple.c: Add a minimal test case.
* tests/test-quotearg.c: Likewise.
* tests/test-quotearg.h: Likewise.
Reported by Grisha Levit
---
 ChangeLog|  15 +++
 lib/quotearg.c   |   2 +-
 tests/test-quotearg-simple.c | 176 +++
 tests/test-quotearg.c|  12 +--
 tests/test-quotearg.h|   7 +-
 5 files changed, 123 insertions(+), 89 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index d6b9f731bc..226d0d5311 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,18 @@
+2024-04-03  Pádraig Brady  
+
+	quotearg: fix shell-escape quoting with single quotes
+	With shell-escape quoting, we misquoted strings
+	where the first and last characters required escaping,
+	while the string also contained single quotes.
+
+	* lib/quotearg.c (quotearg_buffer_restyled): Ensure that
+	pending_shell_escape_end is reset to the initial state
+	when reprocessing input due to single quotes.
+	* tests/test-quotearg-simple.c: Add a minimal test case.
+	* tests/test-quotearg.c: Likewise.
+	* tests/test-quotearg.h: Likewise.
+	Reported by Grisha Levit
+
 2024-04-02  Collin Funk  
 
 	gnulib-tool.py: Use [] instead of list() to initialize empty lists.
diff --git a/lib/quotearg.c b/lib/quotearg.c
index 847101e605..958f589e52 100644
--- a/lib/quotearg.c
+++ b/lib/quotearg.c
@@ -260,7 +260,6 @@ quotearg_buffer_restyled (char *buffer, size_t buffersize,
   bool backslash_escapes = false;
   bool unibyte_locale = MB_CUR_MAX == 1;
   bool elide_outer_quotes = (flags & QA_ELIDE_OUTER_QUOTES) != 0;
-  bool pending_shell_escape_end = false;
   bool encountered_single_quote = false;
   bool all_c_and_shell_quote_compat = true;
 
@@ -304,6 +303,7 @@ quotearg_buffer_restyled (char *buffer, size_t buffersize,
 while (0)
 
  process_input:
+  bool pending_shell_escape_end = false;
 
   switch (quoting_style)
 {
diff --git a/tests/test-quotearg-simple.c b/tests/test-quotearg-simple.c
index 5a9a7695e8..18d33d4344 100644
--- a/tests/test-quotearg-simple.c
+++ b/tests/test-quotearg-simple.c
@@ -33,124 +33,138 @@
 
 static struct result_groups results_g[] = {
   /* literal_quoting_style */
-  { { "", "\0""1\0", 3, "simple", " \t\n'\"\033?""?/\\", "a:b", "a\\b",
+  { { "", "\0""1\0", 3, "simple", "\t'\t", " \t\n'\"\033?""?/\\", "a:b", "a\\b",
   "a' b", LQ RQ, LQ RQ },
-{ "", "1", 1, "simple", " \t\n'\"\033?""?/\\", "a:b", "a\\b",
+{ "", "1", 1, "simple", "\t'\t", " \t\n'\"\033?""?/\\", "a:b", "a\\b",
   "a' b", LQ RQ, LQ RQ },
-{ "", "1", 1, "simple", " \t\n'\"\033?""?/\\", "a:b", "a\\b",
+{ "", "1", 1, "simple", "\t'\t", " \t\n'\"\033?""?/\\", "a:b", "a\\b",
   "a' b", LQ RQ, LQ RQ } },
 
   /* shell_quoting_style */
-  { { "''", "\0""1\0", 3, "simple", "' \t\n'\\''\"\033?""?/\\'", "a:b",
-  "'a\\b'", "\"a' b\"", LQ RQ, LQ RQ },
-{ "''", "1", 1, "simple", "' \t\n'\\''\"\033?""?/\\'", "a:b",
-  "'a\\b'", "\"a' b\"", LQ RQ, LQ RQ },
-{ "''", &q

[PATCH] renameatu: handle ENOSYS from renameatx_np

[Pádraig Brady]

* lib/renameatu.c(): Fall back to renameat() without RENAME_EXCL
if "Function not implemented" is returned.  This was seen with macFUSE.
Reported at https://github.com/coreutils/coreutils/issues/79
---
 ChangeLog   | 7 +++
 lib/renameatu.c | 2 +-
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/ChangeLog b/ChangeLog
index 5c51c94166..1fda03418b 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,10 @@
+2024-04-02  Pádraig Brady  
+
+   renameatu: handle ENOSYS from renameatx_np
+   * lib/renameatu.c(): Fall back to renameat() without RENAME_EXCL
+   if "Function not implemented" is returned.  This was seen with macFUSE.
+   Reported at https://github.com/coreutils/coreutils/issues/79
+
 2024-04-02  Bruno Haible  
 
gnulib-tool: Remove unused variables.
diff --git a/lib/renameatu.c b/lib/renameatu.c
index 6893232516..815e3e0da9 100644
--- a/lib/renameatu.c
+++ b/lib/renameatu.c
@@ -78,7 +78,7 @@ renameat2ish (int fd1, char const *src, int fd2, char const 
*dst,
   if (flags)
 {
   int r = renameatx_np (fd1, src, fd2, dst, RENAME_EXCL);
-  if (r == 0 || errno != ENOTSUP)
+  if (r == 0 || (errno != ENOTSUP && errno != ENOSYS))
 return r;
 }
 # endif
-- 
2.44.0

Re: quotearg: shell escape issue with single quote

[Pádraig Brady]

On 31/03/2024 02:16, Grisha Levit wrote:

If a string starts with a sequence that requires $'' quoting followed
by a \' and then another charactrer requiring $'' quoting, the '$' at
the start of the quoted string ends up missing:

 $ env printf '%q\n' $'\1\'\2'
 '\001'\'''$'\002'


Indeed that is a bug.
I'll be able to have a look at this tomorrow.

cheers,
Pádraig

Re: bug#70104: "FAIL: test-canonicalize" coreutils v9.5 on musl libc

[Pádraig Brady]

On 31/03/2024 10:02, Adept's Lab wrote:

test-canonicalize.c:411: assertion 'strcmp (result1, "//") == 0' failed

^ the only error log message I get. Fail was not presented with previous
stable versions.


This is on musl 1.1.24 as detailed at:
https://github.com/coreutils/coreutils/issues/83

CC'ing bug-gnulib

cheers,
Pádraig

Re: gnulib-tool.py: Display specified modules in bold.

[Pádraig Brady]

On 30/03/2024 23:38, Bruno Haible wrote:

Hi Pádraig,


As a matter of interest, where did you get the figure that
94% of users have a $TERM set to xterm.* ?


Personal guesses / estimations :-D.


+def get_terminfo_string(capability: str) -> str:
+'''Returns the value of a string-type terminfo capability for the current 
value of $TERM.
+Returns the empty string if not defined.'''
+value = ''
+try:
+value = sp.run(['tput', capability], stdout=sp.PIPE, 
stderr=sp.DEVNULL).stdout.decode('utf-8')
+except Exception:
+pass
+return value


Might latin-1 be more appropriate here, to accept all byte sequences?


But it would be output in UTF-8 later, during print(...). therefore
converting to a string here is inappropriate.

I tried to return a byte array here instead of a string, but this led to
ugly code elsewhere.


I used the following for example to see that xterm-8bit outputs an invalid 
utf-8 sequence:

find /usr/share/terminfo -type f -printf '%f\n' | while read t; do
  echo -n $t; TERM=$t tput bold | od -An -tx1
done | grep -v 1b

I know the code path isn't used for xterm.* and I don't want to cause any more 
complexity,
but was wondering all the same.


If such a TERM value is used, the decode('utf-8') step will fail, thus the
value returned from get_terminfo_string will be empty, and bold-face won't
work.

This is not perfect, but it is good enough. Often, it's necessary to say
"it's good enough" because otherwise I spend more and more time on a tiny
feature. I have verified that it works with all TERM values of all OSes
for which I have virtual machines (except for Solaris 11.4, as mentioned);
I'm not inclined in spending more time on it.


Oh right I missed the exception handling :)
Absolutely agreed on all points.

Thanks for the clarification,
Pádraig.

Re: gnulib-tool.py: Display specified modules in bold.

[Pádraig Brady]

On 29/03/2024 21:59, Bruno Haible wrote:

Pádraig Brady wrote:

You could determine that programatically with something like:

if os.system(r'{ tput bold || tput md; } >/dev/null 2>&1') == 0:
# enable bold output


As a matter of interest, where did you get the figure that
94% of users have a $TERM set to xterm.* ?


+def get_terminfo_string(capability: str) -> str:
+'''Returns the value of a string-type terminfo capability for the current 
value of $TERM.
+Returns the empty string if not defined.'''
+value = ''
+try:
+value = sp.run(['tput', capability], stdout=sp.PIPE, 
stderr=sp.DEVNULL).stdout.decode('utf-8')
+except Exception:
+pass
+return value


Might latin-1 be more appropriate here, to accept all byte sequences?

I used the following for example to see that xterm-8bit outputs an invalid 
utf-8 sequence:

  find /usr/share/terminfo -type f -printf '%f\n' | while read t; do
echo -n $t; TERM=$t tput bold | od -An -tx1
  done | grep -v 1b

I know the code path isn't used for xterm.* and I don't want to cause any more 
complexity,
but was wondering all the same.

cheers,
Pádraig

Re: gnulib-tool.py: Display specified modules in bold.

[Pádraig Brady]

On 29/03/2024 13:12, Collin Funk wrote:

I noticed that the bold printing was missing. One of the most important
changes I've made yet.


You could determine that programatically with something like:

if os.system(r'{ tput bold || tput md; } >/dev/null 2>&1') == 0:
  # enable bold output

cheers,
Pádraig

Re: bug#70070: FAIL: test-localtime_r

[Pádraig Brady]

On 29/03/2024 12:40, Andreas Schwab wrote:

FAIL: test-localtime_r
==

test-localtime_r.c:58: assertion 'result->tm_hour == 18' failed
FAIL test-localtime_r (exit status: 134)

FAIL: test-localtime_r-mt
=

thread2 disturbed by thread1!
thread1 disturbed by thread2!
FAIL test-localtime_r-mt (exit status: 134)



CC'ing gnulib as those tests are from gnulib.
It may help to detail your platform.

thanks,
Pádraig.

Re: [PATCH] gnulib-tool.py: Follow gnulib-tool changes, part 39.

[Pádraig Brady]

On 03/03/2024 05:33, Collin Funk wrote:

On 3/2/24 7:02 PM, Collin Funk wrote:

When working on the one of the TODO entries I noticed that
the gnulib-comp.m4 output was incorrect.


Here is the item I was working on. I _think_ I understood it correctly
but you guys are much more talented at shell than me. :)

Specifically, I was a bit confused at first by the
func_emit_autoconf_snippets function. The diff showed the following
change:

-for m in $modules; do echo $m; done | LC_ALL=C sort -u > "$tmp"/modules
+for m in $referenceable_modules; do echo $m; done | LC_ALL=C sort -u > 
"$tmp"/modules

Which I didn't understand until seeing this line [1]:

deps=`for m in $deps; do echo $m; done | LC_ALL=C sort -u | LC_ALL=C join - 
"$tmp"/modules`

This just means that join takes stdin as its first file which is the
piped output of sort. Then the second file is the sorted
referenceable_modules stored in "$tmp"/modules right?

If I am interpreting that correctly then the lines I used would be
correct I think:

depmodules = sorted(set(depmodules).intersection(referenceable_modules))

The merge-gnulib script output seems more correct than usual, but I
could be missing something.

[1] 
https://git.savannah.gnu.org/cgit/gnulib.git/tree/gnulib-tool?id=589e96475f8f2d21a83405ab0672ce95091b80e5#n4296

Collin


Right, join without options is essentially set intersection.
See https://www.pixelbeat.org/cmdline.html#sets

cheers,
Pádraig

Re: sort dynamic linking overhead

[Pádraig Brady]

On 27/02/2024 21:36, Bruno Haible wrote:

Pádraig Brady wrote:

Does this work for all the various names of libcrypto in various distros?

Debian 12   libcrypto.so.3
Ubuntu 22.04libcrypto.so.1.1 libcrypto.so.3
Slackware 15libcrypto.so.1.1
openSUSE 15.5   libcrypto.so.1.1
CentOS Stream 9 libcrypto.so.3
Guix 1.4libcrypto.so.1.1
Alpine 3.19 libcrypto.so.3
FreeBSD 14.0libcrypto.so.38
NetBSD 9.3  libcrypto.so.14
OpenBSD 7.4 libcrypto.so.52.0



I only tested with libcrypto.so.3, but it should match all of the above.
It matches libcrypto.so.[.0-9]*


Here are my testing results (with the LIB_DL fix):

* On some machines, I had to install the packages with the 
   header files first:

   - Debian 12:
 # apt install libssl-dev
   - openSUSE 15.5:
 YaST software > install libopenssl-3-devel
   - Slackware 15:
 Download and unpack the openssl-1.1.1w binary packages
   - Alpine Linux 3.19:
 # apk add openssl openssl-dev

* On some platforms, the configure test gave

  checking whether openssl is GPL compatible... no

   due to the  header files being absent:

 - Guix 1.4
 - macOS 12.5
 - Cygwin 2.9.0

* On some platforms, the configure test gave

  checking whether openssl is GPL compatible... no

   because the openssl version is not >= 3.

 - Slackware 15 (has libcrypto.so.1.1)
 - NetBSD 9.3 (has libcrypto.so.14)
 - OpenBSD 7.4 (has libcrypto.so.52.0)
 - Solaris 11.4 (has libcrypto.so.1.0.0)

* On these platforms, the configure test gave

  checking whether openssl is GPL compatible... yes

   and LIBCRYPTO_SONAME got defined.

 - Debian 12
 - Ubuntu 22.04
 - openSUSE 15.5
 - CentOS Stream 9
 - Alpine Linux 3.19
 - FreeBSD 14.0
 - Android

   The value of LIBCRYPTO_SONAME is
 "libcrypto.so.30" on Android,
 "libcrypto.so.3" on the other platforms.


Thanks for all the testing / info.

FTR some platforms may deem openssl < 3 as GPL compat,
and they can `configure --with-openssl=auto`,
in which case this optimization would be enabled.

thanks!
Pádraig

Re: sort dynamic linking overhead

[Pádraig Brady]

On 27/02/2024 11:15, Bruno Haible wrote:

Paul Eggert wrote:

I installed the
attached patch into coreutils. With it, 'sort -R' continues to use MD5
but on GNUish platforms 'sort' links libcrypto dynamically only if -R is
used (Bruno's suggestion). This doesn't significantly affect 'sort -R'
performance, and reduces the startup overhead of plain 'sort'


The patch has no effect on openSUSE 15.5: DLOPEN_LIBCRYPTO is not defined
in config.h.

config.log shows a link error of the test program:
   undefined reference to symbol 'dlopen@@GLIBC_2.2.5'
Both the test program and 'sort' need to link with -ldl.

This is generally true on the following platforms:
   - glibc < 2.34,
   - Android (with some compilers, not with Termux (*)).

(*) In the Android Termux environment, the compiler is configured to pass
the options '-ldl -lc', rather than just '-lc', to the linker. Thus,
we don't need to pass '-ldl' to the compiler. But the 'sort' program will
be linked with -ldl. In other Android environments, things may be different,
though.

The proposed attached patch fixes the problem: It defines a variable LIB_DL
that contains '-ldl' where needed or '' if not needed, and uses it with
the test program and with 'sort'.

You might think that this patch is no win, because it trades one link
dependency for another link dependency? But that's not what it does:
'ldd' shows that without the patch, 'sort' loads the libraries

   linux-vdso.so.1
   libcrypto.so.3
   libpthread.so.0
   libc.so.6
   libz.so.1
   libdl.so.2
   /lib64/ld-linux-x86-64.so.2

and with the patch, 'sort' loads the libraries

   linux-vdso.so.1
   libdl.so.2
   libpthread.so.0
   libc.so.6
   /lib64/ld-linux-x86-64.so.2

— that is, libdl.so.2 is getting loaded anyway.


Applied.

thanks,
Pádraig

Re: sort dynamic linking overhead

[Pádraig Brady]

On 26/02/2024 18:06, Andreas Schwab wrote:

On Feb 26 2024, Pádraig Brady wrote:


https://git.sv.gnu.org/gitweb/?p=coreutils.git;a=commitdiff;h=3bbdb3938


It's still bad as it adds a hidden dependency that is invisible to rpm.


Right. In practice though since coreutils also links libcrypto
for cksum and the separate digest utilities, this should be OK.
In edge cases with a separate package per util, the dependency
can be added manually.


Also, the regexp in the sed command contains unquoted uses of '.' that
are supposed to be matched literally.


Fixed.

thanks,
Pádraig

Re: sort dynamic linking overhead

[Pádraig Brady]

On 26/02/2024 17:39, Bruno Haible wrote:

Pádraig Brady wrote:

+  void *handle = dlopen ("libcrypto.so", RTLD_LAZY | RTLD_GLOBAL);


That only works if libopenssl-devel is installed.


Good spot.
I'd already pushed a fix for this at:
https://git.sv.gnu.org/gitweb/?p=coreutils.git;a=commitdiff;h=3bbdb3938


Does this work for all the various names of libcrypto in various distros?

Debian 12   libcrypto.so.3
Ubuntu 22.04libcrypto.so.1.1 libcrypto.so.3
Slackware 15libcrypto.so.1.1
openSUSE 15.5   libcrypto.so.1.1
CentOS Stream 9 libcrypto.so.3
Guix 1.4libcrypto.so.1.1
Alpine 3.19 libcrypto.so.3
FreeBSD 14.0libcrypto.so.38
NetBSD 9.3  libcrypto.so.14
OpenBSD 7.4 libcrypto.so.52.0



I only tested with libcrypto.so.3, but it should match all of the above.
It matches libcrypto.so.[.0-9]*

cheers,
Pádraig

Re: sort dynamic linking overhead

[Pádraig Brady]

On 26/02/2024 17:11, Andreas Schwab wrote:

On Feb 25 2024, Paul Eggert wrote:


+/* Dynamically link the crypto library, if it needs linking.  */
+static void
+link_libcrypto (void)
+{
+#if DLOPEN_LIBCRYPTO && HAVE_OPENSSL_MD5
+  void *handle = dlopen ("libcrypto.so", RTLD_LAZY | RTLD_GLOBAL);


That only works if libopenssl-devel is installed.


Good spot.
I'd already pushed a fix for this at:
https://git.sv.gnu.org/gitweb/?p=coreutils.git;a=commitdiff;h=3bbdb3938

thanks,
Pádraig

Re: sort dynamic linking overhead

[Pádraig Brady]

On 26/02/2024 06:44, Yann Collet wrote:

  * xxhash128 is not a cryptographic hash function, so it doesn't attempt tobe 
random.

Just a correction : xxh128 does try to be random. And quite hardly: a 
significant amount of development is spent on ensuring this property.

It’s even tested with PractRand, and it could be used as a good random number 
generator.

Being non-cryptographic means that what it doesn’t try is to make sure no one 
can intentionally forge a hash collision from 2 different files (other than 
brute-forcing, which is impractical).

But that’s different, and I wouldn’t call this property “randomness”, even 
though randomness is a pre-requisite (but not sufficient in itself) to 
collision resistance.


Right. I was looking at both md5 and xxhash128 having a 10 quality score in the 
SMHasher metric.
I even saw a comment from you Yann that xxhas128 may have slightly better 
dispersion than md5.
Also md5 shouldn't be considered as cryptographic anyway since it's broken.
I.e. I don't think users would need to be informed of this change if made.

Re Paul's committed patch, it's a good improvement, and does not add a new 
(xxhash) dependency.
Paul, should the configure check, be testing for the MD5 routines rather than 
SHA512?
Also an entry in the Improvements section of NEWS would be appropriate.

thanks!
Pádraig

Re: [GNULIB PATCH] selinux: add stubs and wrappers for raw counterparts

[Pádraig Brady]

On 19/12/2023 14:54, Christian Göttsche wrote:

Add stubs and wrappers of already covered libselinux interfaces for
their `raw` counterparts.  These counterparts perform the same
operation expect for context translation.  Context translation is used
to convert SELinux MCS/MLS labales into human readable form, see
mcstransd(8).


I adjusted the commit message a little and pushed.

thanks,
Pádraig

Re: bug#67731: bootstrap, tries to fetch files although using --no-git --skip-po and --gnulib-srcdir...

[Pádraig Brady]

On 10/12/2023 15:33, Bruno Haible wrote:

Pádraig Brady wrote:

The attached adjusts bootstrap so the options are passed
through the upgrade_bootstrap() function.


What about the upgrade_bootstrap invocation in top/bootstrap line 195?
Does it need the change as well?


Oh I see; build-aux/bootstrap is auto generated.
Fixed and pushed.

Also pushed an update to coreutils to reference the fix.

thanks,
Pádraig

Re: bug#67731: bootstrap, tries to fetch files although using --no-git --skip-po and --gnulib-srcdir...

[Pádraig Brady]

On 09/12/2023 20:22, Bjarni Ingi Gislason wrote:

bootstrap.loc: bootstrapping with --no-git
--gnulib-srcdir=/home/bg/git/gnulib --skip-po --bootstrap-sync
./bootstrap: updating bootstrap and restarting...
./bootstrap: Bootstrapping from checked-out coreutils sources...
./bootstrap: consider installing git-merge-changelog from gnulib
./bootstrap: getting translations into po/.reference for
coreutils...
OUTPUT:INVALID,NEW,REJECT IN= OUT=eth0 SRC=192.168.1.72
DST=80.69.83.146 LEN=60 TOS=0x00 PREC=0x00 TTL=64 ID=21973 DF
PROTO=TCP
SPT=51544 DPT=443 WINDOW=64240 RES=0x00 SYN URGP=0
OUTPUT:INVALID,NEW,REJECT IN= OUT=eth0 SRC=192.168.1.72
DST=80.69.83.146 LEN=60 TOS=0x00 PREC=0x00 TTL=64 ID=21974 DF
PROTO=TCP
SPT=51544 DPT=443 WINDOW=64240 RES=0x00 SYN URGP=0
failed: Connection refused.
failed: Network is unreachable.
./bootstrap: could not fetch auxiliary files


It looks like --bootstrap-sync is the issue here.
Adding a little debug shows that upgrade_bootstrap()
does not propagate params to the subsequent bootstrap run:

$ ./bootstrap --gnulib-srcdir=/home/padraig/g/gnulib --no-git --skip-po 
--bootstrap-sync
bootstrap params: --gnulib-srcdir=/home/padraig/g/gnulib --no-git --skip-po 
--bootstrap-sync
./bootstrap: updating bootstrap and restarting...
bootstrap params: --no-bootstrap-sync

The attached adjusts bootstrap so the options are passed
through the upgrade_bootstrap() function.

Marking this as done,

thanks,
PádraigFrom cb632d68ef3cb976c5b366f633478419d1791121 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= 
Date: Sun, 10 Dec 2023 14:46:58 +
Subject: [PATCH] bootstrap: fix option propagation with --bootstrap-sync

* build-aux/bootstrap: Ensure options are propagated through
upgrade_bootstrap().
Fixes https://bugs.gnu.org/67731
---
 ChangeLog   | 6 ++
 build-aux/bootstrap | 4 ++--
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 1a3429e03b..b0d748029d 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,9 @@
+2023-12-10  Pádraig Brady  
+
+	bootstrap: fix option propagation with --bootstrap-sync
+	* build-aux/bootstrap: Ensure options are propagated through
+	upgrade_bootstrap().
+
 2023-12-01  Paul Eggert  
 
 	frexp, frexpf: pacify clang re address-of-volatile
diff --git a/build-aux/bootstrap b/build-aux/bootstrap
index 979b3af62e..15bda089f6 100755
--- a/build-aux/bootstrap
+++ b/build-aux/bootstrap
@@ -777,7 +777,7 @@ autopull()
   if $use_gnulib || $bootstrap_sync; then
 prepare_GNULIB_SRCDIR
 if $bootstrap_sync; then
-  upgrade_bootstrap
+  upgrade_bootstrap "$@"
 fi
   fi
 
@@ -1496,7 +1496,7 @@ check_build_prerequisites $use_git
 
 if $bootstrap_sync; then
   prepare_GNULIB_SRCDIR
-  upgrade_bootstrap
+  upgrade_bootstrap "$@"
   # Since we have now upgraded if needed, no need to try it a second time below.
   bootstrap_sync=false
 fi
-- 
2.41.0

Re: [PATCH] base32, base64: disallow non-canonical encodings

[Pádraig Brady]

On 27/10/2023 10:23, Simon Josefsson wrote:

Pádraig Brady  writes:


However if there are good use-cases for bad inputs
we may need to adjust this patch,
rather than failing unconditionally.

For example we could just flag non canonical input in the context,
and leave it up to the caller how to deal with that.


That adds complexity -- I'd prefer to just default to fail and see if we
get complaints.


It would be good to know an example of good use-cases
for bad inputs though, as I can't think of any.


The simplest example of good use-case is to be able to decode existing
incorrectly formatted inputs.  However I think this is one that could be
defered to other tools for that purpose, since generally this is not a
trivial feature and it is a slippery slope to support all needs.

This may becomes a problem if user failure happens at a very high level
and doing the low-level base64-decoding separately is not feasible in an
application, but let's see...

/Simon


Ok pushed.

Thanks for the review!

Pádraig

Re: [PATCH] base32, base64: disallow non-canonical encodings

[Pádraig Brady]

On 27/10/2023 08:28, Simon Josefsson wrote:

Pádraig Brady  writes:


To give a little more context, this will avoid
round trip issues like the following, by failing early:

   $ echo "HelloWorld==" | base64 -d | base64
   HelloWorlQ==


Thanks for background and patches!  There are use-cases for bad inputs
(both for good and malicious purposes), but I believe these should be
considered corner-cases and agree that the default should be to reject
them.


Right the default operation should be more resilient.

However if there are good use-cases for bad inputs
we may need to adjust this patch,
rather than failing unconditionally.

For example we could just flag non canonical input in the context,
and leave it up to the caller how to deal with that.

It would be good to know an example of good use-cases
for bad inputs though, as I can't think of any.

thanks,
Pádraig.

Re: [PATCH] base32, base64: disallow non-canonical encodings

[Pádraig Brady]

On 26/10/2023 16:38, Pádraig Brady wrote:

Unconditionally disallow encodings that don't have
appropriate zero bits before padding characters.
This handles one class of encoding variations discussed at:
https://eprint.iacr.org/2022/361.pdf
Note the other variations discussed there, due to optional
padding characters are already disallowed.

* lib/base32.c: Check that discarded bits in the encoding are zero.
* lib/base64.c: Likewise.
* tests/test-base32.c: Add test cases.
* tests/test-base64.c: Likewise.


To give a little more context, this will avoid
round trip issues like the following, by failing early:

  $ echo "HelloWorld==" | base64 -d | base64
  HelloWorlQ==

In general that would make the decoder a bit better
at detecting corruption in the encoded stream,
and more resilient to attacks where users tweak the
encoded stream for nefarious purposes.

I can't see any legitimate case where one would not want/have
zero bits in this case.

Note RFC4648 says:
"In some environments, the alteration is critical and therefore
decoders MAY chose to reject an encoding if the pad bits have not
been set to zero."

So it's within spec, but I don't see a reason why
you would want the behavior of allowing non zero pad bits.

cheers,
Pádraig.

[PATCH] base32, base64: disallow non-canonical encodings

[Pádraig Brady]

Unconditionally disallow encodings that don't have
appropriate zero bits before padding characters.
This handles one class of encoding variations discussed at:
https://eprint.iacr.org/2022/361.pdf
Note the other variations discussed there, due to optional
padding characters are already disallowed.

* lib/base32.c: Check that discarded bits in the encoding are zero.
* lib/base64.c: Likewise.
* tests/test-base32.c: Add test cases.
* tests/test-base64.c: Likewise.
---
 ChangeLog   |  8 
 lib/base32.c| 18 ++
 lib/base64.c|  9 +
 tests/test-base32.c | 15 +++
 tests/test-base64.c |  9 +
 5 files changed, 59 insertions(+)

diff --git a/ChangeLog b/ChangeLog
index fed7803763..2a42cd4217 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,11 @@
+2023-10-26  Pádraig Brady  
+
+   base32, base64: disallow non-canonical encodings
+   * lib/base32.c: Check that discarded bits in the encoding are zero.
+   * lib/base64.c: Likewise.
+   * tests/test-base32.c: Add test cases.
+   * tests/test-base64.c: Likewise.
+
 2023-10-25  Paul Eggert  
 
base32: new function isubase32; also, tune.
diff --git a/lib/base32.c b/lib/base32.c
index 2d692a2b38..132c01df64 100644
--- a/lib/base32.c
+++ b/lib/base32.c
@@ -354,6 +354,10 @@ decode_8 (char const *restrict in, idx_t inlen,
   if (in[3] != '=' || in[4] != '=' || in[5] != '='
   || in[6] != '=' || in[7] != '=')
 return_false;
+
+  /* Reject non-canonical encodings.  */
+  if (base32_to_int[to_uchar (in[1])] & 0x03)
+return_false;
 }
   else
 {
@@ -372,6 +376,10 @@ decode_8 (char const *restrict in, idx_t inlen,
 {
   if (in[5] != '=' || in[6] != '=' || in[7] != '=')
 return_false;
+
+  /* Reject non-canonical encodings.  */
+  if (base32_to_int[to_uchar (in[3])] & 0x0F)
+return_false;
 }
   else
 {
@@ -389,6 +397,10 @@ decode_8 (char const *restrict in, idx_t inlen,
 {
   if (in[6] != '=' || in[7] != '=')
 return_false;
+
+  /* Reject non-canonical encodings.  */
+  if (base32_to_int[to_uchar (in[4])] & 0x01)
+return_false;
 }
   else
 {
@@ -415,6 +427,12 @@ decode_8 (char const *restrict in, idx_t inlen,
   --*outleft;
 }
 }
+  else
+{
+  /* Reject non-canonical encodings.  */
+  if (base32_to_int[to_uchar (in[6])] & 0x07)
+return_false;
+}
 }
 }
 }
diff --git a/lib/base64.c b/lib/base64.c
index 59a2135bf1..1f53498bb3 100644
--- a/lib/base64.c
+++ b/lib/base64.c
@@ -396,6 +396,10 @@ decode_4 (char const *restrict in, idx_t inlen,
 
   if (in[3] != '=')
 return_false;
+
+  /* Reject non-canonical encodings.  */
+  if (base64_to_int[to_uchar (in[1])] & 0x0F)
+return_false;
 }
   else
 {
@@ -416,6 +420,11 @@ decode_4 (char const *restrict in, idx_t inlen,
 {
   if (inlen != 4)
 return_false;
+
+  /* Reject non-canonical encodings.  */
+  if (base64_to_int[to_uchar (in[2])] & 0x03)
+return_false;
+
 }
   else
 {
diff --git a/tests/test-base32.c b/tests/test-base32.c
index 2e7d3c53b3..16fb982edb 100644
--- a/tests/test-base32.c
+++ b/tests/test-base32.c
@@ -256,5 +256,20 @@ main (void)
   ok = base32_decode_alloc_ctx (NULL, "AABBAA=A", 8, , );
   ASSERT (!ok);
 
+  ok = base32_decode_alloc_ctx (NULL, "FZ==", 8, , );
+  ASSERT (!ok);
+
+  ok = base32_decode_alloc_ctx (NULL, "FYXB", 8, , );
+  ASSERT (!ok);
+
+  ok = base32_decode_alloc_ctx (NULL, "FYXC5===", 8, , );
+  ASSERT (!ok);
+
+  ok = base32_decode_alloc_ctx (NULL, "FYXC4LR=", 8, , );
+  ASSERT (!ok);
+
+  ok = base32_decode_alloc_ctx (NULL, "FZ==FY==", 16, , );
+  ASSERT (!ok);
+
   return 0;
 }
diff --git a/tests/test-base64.c b/tests/test-base64.c
index 5196db09f4..0da5f99054 100644
--- a/tests/test-base64.c
+++ b/tests/test-base64.c
@@ -233,5 +233,14 @@ main (void)
   ok = base64_decode_alloc_ctx (NULL, "aax=X", 5, , );
   ASSERT (!ok);
 
+  ok = base64_decode_alloc_ctx (NULL, "SGVsbG9=", 8, , );
+  ASSERT (!ok);
+
+  ok = base64_decode_alloc_ctx (NULL, "TR==", 4, , );
+  ASSERT (!ok);
+
+  ok = base64_decode_alloc_ctx (NULL, "TWF=TWE=", 8, , );
+  ASSERT (!ok);
+
   return 0;
 }
-- 
2.41.0

Re: new modules nan, qnan, snan

[Pádraig Brady]

Hi Bruno,

coreutils ci fails due to modules/snan referring to
a non existent m4/snan.m4 file.

cheers,
Pádraig

Re: sort dynamic linking overhead

[Pádraig Brady]

On 08/10/2023 21:53, Pádraig Brady wrote:

On 08/10/2023 14:36, Pádraig Brady wrote:

On 07/10/2023 22:29, Paul Eggert wrote:

On 2023-10-07 04:42, Pádraig Brady wrote:


The auto linking is globally controlled with the --with-openssl
cofigure option, but you could build sort (and md5sum)
without that dependency with:

  ./configure ac_cv_lib_crypto_MD5=no


Thanks, I was thinking more along the lines that Bruno suggested, which
to continue to link to libcrypto, but do it with dlopen/dlsym in 'sort'
only when need_random is true.

It's not clear to me offhand whether this should be done entirely in
Coreutils, or whether we should add some Gnulib support to make it
easier to do this sort of lazier linking.


I was wondering if this was worth worrying about at all,
but it is a significant overhead that's worth improving.
To quantify the overhead I compared optimized builds,
with and without the above configure option, giving:

$ time seq 1 | xargs -I'{}' src/sort /dev/null -k'{}'
real0m7.009s
user0m3.462s
sys 0m3.578s

$ time seq 1 | xargs -I'{}' src/sort-lc /dev/null -k'{}'
real0m12.950s
user0m3.754s
sys 0m9.200s


So we should do something. Now dlopening libcrypto on demand
would work, but there may be better solutions.
sort doesn't have to use md5. It could use blake2 routines
already in coreutils to avoid the issue (and get some speed ups).
Alternatively it might use some other hash function.
For example see the other 128 bit functions compared at:
https://github.com/Cyan4973/xxHash

BTW there was mention of static linking as an option in this thread.
That's is an option to provide better speed an isolation for binaries,
however it's best left to the system builders to use this for their builds.
There can be security implications for prompt library updating,
and libcrypto is particularly sensitive in this regard.


Adding coreutils list...

So above we've demonstrated that sort dynamically loading libcrypto
does nearly double the startup time for the process.

Attached is a patch to use the coreutils reference blake2b hash instead
of the optimized libcrypto md5 routines.

$ seq 100 > 1.txt

$ time src/sort-md5-lc -R < 1.txt > /dev/null
real0m6.734s
user0m23.258s
sys 0m0.047s

$ time src/sort-blake2 -R < 1.txt > /dev/null
real0m7.215s
user0m25.683s
sys 0m0.043s

$ grep 'model name' /proc/cpuinfo | head -n1
model name  : Intel(R) Core(TM) i7-5600U CPU @ 2.60GHz
$ rpm -q openssl-libs
openssl-libs-3.0.9-2.fc38.x86_64

So while this avoids the startup overhead,
the reference blake2 routines are a little less efficient
than the optimized md5 libcrypto routines.


An incremental patch attached to use xxhash128 (0.8.2)
shows a good improvement (note avx2 being used on this cpu):

  $ time src/sort-xxh -R < 1.txt > /dev/null
  real  0m4.111s
  user  0m14.429s
  sys   0m0.058s

I'm not sure how best to avail of it though.
Perhaps embed, or maybe link statically if available?

cheers,
PádraigFrom 912c5d139c24bf0ad5aa5459bd02506be30ab206 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= 
Date: Mon, 9 Oct 2023 14:45:01 +0100
Subject: [PATCH] sort: use XXH128 rather than blake2b

  $ seq 100 > 1.txt

  $ time src/sort-md5-lc -R < 1.txt > /dev/null
  real	0m6.734s
  user	0m23.258s
  sys	0m0.047s

  $ time src/sort-blake2 -R < 1.txt > /dev/null
  real	0m7.215s
  user	0m25.683s
  sys	0m0.043s

  Using xxhash128 (0.8.2) shows a good improvement
  (note avx2 being used on this cpu):

  $ time src/sort-xxh -R < 1.txt > /dev/null
  real	0m4.111s
  user	0m14.429s
  sys	0m0.058s

  $ grep 'model name' /proc/cpuinfo | head -n1
  model name	: Intel(R) Core(TM) i7-5600U CPU @ 2.60GHz
  $ rpm -q openssl-libs
  openssl-libs-3.0.9-2.fc38.x86_64

* src/sort.c: Use xxh128 routines rather than blake2b.
* src/xxhash.h: Include xxhash appropriately.
---
 src/sort.c   | 26 +-
 src/xxhash.h |  3 +++
 2 files changed, 16 insertions(+), 13 deletions(-)
 create mode 100644 src/xxhash.h

diff --git a/src/sort.c b/src/sort.c
index 14c3951ce..5f04d6921 100644
--- a/src/sort.c
+++ b/src/sort.c
@@ -31,7 +31,7 @@
 #include "system.h"
 #include "argmatch.h"
 #include "assure.h"
-#include "blake2/blake2.h"
+#include "xxhash.h"
 #include "fadvise.h"
 #include "filevercmp.h"
 #include "flexmember.h"
@@ -2085,7 +2085,7 @@ getmonth (char const *month, char **ea)
 }
 
 /* A randomly chosen state, used for random comparison.  */
-static blake2b_state random_hash_state;
+static XXH3_state_t random_hash_state;
 #define RANDOM_HASH_BYTES 16
 
 /* Initialize the randomly chosen hash state.  */
@@ -2100,8 +2100,8 @@ random_hash_state_init (char const *random_source)
   randread (r, buf, sizeof buf);
   if (randread_free (r) != 0)
 sort_die (_("close failed&

sort dynamic linking overhead

[Pádraig Brady]

On 08/10/2023 14:36, Pádraig Brady wrote:

On 07/10/2023 22:29, Paul Eggert wrote:

On 2023-10-07 04:42, Pádraig Brady wrote:


The auto linking is globally controlled with the --with-openssl
cofigure option, but you could build sort (and md5sum)
without that dependency with:

     ./configure ac_cv_lib_crypto_MD5=no


Thanks, I was thinking more along the lines that Bruno suggested, which
to continue to link to libcrypto, but do it with dlopen/dlsym in 'sort'
only when need_random is true.

It's not clear to me offhand whether this should be done entirely in
Coreutils, or whether we should add some Gnulib support to make it
easier to do this sort of lazier linking.


I was wondering if this was worth worrying about at all,
but it is a significant overhead that's worth improving.
To quantify the overhead I compared optimized builds,
with and without the above configure option, giving:

$ time seq 1 | xargs -I'{}' src/sort /dev/null -k'{}'
real0m7.009s
user0m3.462s
sys 0m3.578s

$ time seq 1 | xargs -I'{}' src/sort-lc /dev/null -k'{}'
real0m12.950s
user0m3.754s
sys 0m9.200s


So we should do something. Now dlopening libcrypto on demand
would work, but there may be better solutions.
sort doesn't have to use md5. It could use blake2 routines
already in coreutils to avoid the issue (and get some speed ups).
Alternatively it might use some other hash function.
For example see the other 128 bit functions compared at:
https://github.com/Cyan4973/xxHash

BTW there was mention of static linking as an option in this thread.
That's is an option to provide better speed an isolation for binaries,
however it's best left to the system builders to use this for their builds.
There can be security implications for prompt library updating,
and libcrypto is particularly sensitive in this regard.


Adding coreutils list...

So above we've demonstrated that sort dynamically loading libcrypto
does nearly double the startup time for the process.

Attached is a patch to use the coreutils reference blake2b hash instead
of the optimized libcrypto md5 routines.

  $ seq 100 > 1.txt

  $ time src/sort-md5-lc -R < 1.txt > /dev/null
  real  0m6.734s
  user  0m23.258s
  sys   0m0.047s

  $ time src/sort-blake2 -R < 1.txt > /dev/null
  real  0m7.215s
  user  0m25.683s
  sys   0m0.043s

  $ grep 'model name' /proc/cpuinfo | head -n1
  model name: Intel(R) Core(TM) i7-5600U CPU @ 2.60GHz
  $ rpm -q openssl-libs
  openssl-libs-3.0.9-2.fc38.x86_64

So while this avoids the startup overhead,
the reference blake2 routines are a little less efficient
than the optimized md5 libcrypto routines.

cheers,
PádraigFrom 7b6765771a235849f424cb6a884cb8237de8380d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= 
Date: Sun, 8 Oct 2023 21:17:09 +0100
Subject: [PATCH] sort: avoid linking with libcrypto

Use blake2b (128 bit) instead of md5
---
 src/blake2/blake2.h |  2 +-
 src/local.mk| 10 ++
 src/sort.c  | 35 +++
 3 files changed, 26 insertions(+), 21 deletions(-)

diff --git a/src/blake2/blake2.h b/src/blake2/blake2.h
index be8b176d7..73bd171b7 100644
--- a/src/blake2/blake2.h
+++ b/src/blake2/blake2.h
@@ -23,7 +23,7 @@
 #ifdef _MSC_VER
 # define BLAKE2_PACKED(x) __pragma (pack (push, 1)) x __pragma (pack (pop))
 #else
-# define BLAKE2_PACKED(x) x _GL_ATTRIBUTE_PACKED
+# define BLAKE2_PACKED(x) x /* _GL_ATTRIBUTE_PACKED */
 #endif
 
 #if defined(__cplusplus)
diff --git a/src/local.mk b/src/local.mk
index ed5d46ddb..35f4d0a35 100644
--- a/src/local.mk
+++ b/src/local.mk
@@ -302,7 +302,6 @@ src_printf_LDADD += $(LIBICONV)
 
 # for libcrypto hash routines
 src_md5sum_LDADD += $(LIB_CRYPTO)
-src_sort_LDADD += $(LIB_CRYPTO)
 src_sha1sum_LDADD += $(LIB_CRYPTO)
 src_sha224sum_LDADD += $(LIB_CRYPTO)
 src_sha256sum_LDADD += $(LIB_CRYPTO)
@@ -409,6 +408,9 @@ src_tac_SOURCES = src/tac.c src/temp-stream.c
 src_tail_SOURCES = src/tail.c src/iopoll.c
 src_tee_SOURCES = src/tee.c src/iopoll.c
 
+src_sort_CPPFLAGS = -DHASH_ALGO_BLAKE2=1 -DHAVE_CONFIG_H $(AM_CPPFLAGS)
+src_sort_SOURCES = src/sort.c $(src_blake2_SOURCES)
+
 src_sum_SOURCES = src/sum.c src/sum.h src/digest.c
 src_sum_CPPFLAGS = -DHASH_ALGO_SUM=1 $(AM_CPPFLAGS)
 
@@ -425,9 +427,9 @@ src_sha384sum_CPPFLAGS = -DHASH_ALGO_SHA384=1 $(AM_CPPFLAGS)
 src_sha512sum_SOURCES = src/digest.c
 src_sha512sum_CPPFLAGS = -DHASH_ALGO_SHA512=1 $(AM_CPPFLAGS)
 src_b2sum_CPPFLAGS = -DHASH_ALGO_BLAKE2=1 -DHAVE_CONFIG_H $(AM_CPPFLAGS)
-src_b2sum_SOURCES = src/digest.c \
-		src/blake2/blake2.h src/blake2/blake2-impl.h \
-		src/blake2/blake2b-ref.c \
+src_blake2_SOURCES = src/blake2/blake2.h src/blake2/blake2-impl.h \
+		 src/blake2/blake2b-ref.c
+src_b2sum_SOURCES = src/digest.c $(src_blake2_SOURCES) \
 		src/blake2/b2sum.c src/blake2/b2sum.h
 
 src_cksum_SOURCES = $(src_b2sum_SOURCES) src/sum.c src/sum.h \
diff --git a/src/sort.c b/src/sort.c
index 5c86b8332..14c3951ce 100644
--- a/src/s

Re: sort and -lm

[Pádraig Brady]

On 07/10/2023 22:29, Paul Eggert wrote:

On 2023-10-07 04:42, Pádraig Brady wrote:


The auto linking is globally controlled with the --with-openssl
cofigure option, but you could build sort (and md5sum)
without that dependency with:

    ./configure ac_cv_lib_crypto_MD5=no


Thanks, I was thinking more along the lines that Bruno suggested, which
to continue to link to libcrypto, but do it with dlopen/dlsym in 'sort'
only when need_random is true.

It's not clear to me offhand whether this should be done entirely in
Coreutils, or whether we should add some Gnulib support to make it
easier to do this sort of lazier linking.


I was wondering if this was worth worrying about at all,
but it is a significant overhead that's worth improving.
To quantify the overhead I compared optimized builds,
with and without the above configure option, giving:

$ time seq 1 | xargs -I'{}' src/sort /dev/null -k'{}'
real0m7.009s
user0m3.462s
sys 0m3.578s

$ time seq 1 | xargs -I'{}' src/sort-lc /dev/null -k'{}'
real0m12.950s
user0m3.754s
sys 0m9.200s


So we should do something. Now dlopening libcrypto on demand
would work, but there may be better solutions.
sort doesn't have to use md5. It could use blake2 routines
already in coreutils to avoid the issue (and get some speed ups).
Alternatively it might use some other hash function.
For example see the other 128 bit functions compared at:
https://github.com/Cyan4973/xxHash

BTW there was mention of static linking as an option in this thread.
That's is an option to provide better speed an isolation for binaries,
however it's best left to the system builders to use this for their builds.
There can be security implications for prompt library updating,
and libcrypto is particularly sensitive in this regard.


Also, why doesn't coreutils/configure's ---with-linux-crypto option
cause 'sort' to use Linux kernel crypto? Is this because the syscall
overhead would be too high and it's significantly faster to do it in
user space?

I naively thought that --with-linux-crypto would mean we wouldn't need
to link to libcrypto. Evidently not.


Well --with-linux-crypto is for specialized setups
and not really for general usage, due to syscall overhead,
differing network syscall class (consider locked down environments),
and generally slower than libcrypto. That was detailed at:
https://git.sv.gnu.org/cgit/gnulib.git/commit/?id=71cc633a0f

cheers,
Pádraig

Re: sort and -lm

[Pádraig Brady]

On 07/10/2023 04:38, Paul Eggert wrote:

On 2023-10-06 19:33, Bruno Haible wrote:

Why would that be a problem? The average run time of a 'sort' invocation
is long enough that an additional link dependency to libm shouldn't matter.


We've avoided -lm in the past, I think more to avoid the dependency.


If you really want to minimize the startup time, you could load the libcrypto,
needed for the option '--random', using dlopen() ? I wouldn't advocate it as
a solution on all platforms, since libltdl surely has its own set of
portability problems. But guarded by a '#ifdef __GLIBC__', why not?


Yes, that sounds like it might be a win too. I hadn't noticed the
dependency on libcrypto (and libz).



FYI.

coreutils auto links with libcrypto >=3 since it's GPL compat,
and quite a bit faster than the fallback routines.
libz is a transitive dependency of libcrypto.

The auto linking is globally controlled with the --with-openssl
cofigure option, but you could build sort (and md5sum)
without that dependency with:

  ./configure ac_cv_lib_crypto_MD5=no

cheers,
Pádraig

Re: ./lib/error.h:410:8: warning: ISO C forbids braced-groups within expressions [-Wpedantic]

[Pádraig Brady]

On 21/09/2023 21:43, Bruno Haible wrote:

Pádraig Brady wrote:

I was trying to use -Wpedantic to check for standards conformance,
where some code compilable on gcc 13 by default, is no longer compilable on gcc 
<= 10.


?? Do you mean, you want to use gcc 13 with some specific options, in order to
test whether gcc 10 can compile the package?

This is not going to work, because we use #if tests on __GNUC__ and
__GNUC_MINOR__ in dozens of places.

The only way to see if gcc 10 can compile the package is to use gcc 10.


Right. gcc 13 vs gcc 10 was just a specific example in this case.

I accept the point that a specific compiler (and flags)
should be presented as early as possible in the build process.

I was thinking more abstractly of adjusting the current
compiler to add extra constraints; -Wpedantic in this case.
It was a little surprising that this was ignored currently,
and the post was more informative/reference than identifying
a specific issue.

cheers,
Pádraig.

Re: ./lib/error.h:410:8: warning: ISO C forbids braced-groups within expressions [-Wpedantic]

[Pádraig Brady]

On 21/09/2023 22:31, Paul Eggert wrote:

On 2023-09-21 13:21, Pádraig Brady wrote:


I was trying to use -Wpedantic to check for standards conformance,
where some code compilable on gcc 13 by default, is no longer compilable
on gcc <= 10.


Like Bruno, I've not had much luck with -Wpedantic for doing that. The
GCC manual even has qualms about it.



  ever since commit:
https://git.sv.gnu.org/gitweb/?p=gnulib.git;a=commitdiff;h=4f6131a786
if the user specifies --Wpedantic, then it's ignored
(and that's not at all obvious as there are early warnings before the pragma).


I tried to work around that problem by installing the attached patch.
Please give it a try. Even if -Wpedantic has problems, it's better if
Gnulib doesn't override the builder unnecessarily.


Yes that works well, and is less surprising.

  $ make CFLAGS='-O2 -Wpedantic -Wno-error' 2>&1 | grep 'src/.*warning:' |
  grep -Ev '_Noreturn|_Static_assert'

  src/tail.c:2211:11: warning: a label can only be part of a statement \
  and a declaration is not a statement [-Wpedantic]

thanks!
Pádraig

Re: ./lib/error.h:410:8: warning: ISO C forbids braced-groups within expressions [-Wpedantic]

[Pádraig Brady]

On 02/09/2023 18:54, Bruno Haible wrote:

Bjarni Ingi Gislason wrote:

While compiling "groff" with gcc (Debian 13.2.0-2) 13.2.0) and "-std=gnu2x":

../lib/openat-die.c: In function 'openat_save_fail':
./lib/error.h:410:8: warning: ISO C forbids braced-groups within expressions 
[-Wpedantic]
   410 |  : ({ \
   |^
./lib/error.h:470:7: note: in expansion of macro '__gl_error_call'
   470 |   __gl_error_call (error, status, __VA_ARGS__)
   |   ^~~
../lib/openat-die.c:37:3: note: in expansion of macro 'error'
37 |   error (exit_failure, errnum,
   |   ^
../lib/openat-die.c: In function 'openat_restore_fail':
./lib/error.h:410:8: warning: ISO C forbids braced-groups within expressions 
[-Wpedantic]
   410 |  : ({ \
   |^
./lib/error.h:470:7: note: in expansion of macro '__gl_error_call'
   470 |   __gl_error_call (error, status, __VA_ARGS__)
   |   ^~~
../lib/openat-die.c:56:3: note: in expansion of macro 'error'
56 |   error (exit_failure, errnum,
   |   ^


The option "-std=gnu2x" is useful; however, the option "-Wpedantic" that you
have enabled is generally much less useful.

This code in lib/error.h exists for the purpose of providing actually
useful warnings about the control flow, such as when a 'break;' statement
in a switch statement has been forgotten. We will not revert or change
this code, just for "-Wpedantic".

GNU extensions like ({...}) have their purpose, and we use them because
they are useful here.

You asked for -Wpedantic warnings; you obtained them.


A recent gotcha I've noticed with -Wpedantic is that it may be ignored.
I.e. testing on gcc 13, and ever since commit:
https://git.sv.gnu.org/gitweb/?p=gnulib.git;a=commitdiff;h=4f6131a786
if the user specifies --Wpedantic, then it's ignored
(and that's not at all obvious as there are early warnings before the pragma).

I was trying to use -Wpedantic to check for standards conformance,
where some code compilable on gcc 13 by default, is no longer compilable on gcc 
<= 10.
A workaround for me for now is to use more specific gcc language conformance 
flags,
and filter to non gnulib code, like:

  $ make CFLAGS='-O2 -Wc99-c11-compat -Wc11-c2x-compat -Wno-error' 2>&1 |
grep 'src/.*warning:' | grep -Ev '_Noreturn|_Static_assert'
  src/tail.c:2211:13: warning: a label can only be part of a statement \
  and a declaration is not a statement [-Wc11-c2x-compat]

cheers,
Pádraig

[PATCH] gnu-web-doc-update: fix updating of manual directory

[Pádraig Brady]

* build-aux/gnu-web-doc-update: Correctly change to the 'manual' directory,
since $tmp is a relative path.  This avoids removing files
outside of the 'manual' directory.  Broken since commit e979787d.
---
 ChangeLog| 7 +++
 build-aux/gnu-web-doc-update | 2 +-
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/ChangeLog b/ChangeLog
index 88e1d80ce7..ac57b7b5bc 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,10 @@
+2023-09-11  Pádraig Brady  
+
+   gnu-web-doc-update: fix updating of manual directory
+   * build-aux/gnu-web-doc-update: Change to the 'manual' directory,
+   since $tmp is a relative path.  This avoids removing files
+   outside of the 'manual' directory.
+
 2023-09-10  Bruno Haible  
 
Fix clang errors "different exception specifier" (regr. 2023-09-04).
diff --git a/build-aux/gnu-web-doc-update b/build-aux/gnu-web-doc-update
index a804031a43..dc13b476ae 100755
--- a/build-aux/gnu-web-doc-update
+++ b/build-aux/gnu-web-doc-update
@@ -182,7 +182,7 @@ $RSYNC -avP "$builddir"/doc/manual/ $tmp/$pkg/manual
   cd $tmp/$pkg
   test -d manual/CVS || $dryrun $CVS add -ko manual
 
-  cd $tmp/$pkg/manual
+  cd manual
 
   # Add all the files.  This is simpler than trying to add only the
   # new ones because of new directories
-- 
2.41.0

Re: readutmp, boot-time: Use the BSD sysctl as fallback

[Pádraig Brady]

On 12/08/2023 14:22, Bruno Haible wrote:

The best approaches to get the boot time access some files. For the
case of programs that execute in Docker containers on Linux, we have
the fallback that relies on the kernel's uptime counter — although it
produces wrong values in a VM that has been put to sleep and then resumed.

Similarly, it is useful to have a fallback for BSD 'jails' on *BSD systems.
There is a sysctl named KERN_BOOTTIME, that coreutils/src/uptime.c already
uses. For consistency between 'who' and 'uptime', it is good to have this
fallback in the 'readutmp' module.

This sysctl exists on macOS, FreeBSD, NetBSD, OpenBSD, GNU/kFreeBSD, Minix.
But note:
   - The boot time that this sysctl reports changes after a VM has been
 put to sleep, then resumed, then its 'date' changed. I've verified
 this on FreeBSD, NetBSD, OpenBSD, GNU/kFreeBSD. It's probably the
 same thing on macOS (but I can't test it).
   - On Minix 3.3, the result of this system call is garbage. When I compile
 the attached program, it produces completely different results when
 invoked as
   $ ./a.out
 vs.
   $ TZ=UTC ./a.out



I notice coreutils CI now failing with:

  In file included from lib/readutmp.c:47:
  /usr/include/x86_64-linux-gnu/sys/sysctl.h:21:2: error:
  #warning "The  header is deprecated and will be removed." 
[-Werror=cpp]
   21 | #warning "The  header is deprecated and will be removed."
  |  ^~~

This deprecation of sysctl on glibc >= 2.30 was dealt with in coreutils before 
like:
https://github.com/coreutils/coreutils/commit/18c938280
Perhaps we should avoid this fallback similarly if defined __GLIBC__ ?

thanks,
Pádraig

Re: Debugging du v9.3

[Pádraig Brady]

On 19/07/2023 19:27, Saulius Krasuckas wrote:

Hello,

I am trying to build Coreutils-9.3 from source. [1]

If I run ./configure && make, it builds.

But I noticed that it calculates some things wrong.  So I decided to
enable some debugging.  Any suggestions?
Since I found none, I looked up at the source and noticed the DU_DEBUG
define: [2]

Now if I clean the dir and run make CFLAGS="-DDU_DEBUG", it fails:

--- snip ---
CC   src/dircolors.o
CCLD src/dircolors
CC   src/dirname.o
CCLD src/dirname
CC   src/du.o
In file included from src/du.c:26:
src/du.c: In function 'du_files':
./lib/config.h:4385:25: warning: implicit declaration of function
'rpl_fts_cross_check'; did you mean 'fts_cross_check'?
[-Wimplicit-function-declaration]
   4385 | #define fts_cross_check rpl_fts_cross_check
| ^~~
src/du.c:60:31: note: in expansion of macro 'fts_cross_check'
 60 | # define FTS_CROSS_CHECK(Fts) fts_cross_check (Fts)
|   ^~~
src/du.c:707:11: note: in expansion of macro 'FTS_CROSS_CHECK'
707 |   FTS_CROSS_CHECK (fts);
|   ^~~
CCLD src/du
/usr/bin/ld: src/du.o: in function `du_files':
du.c:(.text+0x14b5): undefined reference to `rpl_fts_cross_check'
/usr/bin/ld: src/du.o: in function `main':
du.c:(.text+0x16fa): undefined reference to `fts_debug'
collect2: error: ld returned 1 exit status
make[2]: *** [Makefile:10638: src/du] Error 1
make[2]: Leaving directory '/home/sskras/debug/src/coreutils-9.3'
make[1]: *** [Makefile:21297: all-recursive] Error 1
make[1]: Leaving directory '/home/sskras/debug/src/coreutils-9.3'
make: *** [Makefile:8434: all] Error 2
--- snip ---

Is this a bug and/how should I report it?


It looks like an issue with gnulib.
With the attached gnulib patch applied you should be able to build with:

  make CFLAGS="-DDU_DEBUG -DGNULIB_FTS_DEBUG=1 -O2"

Note -O2 is specified to avoid compiler warnings at default optimization levels.

It would be useful to know if fts_cross_check() is useful to you.

cheers,
Pádraig


diff --git a/lib/fts.c b/lib/fts.c
index 884b84a5a1..875fe05793 100644
--- a/lib/fts.c
+++ b/lib/fts.c
@@ -1736,11 +1736,11 @@ fd_ring_print (FTS const *sp, FILE *stream, char const *msg)
 {
   int fd = fd_ring->ir_data[i];
   if (fd < 0)
-fprintf (stream, "%d: %d:\n", i, fd);
+fprintf (stream, "%u: %d:\n", i, fd);
   else
 {
   struct devino wd = getdevino (fd);
-  fprintf (stream, "%d: %d: "PRINT_DEVINO"\n", i, fd, wd.dev, wd.ino);
+  fprintf (stream, "%u: %d: "PRINT_DEVINO"\n", i, fd, wd.dev, wd.ino);
 }
   if (i == fd_ring->ir_back)
 break;
diff --git a/lib/fts_.h b/lib/fts_.h
index fa3d4146e2..82da038a53 100644
--- a/lib/fts_.h
+++ b/lib/fts_.h
@@ -271,6 +271,10 @@ _GL_ATTRIBUTE_NODISCARD
 FTSENT  *fts_read (FTS *) __THROW;
 
 int  fts_set (FTS *, FTSENT *, int) __THROW;
+
+#if GNULIB_FTS_DEBUG
+void fts_cross_check (FTS const *sp);
+#endif
 __END_DECLS
 
 #endif /* fts.h */

Re: uses of xinmalloc and xpmalloc triggered make syntax-check failure

[Pádraig Brady]

On 07/06/2023 05:34, Jim Meyering wrote:

I noticed some syntax-check failures in diffutils.
Its uses of relatively new xinmalloc and xpmalloc functions triggered a
"make syntax-check" failure claiming that xalloc.h need not be included,
because none of its (old list) functions was used. This updates the regex
to match the current list of functions.

I expect to regenerate other syntax-checked header regexps
and may find time to automate the update process.


I think this misses xalloc_die due to the "extern"
adjustments in gnulib 3801e9bb67b.
It seems that the perl regexps in the comment need to be adjusted
to cater for the commented /*extern*/

cheers,
Pádraig

Re: error: Support the compiler's control flow analysis better (was: copy-file: Silence gcc warnings)

[Pádraig Brady]

On 01/06/2023 13:10, Bruno Haible wrote:

Pádraig Brady wrote:

Was there a reason to prefer curly braces there, rather than the more
conventional parentheses?

 '$(error_fns) \(.*%s[:"], .*(name|file)[^"]*\);$$' *.c; \


I had a slight preference for the curly braces since it was used in a shell 
pipeline
and so it's immediately obvious it's a ${variable interpolation}
rather than being misread as a $(command substitution).


Sorry, but I find it as confusing as Jim. Inside the commands of a Makefile 
rule,
   $$x or $${x}   references a shell variable
   $$(x)  does a command substitution
   $(x) or ${x)   references a Makefile variable
   $(x y) or ${x y}   does a GNU make function call
For the latter two, the customary notation is with parentheses.

So, if someone writes  ${x)  or  ${x y}, the reader immediately wonders:
What is the point of the braces? Was it a typo, and they mean $${x} instead?


Fair point.
I've adjusted to consistent $() interpolation now.

cheers,
Pádraig

Re: error: Support the compiler's control flow analysis better (was: copy-file: Silence gcc warnings)

[Pádraig Brady]

On 31/05/2023 18:46, Jim Meyering wrote:

On Wed, May 31, 2023 at 9:12 AM Pádraig Brady  wrote:

-   'error \(.*%s[:"], .*(name|file)[^"]*\);$$' *.c; \
+   '${error_fns} \(.*%s[:"], .*(name|file)[^"]*\);$$' *.c; \


Thanks!
Was there a reason to prefer curly braces there, rather than the more
conventional parentheses?

'$(error_fns) \(.*%s[:"], .*(name|file)[^"]*\);$$' *.c; \


I had a slight preference for the curly braces since it was used in a shell 
pipeline
and so it's immediately obvious it's a ${variable interpolation}
rather than being misread as a $(command substitution).

cheers,
Pádraig

Re: error: Support the compiler's control flow analysis better (was: copy-file: Silence gcc warnings)

[Pádraig Brady]

On 30/05/2023 22:29, Paul Eggert wrote:

On 5/28/23 06:07, Pádraig Brady wrote:

There still is a gotcha (hit in dd.c in coreutils)
where if you define an error macro yourself
you get a macro redefinition error,


I see you fixed that by adding a quick "#define _GL_NO_INLINE_ERROR" to
coreutils/src/dd.c. It's a bit cleaner to fix the underlying naming
problem instead, so that dd.c need not define the Gnulib internals macro
(or its own quirky error macro), so I installed the attached to
coreutils to do that.


I was debating that option but decided against it
as we'd then lose some of the syntax checks on error(args).
But we can augment the syntax checks to cater for this class of function,
which I'm doing as follows.

cheers,
Pádraig

diff --git a/cfg.mk b/cfg.mk
index 263bc0cfd..64db2bec4 100644
--- a/cfg.mk
+++ b/cfg.mk
@@ -189,12 +189,15 @@ sc_prohibit_quotes_notation:
   exit 1; }  \
  || :

+error_fns = (error|die|diagnose)
+
 # Files in src/ should quote all strings in error() output, so that
 # unexpected input chars like \r etc. don't corrupt the error.
 # In edge cases this can be avoided by putting the format string
 # on a separate line to the arguments, or the arguments in parenthesis.
 sc_error_quotes:
-   @cd $(srcdir)/src && GIT_PAGER= git grep -n 'error *(.*%s.*, [^(]*);$$'\
+   @cd $(srcdir)/src \
+ && GIT_PAGER= git grep -E -n '${error_fns} *\(.*%s.*, [^(]*\);$$' \
  *.c | grep -v ', q' \
  && { echo '$(ME): '"Use quote() for error string arguments" 1>&2; \
   exit 1; }  \
@@ -206,7 +209,7 @@ sc_error_quotes:
 sc_error_shell_quotes:
@cd $(srcdir)/src && \
  { GIT_PAGER= git grep -E \
-   'error \(.*%s[:"], .*(name|file)[^"]*\);$$' *.c; \
+   '${error_fns} \(.*%s[:"], .*(name|file)[^"]*\);$$' *.c; \
GIT_PAGER= git grep -E \
' quote[ _].*file' *.c; } \
  | grep -Ev '(quotef|q[^ ]*name)' \
@@ -220,13 +223,13 @@ sc_error_shell_quotes:
 # to provide better support for copy and paste.
 sc_error_shell_always_quotes:
@cd $(srcdir)/src && GIT_PAGER= git grep -E \
-   'error \(.*[^:] %s[ "].*, .*(name|file)[^"]*\);$$' \
+   '${error_fns} \(.*[^:] %s[ "].*, .*(name|file)[^"]*\);$$' \
*.c | grep -Ev '(quoteaf|q[^ ]*name)' \
  && { echo '$(ME): '"Use quoteaf() for space delimited names" 1>&2; \
   exit 1; }  \
  || :
@cd $(srcdir)/src && GIT_PAGER= git grep -E -A1 \
-   'error \([^%]*[^:] %s[ "]' *.c | grep 'quotef' \
+   '${error_fns} \([^%]*[^:] %s[ "]' *.c | grep 'quotef' \
  && { echo '$(ME): '"Use quoteaf() for space delimited names" 1>&2; \
   exit 1; }  \
  || :

Re: error: Support the compiler's control flow analysis better (was: copy-file: Silence gcc warnings)

[Pádraig Brady]

On 28/05/2023 13:50, Pádraig Brady wrote:

On 28/05/2023 13:31, Pádraig Brady wrote:

On 27/05/2023 21:53, Bruno Haible wrote:

+# ifndef _GL_NO_INLINE_ERROR
+#  undef error
+#  define error(status, ...) \
+ ((rpl_error)(0, __VA_ARGS__), (status) ? exit (status) : (void)0)
+# endif


We might need to cast STATUS to bool to avoid the
following failure from coreutils CI:

In file included from src/die.h:22,
from src/chroot.c:27:
src/chroot.c: In function 'main':
src/chroot.c:362:25: error: '?:'using integer constants in boolean context 
[-Werror=int-in-bool-context]
 362 | error (warn ? 0 : EXIT_CANCELED, 0, "%s", (err));
./lib/error.h:422:33: note: in definition of macro 'error'
 422 |  ((error)(0, __VA_ARGS__), (status) ? exit (status) : (void)0)
 | ^~


Actually casting with (bool), or using !! does NOT help here.

It looks to be due to the nested use of ?: that's triggering the issue.

$ gcc --version
gcc (Debian 10.2.1-6) 10.2.1 20210110


To avoid this one can use `(status) != 0`.

There still is a gotcha (hit in dd.c in coreutils)
where if you define an error macro yourself
you get a macro redefinition error,
but that's something we can/should handle on the coreutils side anyway.

cheers,
Pádraig

Re: error: Support the compiler's control flow analysis better (was: copy-file: Silence gcc warnings)

[Pádraig Brady]

On 28/05/2023 13:31, Pádraig Brady wrote:

On 27/05/2023 21:53, Bruno Haible wrote:

+# ifndef _GL_NO_INLINE_ERROR
+#  undef error
+#  define error(status, ...) \
+ ((rpl_error)(0, __VA_ARGS__), (status) ? exit (status) : (void)0)
+# endif


We might need to cast STATUS to bool to avoid the
following failure from coreutils CI:

In file included from src/die.h:22,
   from src/chroot.c:27:
src/chroot.c: In function 'main':
src/chroot.c:362:25: error: '?:'using integer constants in boolean context 
[-Werror=int-in-bool-context]
362 | error (warn ? 0 : EXIT_CANCELED, 0, "%s", (err));
./lib/error.h:422:33: note: in definition of macro 'error'
422 |  ((error)(0, __VA_ARGS__), (status) ? exit (status) : (void)0)
| ^~


Actually casting with (bool), or using !! does NOT help here.

It looks to be due to the nested use of ?: that's triggering the issue.

$ gcc --version
gcc (Debian 10.2.1-6) 10.2.1 20210110

cheers,
Pádraig

Re: error: Support the compiler's control flow analysis better (was: copy-file: Silence gcc warnings)

[Pádraig Brady]

On 27/05/2023 21:53, Bruno Haible wrote:

Paul Eggert wrote:

Maybe by defining
error and error_at_line as inline functions


They're defined by glibc, no? The definitions might collide.


Yes, they are defined in glibc. Overriding functions without causing
collisions is our core expertise here :)


Also, I suppose the compiler might not inline them


Indeed, when I attempt to define
   void inline_error (int status, int errnum, const char *fmt, ...) { ... }
gcc does not inline the function, because it never inlines varargs functions.

An alternative would be to define
   void inline_error (int status, int errnum, const char *message) { ... }
and pass xasprintf (fmt, ...) as message argument. But the out-of-memory
handling or the LGPLv2+ / GPL license difference causes problems.

Fortunately, we don't need an inline function: Jim's die() implementation
shows that it can be done with just a macro definition.


The basic problem is that the old 'error' API doesn't mix well with
[[noreturn]] and the like. We could write a new function, "eexit" say,
that behaves like "error" except it never returns. (I chose the name
"eexit" so as to not mess up the indenting of existing code.)

Or we could just live with "die", as it works.


While this is definitely working, it has the problem that it pulls
developers away from the glibc API. Things are easier for developers
if they can use the symbols from POSIX or glibc rather than gnulib-
invented symbols. And that is possible here.

Also, the name 'die' has the problem that it may invoke traumatic
memories in some people (like the verb 'kill', but we can't get rid
of this one since it's standardized).



+# ifndef _GL_NO_INLINE_ERROR
+#  undef error
+#  define error(status, ...) \
+ ((rpl_error)(0, __VA_ARGS__), (status) ? exit (status) : (void)0)
+# endif


We might need to cast STATUS to bool to avoid the
following failure from coreutils CI:

In file included from src/die.h:22,
 from src/chroot.c:27:
src/chroot.c: In function 'main':
src/chroot.c:362:25: error: '?:'using integer constants in boolean context 
[-Werror=int-in-bool-context]
  362 | error (warn ? 0 : EXIT_CANCELED, 0, "%s", (err));
./lib/error.h:422:33: note: in definition of macro 'error'
  422 |  ((error)(0, __VA_ARGS__), (status) ? exit (status) : (void)0)
  | ^~


cheers,
Pádraig

Re: copy-file: Silence gcc warnings

[Pádraig Brady]

On 26/05/2023 18:34, Bruno Haible wrote:

In GNU gettext, I see these compilation warnings with gcc 13.1.0:

gcc ... -Wall -Wno-cast-qual -Wno-conversion -Wno-float-equal -Wno-sign-compare 
-Wno-undef -Wno-unused-function -Wno-unused-parameter -Wno-float-conversion 
-Wimplicit-fallthrough -Wno-pedantic -Wno-sign-conversion -Wno-type-limits 
-Wno-unsuffixed-float-constants -g -O2 -c copy-file.c  -fPIC -DPIC -o 
.libs/libgettextlib_la-copy-file.o
copy-file.c: In function 'copy_file_preserving':
copy-file.c:192:7: warning: this statement may fall through 
[-Wimplicit-fallthrough=]
   192 |   error (EXIT_FAILURE, errno, _("error while opening %s for 
reading"),
   |   
^~~~
   193 |  quote (src_filename));
   |  ~
copy-file.c:195:5: note: here
   195 | case GL_COPY_ERR_OPEN_BACKUP_WRITE:
   | ^~~~
and so on.


The option -Wimplicit-fallthrough gets added as part of GL_CFLAG_GNULIB_WARNINGS
(collected by gl_CC_GNULIB_WARNINGS in m4/gnulib-common.m4). That explains
why I'm not seeing these warnings in gnulib testdirs.

It's probably too hard to teach gcc that error (EXIT_FAILURE, ...) does not
return but error (0, ...) does return. Therefore just disabling this warning
seems the best action.


FWIW grep/coreutils use a die() wrapper to achieve this
for this common idiom.

https://github.com/coreutils/coreutils/commit/dad7ab0b7

cheers,
Pádraig.

Re: nstrftime.c fails to build due to memset overflow

[Pádraig Brady]

On 19/05/2023 17:39, Bruno Haible wrote:

Pádraig Brady wrote:

I'm going to keep this one.
...
I've lost many hours to analyzing false positives from this one ...
and I've never found a real issue identified by this warning.


But can you please remove the line
   # FP wth -O0 in nstrftime.c w/gcc 12, and 13 at least
since we now have a workaround in gnulib/lib/nstrftime.c ?


Done,

thanks,
Pádraig

Re: [PATCH] hash: pacify gcc -Wsuggest-attribute=const

[Pádraig Brady]

On 19/05/2023 12:03, Bruno Haible wrote:

Hi Pádraig,


diff --git a/lib/hash.c b/lib/hash.c
index 918aa0d1c3..332cca6df9 100644
--- a/lib/hash.c
+++ b/lib/hash.c
@@ -492,7 +492,7 @@ check_tuning (Hash_table *table)
 TUNING, or return 0 if there is no possible way to allocate that
 many entries.  */
  
-static size_t _GL_ATTRIBUTE_PURE

+static size_t _GL_ATTRIBUTE_CONST
  compute_bucket_size (size_t candidate, const Hash_tuning *tuning)
  {
if (!tuning->is_n_buckets)



This looks wrong. Per the definition of the 'const' attribute [1]
   "The const attribute prohibits a function from reading objects that
affect its return value between successive invocations. However,
functions declared with the attribute can safely read objects that
do not change their return value, such as non-volatile constants."
In this function's body, testing tuning->is_n_buckets affects the
return value.

Bruno

[1] 
https://gcc.gnu.org/onlinedocs/gcc-13.1.0/gcc/Common-Function-Attributes.html


I was unsure about that TBH.

In coreutils usage tuning==NULL so the static const default_tuning is passed 
always.
So I guess _GL_ATTRIBUTE_CONST is fine there.

In general for any tuning though, since the tuning param struct is declared 
const,
can the compiler not assume successive invocations against the _same object_
returns the same value?

cheers,
Pádraig

[PATCH] hash: pacify gcc -Wsuggest-attribute=const

[Pádraig Brady]

* lib/hash.c (compute_bucket_size): Change _GL_ATTRIBUTE_PURE
to _GL_ATTRIBUTE_CONST as suggested by GCC 13 with -flto.
---
 ChangeLog  | 6 ++
 lib/hash.c | 2 +-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/ChangeLog b/ChangeLog
index 5e93f9ac31..a57cc4ae59 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,9 @@
+2023-05-19  Pádraig Brady  
+
+   hash: pacify gcc -Wsuggest-attribute=const
+   * lib/hash.c (compute_bucket_size): Change _GL_ATTRIBUTE_PURE
+   to _GL_ATTRIBUTE_CONST as suggested by GCC 13 with -flto.
+
 2023-05-19  Pádraig Brady  
 
modechange: pacify gcc -Wsuggest-attribute=pure
diff --git a/lib/hash.c b/lib/hash.c
index 918aa0d1c3..332cca6df9 100644
--- a/lib/hash.c
+++ b/lib/hash.c
@@ -492,7 +492,7 @@ check_tuning (Hash_table *table)
TUNING, or return 0 if there is no possible way to allocate that
many entries.  */
 
-static size_t _GL_ATTRIBUTE_PURE
+static size_t _GL_ATTRIBUTE_CONST
 compute_bucket_size (size_t candidate, const Hash_tuning *tuning)
 {
   if (!tuning->is_n_buckets)
-- 
2.40.1

[PATCH] modechange: pacify gcc -Wsuggest-attribute=pure

[Pádraig Brady]

* lib/modechange.h (mode_adjust): Add _GL_ATTRIBUTE_PURE
suggested with GCC 12 with -flto.
---
 ChangeLog| 6 ++
 lib/modechange.h | 3 ++-
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/ChangeLog b/ChangeLog
index 8e980741c5..5e93f9ac31 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,9 @@
+2023-05-19  Pádraig Brady  
+
+   modechange: pacify gcc -Wsuggest-attribute=pure
+   * lib/modechange.h (mode_adjust): Add _GL_ATTRIBUTE_PURE
+   suggested with GCC 12 with -flto.
+
 2023-05-18  Bruno Haible  
 
getndelim2: Silence gcc warning.
diff --git a/lib/modechange.h b/lib/modechange.h
index b4fc36a563..d2fbfed082 100644
--- a/lib/modechange.h
+++ b/lib/modechange.h
@@ -32,6 +32,7 @@ struct mode_change *mode_compile (const char *)
 struct mode_change *mode_create_from_ref (const char *)
   _GL_ATTRIBUTE_MALLOC _GL_ATTRIBUTE_DEALLOC_FREE;
 mode_t mode_adjust (mode_t, bool, mode_t, struct mode_change const *,
-mode_t *);
+mode_t *)
+  _GL_ATTRIBUTE_PURE;
 
 #endif
-- 
2.40.1

Re: nstrftime.c fails to build due to memset overflow

[Pádraig Brady]

On 18/05/2023 22:27, Paul Eggert wrote:

Let's revert the "avoid incorrect -Wmaybe-uninitialized warnings" patch.

--enable-gcc-warnings is designed for the default gcc -O2, and we
shouldn't dumb down our source code for lesser platforms like "gcc -O0",
or clang, or whatever.


OK I'll revert.
I didn't think it was too invasive,
but I see your general point about needlessly initializing.

`gcc -O0` or  `gcc -Og` is a very common requirement though
to allow effective debugging with gdb etc.
So we should to do something to make this easier.
Reconfiguring with --enable-gcc-warnings=no is neither efficient or obvious.
It's fine for us who know the build intimately, that we can:
`make CFLAGS='-O0 -ggdb3' WERROR_CFLAGS=`
though that's not obvious to most not familiar with the build,
and also has the disadvantage of distracting warnings being shown.
Perhaps we can create a debug target or something
to make this more discoverable for folks.

We'll also still have the issue with -O3 or -Ofast etc.,
but I suppose they're less likely to be set in dev builds
and so WERROR_CFLAGS= would be implicit for "dist" builds.

cheers,
Pádraig

Re: nstrftime.c fails to build due to memset overflow

[Pádraig Brady]

On 18/05/2023 22:43, Paul Eggert wrote:

--- a/configure.ac
+++ b/configure.ac
@@ -261,6 +261,11 @@ if test $gl_gcc_warnings != no; then
# FP in careadlinkat.c w/gcc 10.0.1 20200205
gl_WARN_ADD([-Wno-return-local-addr])
  
+  # FIXME: remove this line when gcc improves

+  # https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88443
+  # FP wth -O0 in nstrftime.c w/gcc 12, and 13 at least
+  gl_WARN_ADD([-Wno-stringop-overflow])


This patch doesn't look right either. First,
 is a meta-bug
report, and so is too vague for anybody to see what problem the FIXME is
referring to. The FIXME should refer to a specific GCC bug report, and
if no such bug report exists one should be created.

Second, if the bug occurs with -O0 but not with -O2, then gl_WARN_ADD
should be invoked only if -O0 is specified. -Wstringop-overflow is a
useful option and should not be suppressed for ordinary (-O2) builds
simply because there's a problem in -O0 builds.

More generally, I don't think we should spend much time worrying about
configuring with --enable-gcc-warnings and compiling with -O0. With
today's GCC it's better to say, "don't do that". That is, either
configure with a high quality of static checking, with
--enable-gcc-warnings; or configure with easy-to-debug options like
CFLAGS="-O0". The current GCC can't do both well, and there's little
sense making application code less reliable in order to try to make GCC
do a little bit better for this poorly-supported combination.


I'm going to keep this one.

The bug above alludes to the plethora of issues with -Wstringop-overflow,
which is also my experience.
I've lost many hours to analyzing false positives from this one
(in a very large non coreutils code base),
and I've never found a real issue identified by this warning.

cheers,
Pádraig

Re: nstrftime.c fails to build due to memset overflow

[Pádraig Brady]

On 14/03/2023 16:50, Pádraig Brady wrote:

On 14/03/2023 13:55, Marcus Müller wrote:

Dear Gnulib community,

On Linux, x86_64, Fedora 37, ran, on today's coreutils' HEAD (e68b15), which 
submodule-includes gnulib f17d3977:

CFLAGS=-Wno-deprecated-declarations ./configure

(as that CFLAGS is necessary, otherwise sha will fail to build due to using 
deprecated functionality; no big issue).
However, building coreutils fails in gnulib and that does seem to be a 
significant bug:

make -j8 fails with

lib/nstrftime.c: In function '__strftime_internal':
lib/nstrftime.c:147:31: error: 'memset' specified size 18446744073709551615 
exceeds maximum object size 9223372036854775807 [-Werror=stringop-overflow=]


This is triggered by -O2 being implicitly removed when you specified the CFLAGS 
explicitly.
I.e. there are gcc false positives at lower optimization levels.


Actually different -Wmaybe-initialized warnings can trigger at various 
optimization levels.


Also you're building from git, and so will have more strict
developer appropriate warning settings by default
(which can be controlled with the --enable-gcc-warnings=no configure option).

In my experience of this particular "stringop-overflow" warning,
I've had to work around it so many times in a large build system in work
that I disabled it by default in the central build config.
It probably makes sense to disable this in the coreutils settings at least,
which is done in the attached.
The attached also addresses -Wmaybe-initialized warnings in coreutils
that show up at lower optimization levels.


I'm applying the two attached patches to coreutils to address these issues.

cheers,
PádraigFrom b4e20026a49297f024d399f1f49c58dc3134f82b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= 
Date: Thu, 18 May 2023 10:38:11 +0100
Subject: [PATCH 1/2] build: avoid incorrect -Wmaybe-uninitialized warnings

Allow easily building a debug build for example with:
  make CFLAGS='-O0 -ggdb'

False -Wmaybe-uninitialized warnings hit in different
places depending on the compiler passes used.
These changes were tested with gcc 10.2.1, 12.2.1, and 13.1.1 like:
  for o in g s z fast 0 1 2 3; do
make clean && make -j$(nproc) CFLAGS="-O$o" || break
  done

* src/digest.c: Disable -Wmaybe-uninitialized that gives
false positive here at -O0.
* src/ln.c: Avoid -Wmaybe-uninitialized that gives
false positive here at -O1.
* src/pr.c: Likewise.
* src/sort.c: Likewise.
* src/tee.c: Avoid -Wmaybe-uninitialized that gives
false positive here at -O3 on gcc 13.1.1 at least.
* src/cp.c: Avoid -Wmaybe-uninitialized that gives
false positive here at -Os on gcc 13.1.1 at least.
* src/copy.c: Avoid -Wmaybe-uninitialized that gives
false positive here at -Og on gcc 13.1.1 at least.
* src/head.c: Likewise.
* src/paste.c: Likewise.
---
 src/copy.c   | 4 ++--
 src/cp.c | 2 +-
 src/digest.c | 3 +++
 src/head.c   | 2 +-
 src/ln.c | 2 +-
 src/paste.c  | 2 +-
 src/pr.c | 2 +-
 src/sort.c   | 2 +-
 src/tee.c| 2 +-
 9 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/src/copy.c b/src/copy.c
index 0dd059d2e..6b2cf3b29 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -1238,8 +1238,8 @@ copy_reg (char const *src_name, char const *dst_name,
   struct stat const *src_sb)
 {
   char *buf = NULL;
-  int dest_desc;
-  int dest_errno;
+  int dest_desc IF_LINT ( = -1);
+  int dest_errno IF_LINT ( = 0);
   int source_desc;
   mode_t src_mode = src_sb->st_mode;
   mode_t extra_permissions;
diff --git a/src/cp.c b/src/cp.c
index 619eb8260..c952eb397 100644
--- a/src/cp.c
+++ b/src/cp.c
@@ -441,7 +441,7 @@ make_dir_parents_private (char const *const_dir, size_t src_offset,
 
   while ((slash = strchr (slash, '/')))
 {
-  struct dir_attr *new;
+  struct dir_attr *new IF_LINT ( = NULL);
   bool missing_dir;
 
   *slash = '\0';
diff --git a/src/digest.c b/src/digest.c
index ab32968db..8c483a425 100644
--- a/src/digest.c
+++ b/src/digest.c
@@ -1125,6 +1125,9 @@ hex_equal (unsigned char const *hex_digest, unsigned char const *bin_buffer)
   return cnt == digest_bin_bytes;
 }
 
+# if defined __GNUC__ && (__GNUC__ + (__GNUC_MINOR__ >= 7) > 4)
+#  pragma GCC diagnostic ignored "-Wmaybe-uninitialized"
+# endif
 static bool
 digest_check (char const *checkfile_name)
 {
diff --git a/src/head.c b/src/head.c
index c9d3b0d05..f576a2200 100644
--- a/src/head.c
+++ b/src/head.c
@@ -351,7 +351,7 @@ elide_tail_bytes_pipe (char const *filename, int fd, uintmax_t n_elide_0,
  bytes.  Then, for each new buffer we read, also write an old one.  */
 
   bool eof = false;
-  size_t n_read;
+  size_t n_read IF_LINT ( = 0);
   bool buffered_enough;
   size_t i, i_next;
   char **b = NULL;
diff --git a/src/ln.c b/src/ln.c
index 1c3307cac..bfdac0cd9 100644
--- a/src/ln.c
+++ b/src/ln.c
@@ -473,7 +473,7 @@ main (int argc, char **argv)
   char const *backup_suffix = NULL

Re: [PATCH] timespec: fill in other members

[Pádraig Brady]

On 15/05/2023 07:30, Paul Eggert wrote:

This problem was found when compiling GNU Emacs with
--enable-gcc-warnings on a platform where tv_sec is 64 bits and
tv_nsec is 32 bits, and struct timespec has padding.  GCC
-Wuse-of-uninitialized-value complained when a struct timespec
initialized only via assigning to tv_sec and tv_nsec was copied
via assignment (this was in lib/timespec.h’s make_timespec).
Although behavior is well-defined on this platform, the warning is
annoying and the behavior might not be well-defined on theoretical
platforms where struct timespec has other members.  To work around
this, initialize all the struct’s members.



* lib/utimecmp.c (utimecmpat):
* lib/utimens.c (fdutimens):
When filling in a struct timespec or similar time-related structure
that might be copied elsewhere, also assign to any storage other
than tv_sec and tv_nsec, to avoid undefined behavior on (likely
theoretical) platforms where struct timespec has other members,
and also to avoid warnings from GCC and/or valgrind.


The new coreutils CI I have in place is failing to build
with gcc (Debian 10.2.1-6) 10.2.1 with gnulib latest (ebd843b3) as follows.
I think this may be due to https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82283
It's a pity that doesn't seem to have been backported to gcc 10 (2020).
I was thinking I might need to ./configure --enable-gcc-warnings=no for CI
at some stage, but it seems that's sooner rather than later.

It might be worth reverting the change to lib/utimecmp.c
and perhaps instead initializing with `= {0}`.
I confirmed that builds without issue.

cheers,
Pádraig

lib/utimecmp.c: In function 'utimecmpat':
lib/utimecmp.c:348:17: error: missing initializer for field 'tv_nsec' of 
'struct timespec' [-Werror=missing-field-initializers]
  348 | [0].tv_nsec = dst_a_ns,
  | ^
In file included from /usr/include/x86_64-linux-gnu/sys/select.h:39,
 from ./lib/sys/select.h:114,
 from /usr/include/x86_64-linux-gnu/sys/types.h:179,
 from ./lib/sys/types.h:46,
 from lib/utimecmp.h:23,
 from lib/utimecmp.c:22:
/usr/include/x86_64-linux-gnu/bits/types/struct_timespec.h:16:21: note: 
'tv_nsec' declared here
   16 |   __syscall_slong_t tv_nsec; /* Nanoseconds.  */
  | ^~~
lib/utimecmp.c:350:17: error: missing initializer for field 'tv_nsec' of 
'struct timespec' [-Werror=missing-field-initializers]
  350 | [1].tv_nsec = dst_m_ns + res / 9
  | ^
In file included from /usr/include/x86_64-linux-gnu/sys/select.h:39,
 from ./lib/sys/select.h:114,
 from /usr/include/x86_64-linux-gnu/sys/types.h:179,
 from ./lib/sys/types.h:46,
 from lib/utimecmp.h:23,
 from lib/utimecmp.c:22:
/usr/include/x86_64-linux-gnu/bits/types/struct_timespec.h:16:21: note: 
'tv_nsec' declared here
   16 |   __syscall_slong_t tv_nsec; /* Nanoseconds.  */
  | ^~~

Re: coreutils CI

[Pádraig Brady]

On 11/05/2023 15:31, Bruno Haible wrote:

Pádraig Brady wrote:

@Padraig: you're at least doing some pre-release tests on several platforms,
but that's no real CI, is it?


Right.
No real CI at present.
It would be good though, directly for coreutils
and indirectly for gnulib.


We have a CI for gnulib at Gitlab [1], but it detects only failures that are
covered by the unit tests, and — as you know — some of the complicated uses
of gnulib are only covered by coreutils tests, not by gnulib tests.

We have a CI also for a couple of GNU packages at Gitlab [2][3][4][5][6][7][8],
and they occasionally detect regressions. But with only 400 free CI minutes per
month [9], you couldn't have very many "expensive" test runs with coreutils
on that platform, even when using the coreutils mirror that someone already
has set up [10].


I think I'll have a look at setting up
some automated testing on the GCC compile farm.


Reading [11], it looks like this would be feasible, if you pick a machine that
has lots of CPU power and use 'ulimit' "to reduce the risk of a DOS attack".

I agree that it would be good.

Bruno

[1] https://gitlab.com/gnulib/gnulib-ci/-/pipelines
[2] https://gitlab.com/gnu-diffutils/ci-distcheck/-/pipelines
[3] https://gitlab.com/gnu-gettext/ci-distcheck/-/pipelines
[4] https://gitlab.com/gnu-grep/ci-distcheck/-/pipelines
[5] https://gitlab.com/gnu-gzip/ci-distcheck/-/pipelines
[6] https://gitlab.com/gnu-libunistring/ci-distcheck/-/pipelines
[7] https://gitlab.com/gnu-m4/ci-distcheck/-/pipelines
[8] https://gitlab.com/gnu-sed/ci-distcheck/-/pipelines
[9] https://about.gitlab.com/pricing/
[10] https://gitlab.com/saiwp/coreutils
[11] https://gcc.gnu.org/wiki/CompileFarm


OK, a daily coreutils build/test run is now in place
against the latest coreutils and gnulib,
on 1 machine in the compile farm.

Failures currently go to my desktop notifications.

I may get time to expand to multiple machines,
and make the results more consumable.

cheers,
Pádraig

Re: coreutils CI

[Pádraig Brady]

On 11/05/2023 07:58, Bernhard Voelker wrote:

On 5/9/23 20:57, Bruno Haible wrote:

Bernhard Voelker wrote:

I noticed the issue because the following "very-expensive" tests failed
(which succeeded with coreutils-9.3):

 FAIL: tests/rm/ext3-perf
 FAIL: tests/rm/many-dir-entries-vs-OOM


Did you notice these failures by chance, or do you run coreutils continuous
integration on a fixed schedule?


I regularly run the 'check-very-expensive' tests on my local clone.


More generally, for which systems are coreutils continuous integrations being
run regularly? For which systems would it be useful to have it?

https://www.gnu.org/software/coreutils/ references
https://hydra.nixos.org/jobset/gnu/coreutils-master but it seems that this
CI stopped working in 2021.


I know the Hydra tests have stopped working, but some (small) attempts to get
support to get it working again failed.

@Padraig: you're at least doing some pre-release tests on several platforms,
but that's no real CI, is it?


Right.
No real CI at present.
It would be good though, directly for coreutils
and indirectly for gnulib.

I think I'll have a look at setting up
some automated testing on the GCC compile farm.

cheers,
Pádraig

Re: Add more reminders to include

[Pádraig Brady]

On 12/04/2023 18:51, Bruno Haible wrote:

Pádraig Brady wrote:

CC   src/cksum-crctab.o
In file included from ./lib/time.h:50,
from /usr/include/sys/time.h:421,
from ./lib/sys/time.h:39,
from /usr/include/sys/select.h:23,
from ./lib/sys/select.h:36,
from /usr/sfw/lib/gcc/sparc-sun-solaris2.10/3.4.3/include/sys/types.h:631,
from ./lib/sys/types.h:39,
from ./lib/stdint.h:105,
from src/crctab.c:1:
./lib/stddef.h:175: error: syntax error before "void"


Ah, this isn't a gnulib issue.
crctab.c in coreutils didn't include 


While we document that #include  is a necessity, in
<https://www.gnu.org/software/gnulib/manual/html_node/Source-changes.html>,
we don't enforce it well.

It would have been better if, instead of getting a syntax error,
you would have gotten an error "Please include config.h first.".

We already have such errors in a few places, but apparently not in enough
places.

With the attached patch, you would have gotten an error
   "Please include config.h first."
from
   - the included lib/sys/select.h,
   - the included lib/sys/time.h,
   - the included lib/time.h,
   - the included lib/stddef.h
on that particular platform.

It is well possible — and even the intention — that this patch causes
compilation errors in packages that before compiled fine, on a particular
platform. It will force the package maintainers to add '#include '
in some (hopefully most) of the missing places and thus increase the
portability, i.e. reduce the risk of other platform-related compilation
errors.


I like it.
It's better to catch such issues as early as possible.

thanks!
Pádraig

Re: stddef: Define 'unreachable', for ISO C 23 compliance

[Pádraig Brady]

On 10/04/2023 19:05, Pádraig Brady wrote:

On 10/04/2023 18:46, Bruno Haible wrote:

Hi Pádraig,


+/* Declare abort(), without including .  */
+extern
+#  if defined __cplusplus
+"C"
+#  endif
+_Noreturn


The above _Noreturn is causing a build failure on Solaris 10 on Sparc.


A build failure of which package, which version?

Can you please show the tail of the output of "make V=1"? I need to
see whether it's a warning or an error (since I guess that you often
compile with -Werror).


   From config.log:

 $ gcc --version
 $ gcc (GCC) 3.4.3 (csl-sol210-3_4-branch+sol_rpath)


Is this on a publicly accessible machine (e.g. in the GCC compile farm)?
If so, that would speed up my reproduction.


Commenting out that _Noreturn allows the build to complete.


Thanks for the hint; I'll consider this after analyzing.


Sorry for the terse info.
This is for the latest coreutils snapshot on a Solaris 10 machine only I have 
access to.
So a dist tarball, so an error not a warning.
https://www.pixelbeat.org/cu/coreutils-ss.tar.xz

End of build output is:

CC   src/cksum-crctab.o
In file included from ./lib/time.h:50,
   from /usr/include/sys/time.h:421,
   from ./lib/sys/time.h:39,
   from /usr/include/sys/select.h:23,
   from ./lib/sys/select.h:36,
   from 
/usr/sfw/lib/gcc/sparc-sun-solaris2.10/3.4.3/include/sys/types.h:631,
   from ./lib/sys/types.h:39,
   from ./lib/stdint.h:105,
   from src/crctab.c:1:
./lib/stddef.h:175: error: syntax error before "void"


Ah, this isn't a gnulib issue.
crctab.c in coreutils didn't include 

Sorry for the noise.

Pádraig

Re: stddef: Define 'unreachable', for ISO C 23 compliance

[Pádraig Brady]

On 10/04/2023 18:46, Bruno Haible wrote:

Hi Pádraig,


+/* Declare abort(), without including .  */
+extern
+#  if defined __cplusplus
+"C"
+#  endif
+_Noreturn


The above _Noreturn is causing a build failure on Solaris 10 on Sparc.


A build failure of which package, which version?

Can you please show the tail of the output of "make V=1"? I need to
see whether it's a warning or an error (since I guess that you often
compile with -Werror).


  From config.log:

$ gcc --version
$ gcc (GCC) 3.4.3 (csl-sol210-3_4-branch+sol_rpath)


Is this on a publicly accessible machine (e.g. in the GCC compile farm)?
If so, that would speed up my reproduction.


Commenting out that _Noreturn allows the build to complete.


Thanks for the hint; I'll consider this after analyzing.


Sorry for the terse info.
This is for the latest coreutils snapshot on a Solaris 10 machine only I have 
access to.
So a dist tarball, so an error not a warning.
https://www.pixelbeat.org/cu/coreutils-ss.tar.xz

End of build output is:

  CC   src/cksum-crctab.o
In file included from ./lib/time.h:50,
 from /usr/include/sys/time.h:421,
 from ./lib/sys/time.h:39,
 from /usr/include/sys/select.h:23,
 from ./lib/sys/select.h:36,
 from 
/usr/sfw/lib/gcc/sparc-sun-solaris2.10/3.4.3/include/sys/types.h:631,
 from ./lib/sys/types.h:39,
 from ./lib/stdint.h:105,
 from src/crctab.c:1:
./lib/stddef.h:175: error: syntax error before "void"

Re: stddef: Define 'unreachable', for ISO C 23 compliance

[Pádraig Brady]

On 16/03/2023 12:48, Bruno Haible wrote:

ISO C 23 § 7.21.1 requires that  implements 'unreachable'.

This patch does it.


2023-03-16  Bruno Haible  

stddef: Define 'unreachable', for ISO C 23 compliance.
* lib/verify.h (_GL_HAS_BUILTIN_UNREACHABLE): Don't define if already
defined.
* lib/stddef.in.h (_GL_HAS_BUILTIN_UNREACHABLE, unreachable): New
macros.
(abort): Declare if needed for unreachable.
* m4/stddef_h.m4 (gl_STDDEF_H): Test for unreachable.
* tests/test-stddef.c (test_unreachable_optimization,
test_unreachable_noreturn): New functions, based on tests/test-verify.c.
* doc/posix-headers/stddef.texi: Mention unreachable.

diff --git a/doc/posix-headers/stddef.texi b/doc/posix-headers/stddef.texi
index e240f93363..33ad48244c 100644
--- a/doc/posix-headers/stddef.texi
+++ b/doc/posix-headers/stddef.texi
@@ -7,6 +7,10 @@
  
  Portability problems fixed by Gnulib:

  @itemize
+@item
+Some platforms fail to provide @code{unreachable}, which was added in C23:
+GCC 13, clang 15, AIX with xlc 12.1, Solaris with Sun C 5.15, and others.
+
  @item
  Some platforms fail to provide @code{max_align_t}, which was added in C11:
  NetBSD 8.0, Solaris 11.0, and others.
diff --git a/lib/stddef.in.h b/lib/stddef.in.h
index 6eadcc3d5a..9e9d4b7cc6 100644
--- a/lib/stddef.in.h
+++ b/lib/stddef.in.h
@@ -18,7 +18,7 @@
  /* Written by Eric Blake.  */
  
  /*

- * POSIX 2008  for platforms that have issues.
+ * POSIX 2008 and ISO C 23  for platforms that have issues.
   * 
   */
  
@@ -142,6 +142,43 @@ typedef union

  # endif
  #endif
  
+/* ISO C 23 § 7.21.1 The unreachable macro  */

+#ifndef unreachable
+
+/* Code borrowed from verify.h.  */
+# ifndef _GL_HAS_BUILTIN_UNREACHABLE
+#  if defined __clang_major__ && __clang_major__ < 5
+#   define _GL_HAS_BUILTIN_UNREACHABLE 0
+#  elif 4 < __GNUC__ + (5 <= __GNUC_MINOR__)
+#   define _GL_HAS_BUILTIN_UNREACHABLE 1
+#  elif defined __has_builtin
+#   define _GL_HAS_BUILTIN_UNREACHABLE __has_builtin (__builtin_unreachable)
+#  else
+#   define _GL_HAS_BUILTIN_UNREACHABLE 0
+#  endif
+# endif
+
+# if _GL_HAS_BUILTIN_UNREACHABLE
+#  define unreachable() __builtin_unreachable ()
+# elif 1200 <= _MSC_VER
+#  define unreachable() __assume (0)
+# else
+/* Declare abort(), without including .  */
+extern
+#  if defined __cplusplus
+"C"
+#  endif
+_Noreturn


The above _Noreturn is causing a build failure on Solaris 10 on Sparc.
From config.log:

  $ gcc --version
  $ gcc (GCC) 3.4.3 (csl-sol210-3_4-branch+sol_rpath)


+void abort (void)
+#  if defined __cplusplus && (__GLIBC__ >= 2)
+throw ()
+#  endif
+;
+#  define unreachable() abort ()
+# endif
+
+#endif
+
  #  endif /* _@GUARD_PREFIX@_STDDEF_H */
  # endif /* _@GUARD_PREFIX@_STDDEF_H */
  #endif /* __need_XXX */


Commenting out that _Noreturn allows the build to complete.

Re: recommending AC_SYS_YEAR2038_REQUIRED ?

[Pádraig Brady]

On 10/04/2023 14:40, Bruno Haible wrote:

Hi Paul,

In yesterday's changes, you added to the Gnulib documentation, section
"Avoiding the year 2038 problem", wording that

   * explicitly recommends the ‘year2038-required’ module,
 which does AC_REQUIRE([AC_SYS_YEAR2038_REQUIRED]):

 "The Gnulib module ‘year2038-required’ is recommended for any package
  that might be used after the year 2038 on 32-bit platforms."

   * presents the ‘year2038-required’ module as the first and the ‘year2038’
 module (which does AC_REQUIRE([AC_SYS_YEAR2038])) only as the secondary
 option.

I strongly object to this recommendation and presentation.

The reason is that we have three personas:
   - The package maintainer who edits configure.ac.
   - The distro people who create distros comprised of many packages,
 passing appropriate options to 'configure'.
   - The user / sysadmin who installs packages on their systems from source,
 passing appropriate options to 'configure' as well.

If the package maintainer adds an AC_SYS_YEAR2038_REQUIRED invocation to
his package's configure script, or equivalently, if he pulls in the
'year2038-required' module, he is *taking freedom away* from *both* the
distro people and the users/sysadmins.

It is simply the wrong person's decision if the package maintainer
uses the AC_SYS_YEAR2038_REQUIRED macro.

In detail:

When AC_SYS_YEAR2038_REQUIRED is used, the package can no longer be
installed on the following 32-bit platforms/ABIs:

   - Linux with glibc < 2.34 on x86, arm, mips (32-bit or n32 ABI), powerpc,
 sparc, s390, hppa, m68k, sh, csky, microblaze, nios2,
   - Linux/riscv32,
   - Mac OS X on x86 and powerpc,
   - GNU/Hurd/x86,
   - GNU/kFreeBSD/x86,
   - FreeBSD/x86,
   - MidnightBSD/x86,
   - AIX/powerpc,
   - Solaris 10 and 11 on x86 and sparc,
   - Cygwin/x86,
   - Haiku/x86.

Regarding the distro people:

   The outcome of a discussion, about a month or two ago, was AFAIU that
   Linux/x86 and Linux/arm distros have a choice between
 (a) enabling 64-bit time_t for all packages, thus breaking ABI
 compatibility once and becoming year 2038 saft, or
 (b) staying with the 32-bit time_t, and announcing that their
 distro will stop working in 2038.
   An incremental or partial move to 64-bit time_t would be too expensive,
   did the distro people say.

   Now, if a package maintainer adds AC_SYS_YEAR2038_REQUIRED to their
   configure.ac, they are telling the distros of type (b) "you cannot use
   new releases of my package any more".

   This is discriminatory and without justification, since the maintainers
   of a type (b) distro already know that they have to limit the lifetime
   expectations of their distro.

Regarding the users/sysadmins:

   It is their decision to know until when they want to use their hardware,
   and which ABI they want to use for the binaries that they install on
   this hardware.

   Now, if a package maintainer adds AC_SYS_YEAR2038_REQUIRED to their
   configure.ac, they are telling the users "you cannot install releases
   of my package any more" or "I force you to install 64-bit binaries
   instead of 32-bit binaries".

   It is just not appropriate for the package maintainer to push such a
   decision onto the user.

I would therefore propose that the module 'year2038-required' gets
removed from Gnulib, as I cannot see any positive uses of it.

If that is not possible, then at least:
   1) The Gnulib documentation should present 'year2038' first, and
  'year2038-required' as a rarely needed alternative,
   2) The Gnulib documentation should not recommend 'year2038-required'.

Note: In constrast, there is nothing wrong with the Autoconf documentation.
It presents the AC_SYS_YEAR2038 and AC_SYS_YEAR2038_REQUIRED macros
without favouring one or the other.


Yes I'm a bit confused with the new restrictions myself.
A build on Solaris 10 on sparc for example now fails.
I was able to get a build to complete though by passing this to configure:

  ac_year2038_required=no

I'm confused also about the auto detection / rejection modes for this.
IIUC, previously a build would fail only if `touch -t` could create 64 bit 
times,
while the build was setup for 32 bit time_t. But now the build fails
irrespective of what touch supports. For completeness touch on this platform
does not support 64 bit times.

  > touch -t 203901010101 file.t
  touch: bad time specification

  > touch -t 203701010101 file.t

  > src/touch -t 203901010101 file.t
  touch: invalid date format '203901010101'

  > src/touch -t 203701010101 file.t

cheers,
Pádraig

Re: bug#62607: cp --recursive --backup broken in 9.2

[Pádraig Brady]

On 03/04/2023 18:19, Pádraig Brady wrote:

On 02/04/2023 13:40, Pádraig Brady wrote:

For completeness the correct repro is:

 mkdir -p {src,dst}/foo
 touch {src,dst}/foo/bar
 cp --recursive --backup src/* dst


The attached two patches should address this.
The first fixes the bug in gnulib (cc'd),
while the second adds a test to coreutils.


Pushed.
Marking this as done.

thanks,
Pádraig

Re: bug#62607: cp --recursive --backup broken in 9.2

[Pádraig Brady]

On 02/04/2023 13:40, Pádraig Brady wrote:

On 01/04/2023 23:40, Kristian Klausen via GNU coreutils Bug Reports wrote:

Hi

After upgrading to coreutils 9.2-2 on Arch Linux the following:
mkdir -p src dst
touch {src,dst}/bar
cp --recursive --backup src/* dst
fails with:
cp: cannot create regular file 'dst/foo/bar': File exists

Running strace on cp I noticed:
renameat2(4, "foo/bar", 4, "foo/bar~", 0) = -1 ENOENT (No such file or
directory)

In coreutils 9.1-3 the syscall succeeds:
renameat2(4, "bar", 4, "bar~", 0)   = 0

I assume renameat2 is called with the wrong oldpath and newpath in 9.2
and that it should just be the basename and not the full relative path.

Cheers
Kristian Klausen


Your analysis is correct wrt the wrong paths being given to the renameat2().
This is related to https://bugs.gnu.org/55029

For completeness the correct repro is:

mkdir -p {src,dst}/foo
touch {src,dst}/foo/bar
cp --recursive --backup src/* dst


The attached two patches should address this.
The first fixes the bug in gnulib (cc'd),
while the second adds a test to coreutils.

thanks,
Pádraig
From 418aa564ebff70c1d118a5d3307a6d0b147ff7a2 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= 
Date: Mon, 3 Apr 2023 18:06:22 +0100
Subject: [PATCH] backupfile: fix bug when renaming from subdirectory

* lib/backupfile.c (backup_internal): Ensure we use the
appropriate offset if operating on a subdirectory,
i.e., on an updated sdir.
Fixes https://bugs.gnu.org/62607
---
 ChangeLog| 8 
 lib/backupfile.c | 7 ---
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index ae851ed8aa..69c19b3ebe 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,11 @@
+2023-04-03  Pádraig Brady  
+
+	backupfile: fix bug when renaming from subdirectory
+	* lib/backupfile.c (backup_internal): Ensure we use the
+	appropriate offset if operating on a subdirectory,
+	i.e., on an updated sdir.
+	Fixes https://bugs.gnu.org/62607
+
 2023-04-03  Bruno Haible  
 
 	vasnprintf-posix: Fix harmless mistake (regression 2023-03-24).
diff --git a/lib/backupfile.c b/lib/backupfile.c
index 9cca271343..5bcf924414 100644
--- a/lib/backupfile.c
+++ b/lib/backupfile.c
@@ -331,7 +331,7 @@ backupfile_internal (int dir_fd, char const *file,
 return s;
 
   DIR *dirp = NULL;
-  int sdir = dir_fd;
+  int sdir = -1;
   idx_t base_max = 0;
   while (true)
 {
@@ -370,9 +370,10 @@ backupfile_internal (int dir_fd, char const *file,
   if (! rename)
 break;
 
-  idx_t offset = backup_type == simple_backups ? 0 : base_offset;
+  dir_fd = sdir < 0 ? dir_fd : sdir;
+  idx_t offset = sdir < 0 ? 0 : base_offset;
   unsigned flags = backup_type == simple_backups ? 0 : RENAME_NOREPLACE;
-  if (renameatu (sdir, file + offset, sdir, s + offset, flags) == 0)
+  if (renameatu (dir_fd, file + offset, dir_fd, s + offset, flags) == 0)
 break;
   int e = errno;
   if (! (e == EEXIST && extended))
-- 
2.26.2

From 1a80fab339d52db7e284b4f2f41068d5d8dd7e4e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= 
Date: Mon, 3 Apr 2023 18:12:33 +0100
Subject: [PATCH] tests: cp: test --backup with subdirectories

* tests/cp/backup-dir.sh: Add a test to ensure
we rename appropriately when backing up through subdirs.
* NEWS: Mention the bug fix.
Addresses https://bugs.gnu.org/62607
---
 NEWS   | 5 +
 tests/cp/backup-dir.sh | 8 +++-
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/NEWS b/NEWS
index f53adab6f..59c6da5a7 100644
--- a/NEWS
+++ b/NEWS
@@ -10,6 +10,11 @@ GNU coreutils NEWS-*- outline -*-
   more restricted systems like android or containers etc.
   [bug introduced in coreutils-9.2]
 
+  cp --recursive --backup will again operate correctly.
+  Previousy it may have hit "File exists" errors when
+  it failed to appropriately rename files being replaced.
+  [bug introduced in coreutils-9.2]
+
   date --file and dircolors will now diagnose a failure to read a file.
   Previously they would have silently ignored the failure.
   [This bug was present in "the beginning".]
diff --git a/tests/cp/backup-dir.sh b/tests/cp/backup-dir.sh
index 6573d58e0..5c17498cf 100755
--- a/tests/cp/backup-dir.sh
+++ b/tests/cp/backup-dir.sh
@@ -1,5 +1,5 @@
 #!/bin/sh
-# Ensure that cp -b doesn't back up directories.
+# Ensure that cp -b handles directories appropriately
 
 # Copyright (C) 2006-2023 Free Software Foundation, Inc.
 
@@ -29,4 +29,10 @@ cp -ab x y || fail=1
 test -d y/x || fail=1
 test -d y/x~ && fail=1
 
+# Bug 62607.
+# This would fail to backup using rename, and thus fail to replace the file
+mkdir -p {src,dst}/foo || framework_failure_
+touch {src,dst}/foo/bar || framework_failure_
+cp --recursive --backup src/* dst || fail=1
+
 Exit $fail
-- 
2.26.2

Re: *printf-posix: ISO C 23: Add %b directive for binary output of integers

[Pádraig Brady]

On 17/03/2023 21:51, Bruno Haible wrote:

This set of patches adds support for the %b format string directive, required
by ISO C 23, to the *printf family of functions.

So far, only glibc implements it (since version 2.35). For portability to the
other platforms, use the *printf-posix modules.


For my reference mainly...

It's interesting that this conflicts with the POSIX specified %b directive
for the printf(1) utility.

cheers,
Pádraig

Re: nstrftime.c fails to build due to memset overflow

[Pádraig Brady]

On 14/03/2023 13:55, Marcus Müller wrote:

Dear Gnulib community,

On Linux, x86_64, Fedora 37, ran, on today's coreutils' HEAD (e68b15), which 
submodule-includes gnulib f17d3977:

CFLAGS=-Wno-deprecated-declarations ./configure

(as that CFLAGS is necessary, otherwise sha will fail to build due to using 
deprecated functionality; no big issue).
However, building coreutils fails in gnulib and that does seem to be a 
significant bug:

make -j8 fails with

lib/nstrftime.c: In function '__strftime_internal':
lib/nstrftime.c:147:31: error: 'memset' specified size 18446744073709551615 
exceeds maximum object size 9223372036854775807 [-Werror=stringop-overflow=]


This is triggered by -O2 being implicitly removed when you specified the CFLAGS 
explicitly.
I.e. there are gcc false positives at lower optimization levels.

Also you're building from git, and so will have more strict
developer appropriate warning settings by default
(which can be controlled with the --enable-gcc-warnings=no configure option).

In my experience of this particular "stringop-overflow" warning,
I've had to work around it so many times in a large build system in work
that I disabled it by default in the central build config.
It probably makes sense to disable this in the coreutils settings at least,
which is done in the attached.
The attached also addresses -Wmaybe-initialized warnings in coreutils
that show up at lower optimization levels.

cheers,
Pádraigdiff --git a/configure.ac b/configure.ac
index 2b2f9468d..f33c10ccc 100644
--- a/configure.ac
+++ b/configure.ac
@@ -262,6 +262,10 @@ if test $gl_gcc_warnings != no; then
   # FP in careadlinkat.c w/gcc 10.0.1 20200205
   gl_WARN_ADD([-Wno-return-local-addr])
 
+  # FIXME: remove this line when gcc -O0 improves
+  # FP in nstrftime.c w/gcc 12.2.1 20221121
+  gl_WARN_ADD([-Wno-stringop-overflow])
+
   gl_MANYWARN_COMPLEMENT([GNULIB_WARN_CFLAGS], [$WARN_CFLAGS], [$nw])
   AC_SUBST([GNULIB_WARN_CFLAGS])
 
diff --git a/src/digest.c b/src/digest.c
index 6ee8a4854..3c1d27f9a 100644
--- a/src/digest.c
+++ b/src/digest.c
@@ -1125,6 +1125,7 @@ hex_equal (unsigned char const *hex_digest, unsigned char const *bin_buffer)
   return cnt == digest_bin_bytes;
 }
 
+# pragma GCC diagnostic ignored "-Wmaybe-uninitialized"
 static bool
 digest_check (char const *checkfile_name)
 {
diff --git a/src/pr.c b/src/pr.c
index 28a695242..75ce756ae 100644
--- a/src/pr.c
+++ b/src/pr.c
@@ -2422,7 +2422,7 @@ static bool
 read_line (COLUMN *p)
 {
   int c;
-  int chars;
+  int chars=0;
   int last_input_position;
   int j, k;
   COLUMN *q;
diff --git a/src/sort.c b/src/sort.c
index 8ca7a88c4..15f06e7d8 100644
--- a/src/sort.c
+++ b/src/sort.c
@@ -1045,7 +1045,7 @@ pipe_fork (int pipefds[2], size_t tries)
   struct tempnode *saved_temphead;
   int saved_errno;
   double wait_retry = 0.25;
-  pid_t pid;
+  pid_t pid = -1;
   struct cs_status cs;
 
   if (pipe2 (pipefds, O_CLOEXEC) < 0)

Re: bug#61386: [PATCH] cp,mv,install: Disable sparse copy on macOS

[Pádraig Brady]

On 06/03/2023 07:37, Paul Eggert wrote:

I recall reading somewhere in this thread that a 'split' test was
failing on macOS because it doesn't let you lseek on /dev/null. I fixed
that problem here:

https://git.savannah.gnu.org/cgit/coreutils.git/commit/?id=aa266f1b3dc4e12acdc46cc0f562adc03c2c0b8f

and fixed some other 'split' issues in adjacent patches, while I was in
the neighborhood.


The lseek /dev/null issue was only in your undef SEEK_DATA test patch,
and already addressed in your final gnulib patch in the area, as discussed at:
https://lists.gnu.org/archive/html/bug-coreutils/2023-02/msg00081.html

Also immediately rejecting input where we can't determine the size is a feature.
I.e. the following is the expected behavior:

  $ : | split -n l/1
  split: -: cannot determine file size

With the changes we now have:

  $ : | split -n l/1  # Creates an empty file
  $ yes | split -n l/1
  split: -: cannot determine file size

This is inconsistent, and an insidious issue that users may introduce to 
scripts,
that will only fail once input data hits a certain size.

Also there are a few `make syntax-check` issues in the new split code.

Would it be possible to revert this change in isolation?

thanks,
Pádraig

Re: 回复: lib/fts.c: return when malloc failed

[Pádraig Brady]

On 27/02/2023 11:36, ChuanGang Jiang wrote:

I found this by accident and then reproduce it through artificial mem pressure 
test.
And I update the patch as you said.

*lib/fts.c：return when malloc failed in function setup_dir()
---
  lib/fts.c | 6 +-
  1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/lib/fts.c b/lib/fts.c
index 78584b2902..c2fa5ee8dc 100644
--- a/lib/fts.c
+++ b/lib/fts.c
@@ -979,7 +979,11 @@ next:   tmp = p;
  }
  free_dir(sp);
  fts_load(sp, p);
-setup_dir(sp);
+if (! setup_dir(sp)) {
+free_dir(sp);
+__set_errno (ENOMEM);
+return (NULL);
+}
  goto check_for_dir;
  }



Pushed.
I'll also apply this to the upcoming coreutils release for rm.

thanks,
Pádraig.

Re: lib/fts.c: return when malloc failed

[Pádraig Brady]

On 26/02/2023 14:46, ChuanGang Jiang wrote:

Hi，‘rm’ of coreutils was terminated with signal SIGSEGV, Segmentation fault.
Here is the backtrace with gdb.

Core was generated by `/usr/bin/rm -rf test1/test2/'.
Program terminated with signal SIGSEGV, Segmentation fault.
#0  cycle_check (state=0x0, sb=sb@entry=0x56296ac29e00) at 
../lib/cycle-check.c:60
60assure (state->magic == CC_MAGIC);
(gdb) bt full
#0  cycle_check (state=0x0, sb=sb@entry=0x56296ac29e00) at 
../lib/cycle-check.c:60
 __PRETTY_FUNCTION__ = "cycle_check"
#1  0x56296aa3d8bd in enter_dir (fts=0x56296ac28ae0, ent=0x56296ac29d90) at 
../lib/fts-cycle.c:108
 st = 
 ad = 
 ad_from_table = 
#2  0x56296aa3f031 in rpl_fts_read (sp=sp@entry=0x56296ac28ae0) at 
../lib/fts.c:1024
 p = 0x56296ac29d90
 tmp = 
 instr = 
 t = 
#3  0x56296aa39858 in rm (file=, x=0x7ffc10c71c60) at 
../src/remove.c:597
 ent = 
 s = 
 bit_flags = 
 fts = 0x56296ac28ae0
 rm_status = RM_OK
 __PRETTY_FUNCTION__ = "rm"
#4  0x56296aa388e2 in main (argc=, argv=0x7ffc10c71e88) at 
../src/rm.c:370
 preserve_root = true
 x = {ignore_missing_files = true, interactive = RMI_NEVER, one_file_system = 
false, recursive = true, remove_empty_directories = false, root_dev_ino = 
0x56296aa480b0 , preserve_all_root = false,
   stdin_tty = true, verbose = false, require_restore_cwd = false}
 prompt_once = 
 c = 
 n_files = 
 file = 0x7ffc10c71e98
 status = 
 __PRETTY_FUNCTION__ = "main"


I think it should return when malloc failed for ’fts->fts_cycle.state‘ in 
’setup_dir(sp)‘
and the patch below may fix this.


*lib/fts.c：return when malloc failed in function setup_dir()
---
  lib/fts.c | 5 -
  1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/lib/fts.c b/lib/fts.c
index 78584b2902..374efa1be7 100644
--- a/lib/fts.c
+++ b/lib/fts.c
@@ -979,7 +979,10 @@ next:   tmp = p;
  }
  free_dir(sp);
  fts_load(sp, p);
-setup_dir(sp);
+if (! setup_dir(sp)) {
+free_dir(sp);
+return (NULL);
+}
  goto check_for_dir;
  }



I agree that assertion may trigger if setup_dir() fails.
free_dir() is safe to call upon setup_dir() failure,
so the patch looks correct.
The only way that setup_dir() can really fail is due to no memory,
so I'd also __set_errno (ENOMEM); in this case.

How did you notice this?
Did you apply artificial mem pressure for testing?

thanks!
Pádraig

Re: bug#61386: [PATCH] cp,mv,install: Disable sparse copy on macOS

[Pádraig Brady]

On 24/02/2023 23:23, George Valkov wrote:

Are we in a state where I can update OpenWRT to the latest coreutils master or 
perhaps it would be better to wait until you fix the tests and coreutils-9.2 is 
released? You mentioned the release is coming soon?


Probably best to wait for the 9.2 release at this stage.
Best guess is 1.5 weeks away.

cheers,
Pádraig.

Re: bug#61386: [PATCH] cp,mv,install: Disable sparse copy on macOS

[Pádraig Brady]

On 24/02/2023 22:06, George Valkov wrote:

If I revert a0803c4bad6f8e11bb05effcc42ef433f4fc3f9b, the requirement to press 
enter after PASS: tests/rm/isatty.sh is fixed.



Ah very useful info.
I'll test this on my macOS 13.2 system.


Sometimes it might randomly hang: gdb after
PASS: tests/rm/r-4.sh
^Cmake[1]: *** Deleting file 'tests/rm/r-root.log'
^C^C^C


This is the same gdb issue, which I'll skip similarly
to the tail-2/inotify tests.


Since all issues related to sparse copy on macOS are now addressed,
I'm marking this bug as done.

The above bugs are unrelated, and I'll take them from here.

thanks again for all the testing.

Pádraig

Re: bug#61386: [PATCH] cp,mv,install: Disable sparse copy on macOS

[Pádraig Brady]

On 24/02/2023 14:33, George Valkov wrote:



On 2023-02-24, at 12:23 AM, Paul Eggert  wrote:

On 2/20/23 13:14, Pádraig Brady wrote:

Since https://github.com/coreutils/gnulib/commit/4db8db34
gnulib has been unconditionally replacing lseek() on macos.
Now with SEEK_DATA undefined that replaced lseek()
will run the code intended for BeOS.
So either the lseek.m4 or lseek.c needs adjusting accordingly.


Good catch, thanks. I updated the Gnulib patch accordingly; see attached.


Nice: I just downloaded a fresh copy of Savannah, and it already includes the 
attached patch, so no action needed on my side.

The copy created by cp is good. I noticed that you have added a —debug switch, 
so I gave it a test:
1. If the target exists I get this output. I would assume zeroes means that all 
data is read from the source, but pages containing only zeroes are skipped, 
while pages containing data are written to the target, which is fine. I would 
guess 4 KB page granularity or some form of detection takes place.


Exactly.


./coreutils-2023-02-24/src/cp --debug cc1-mmap cc1-ori
'cc1-mmap' -> 'cc1-ori'
copy offload: avoided, reflink: unsupported, sparse detection: zeros

2. If the target does not exist, the file is cloned. You might want to report 
something about that in the debug output. Otherwise the clone is good.

./coreutils-2023-02-24/src/cp --debug cc1-mmap cc1-ori
'cc1-mmap' -> 'cc1-ori’


Yes that was a mistake.
Should be fixed with
https://git.sv.gnu.org/gitweb/?p=coreutils.git;a=commitdiff;h=65bb27656


My first attempt to run the tests stopped here, so I had to interrupt it 10 
minutes later with Control+C.



^Cmake[1]: *** Deleting file 'tests/tail-2/inotify-race.log'
^C^C^C^C^C^C

killall gdb
ps -A |grep gdb
29584 ?? 0:00.09 gdb -nx --batch-silent --eval-command=break 1475 
--eval-command=run --pid=29583 -f file --eval-command=quit tail
23269 ttys0100:00.09 gdb -nx --batch-silent --eval-command=break 1475 
--eval-command=run --pid=23268 -f file --eval-command=quit tail

killall -9 gdb
ps -A |grep gdb

Killing gdb allowed the tests to continue, I had to do it twice:


That's awkward.
That test documents the issue with protecting the gdb invocation with 
timeout(1).
I suppose we could restrict this test to inotify capable systems,
which would have the side effect of avoiding its use on non linux systems.
The attached patch does that.

thanks for all the testing.

PádraigFrom 952c8bad4c297ed48cddbb81f1030a35812ca980 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= 
Date: Fri, 24 Feb 2023 15:40:37 +
Subject: [PATCH] tests: avoid gdb hang on macOS

* tests/tail-2/inotify-race.sh: Restrict the test to
inotify capable systems to avoid the hang with some gdbs.
* tests/tail-2/inotify-race.sh: Likewise.
---
 tests/tail-2/inotify-race.sh  | 3 +++
 tests/tail-2/inotify-race2.sh | 3 +++
 2 files changed, 6 insertions(+)

diff --git a/tests/tail-2/inotify-race.sh b/tests/tail-2/inotify-race.sh
index c722fb9fa..63f906536 100755
--- a/tests/tail-2/inotify-race.sh
+++ b/tests/tail-2/inotify-race.sh
@@ -23,6 +23,9 @@
 . "${srcdir=.}/tests/init.sh"; path_prepend_ ./src
 print_ver_ tail sleep
 
+grep '^#define HAVE_INOTIFY 1' "$CONFIG_HEADER" >/dev/null && is_local_dir_ . \
+  || skip_ 'inotify is not supported'
+
 # Terminate any background gdb/tail process
 cleanup_() {
   kill $pid 2>/dev/null && wait $pid
diff --git a/tests/tail-2/inotify-race2.sh b/tests/tail-2/inotify-race2.sh
index 89b02c6cf..19219b72e 100755
--- a/tests/tail-2/inotify-race2.sh
+++ b/tests/tail-2/inotify-race2.sh
@@ -22,6 +22,9 @@
 . "${srcdir=.}/tests/init.sh"; path_prepend_ ./src
 print_ver_ tail sleep
 
+grep '^#define HAVE_INOTIFY 1' "$CONFIG_HEADER" >/dev/null && is_local_dir_ . \
+  || skip_ 'inotify is not supported'
+
 # Terminate any background gdb/tail process
 cleanup_() {
   kill $pid 2>/dev/null && wait $pid
-- 
2.26.2

Re: bug#61386: [PATCH] cp,mv,install: Disable sparse copy on macOS

[Pádraig Brady]

On 23/02/2023 22:23, Paul Eggert wrote:

On 2/20/23 13:14, Pádraig Brady wrote:

Since https://github.com/coreutils/gnulib/commit/4db8db34
gnulib has been unconditionally replacing lseek() on macos.
Now with SEEK_DATA undefined that replaced lseek()
will run the code intended for BeOS.
So either the lseek.m4 or lseek.c needs adjusting accordingly.


Good catch, thanks. I updated the Gnulib patch accordingly; see attached.


Cool.


On 2/20/23 02:21, George Valkov wrote:
  > What is the correct way to apply your patch?

Sounds like patching is a bit of a hassle, so to make things easier I
installed the attached patch into Gnulib, and propagated this into
Coreutils.


The latest coreutils that should include all fixes for this issue is at
https://www.pixelbeat.org/cu/coreutils-9.1.160-5c8c2.tar.gz
That should be buildable with the standard ./configure && make combo

thanks!,
Pádraig

Re: bug#61386: [PATCH] cp,mv,install: Disable sparse copy on macOS

[Pádraig Brady]

On 20/02/2023 19:35, George Valkov wrote:



On 2023-02-20, at 7:49 PM, Pádraig Brady  wrote:

On 20/02/2023 15:02, George Valkov wrote:

Hi Paul, the following tests fail after your patch:
FAIL: tests/split/filter.sh
FAIL: tests/split/b-chunk.sh
FAIL: tests/split/l-chunk.sh


These look unrelated and may be due to some other change in your environment.
Specifically lseek() is failing on /dev/null and returning:
split: /dev/null: cannot determine file size


I deleted the lines that were introduced by the patch in unistd.in.h, then make 
clean; make -j 16 and ran all tests: back to 5 failed. Then I restored the 
deleted lines, rebuild and I got 9 failed tests again.


Oh right sorry.
I think I see what's happening.
Since https://github.com/coreutils/gnulib/commit/4db8db34
gnulib has been unconditionally replacing lseek() on macos.
Now with SEEK_DATA undefined that replaced lseek()
will run the code intended for BeOS.
So either the lseek.m4 or lseek.c needs adjusting accordingly.

cheers,
Pádraig

Re: bug#61386: [PATCH] cp,mv,install: Disable sparse copy on macOS

[Pádraig Brady]

On 20/02/2023 15:02, George Valkov wrote:

Hi Paul, the following tests fail after your patch:
FAIL: tests/split/filter.sh
FAIL: tests/split/b-chunk.sh
FAIL: tests/split/l-chunk.sh


These look unrelated and may be due to some other change in your environment.
Specifically lseek() is failing on /dev/null and returning:
split: /dev/null: cannot determine file size


FAIL: tests/cp/sparse-perf.sh


As expected.
I've a fix for this locally
which leverages another patch to determine
dynamically if we support SEEK_HOLE.

cheers,
Pádraig

Re: bug#61386: [PATCH] cp,mv,install: Disable sparse copy on macOS

[Pádraig Brady]

On 19/02/2023 06:08, Paul Eggert wrote:

George, given what you've written I suppose we should give up the idea
of copying sparse files efficiently on macOS (and on FreeBSD
13.0-RELEASE, as it has a similar bug with SEEK_HOLE and SEEK_DATA), in
cases where fclonefileat does not work.

Please try the attached Gnulib patch; it uses your idea of disabling
SEEK_HOLE and SEEK_DATA on macOS, except on steroids because it disables
these two macros everywhere, for all Gnulib-using applications, and it
also disables them on FreeBSD < 14. For coreutils you need only this
patch's change to lib/unistd.in.h. If this works for you I suppose we
can install it into Gnulib and propagate that into coreutils etc.

Also, you reported the bug against macOS 12.6. Can you check whether the
same bug occurs on macOS 13? If it's still there I suppose the attached
patch will need to be updated since it guesses the bug is fixed in macOS 13.

I'll cc this email to bug-gnulib to give them a heads-up. (Gnulib
readers can see  for context.)

Really, Apple should fix this serious data corruption bug in APFS and macOS.


Yes I concur.
I'll adjust the coreutils tests to avoid
using SEEK_DATA if not available in a more dynamic manner.

thank you,
Pádraig

Re: Improve support for ACLs in coreutils (ls & chmod) following the Solaris way

[Pádraig Brady]

On 16/01/2023 15:03, Ondrej Valousek wrote:

Hi,

As per our conversation with Bruno I was thinking if it would make a sense to extend support of 
ACLs in gnulib/coreutils, mainly covering "ls" (1st stage) and "chmod" (2nd 
stage)  with the goal to have the ACLs better understandable for end users.

For "ls" we would:
- Introduce a new flag "-V" that would work like "-l" but also append text 
interpretation of ACLs as in Solaris, i.e.:
# ls -V
total 7
-rw-r--r--+  1 root root   5 Jan  4 09:11 acl
 user:ondrej:rwx---:---:allow
  owner@:rw-p--aARWcCos:---:allow
  group@:r-a-R-c--s:---:allow
   everyone@:r-a-R-c--s:---:allow

For "chmod" we would add new option "A" that would allow modify ACEs like in 
Solaris:
# chmod A+user:marks:rw- file.1

Technical implementation:
- I'd like to support NFSv4 ACLs, but since we have no library for it, then we would need 
to provide some parsing code for it and stick in Gnulib - we have something in 
"file-has-acl.c" already and it would be a good starting point.
- file_has_acl() function would need to be modified slightly to return 2 in 
case NFSv4 acls were found (this is backward compatible).

For Posix acls we would use the existing libacl.

Is this something I would find support in both coreutils and Gnulib?
Thanks


Maybe, though I'm not convinced about adding to ls and chmod.
This would add lots more complexity for parsing ACLs on input and output.

Now saying that, there is some precedence with SELinux attributes
generally integrated through the -Z option.

For completeness, if "additional attributes" manipulation we have:

ACLS: {get,set}facl
Capabilities: {get,set}cap
SELinux: getfattr -m 'selinux' -d,chcon
xattrs: {get,set}fattr
linux extra attributes: {ls,ch}attr

So as we see there are lots of "additional attributes"
with dedicated programs to manipulate them.
What's the big advantage of merging with ls and chmod,
over the current situation of separate utilities?

Also there is the question of whether ACLs are always available.
ext4 or nfs could be mounted with noacl for example, or some file systems
may need acl support enabled with a mount option.

Personally I feel we're exposing lots of complexity here for not much gain.

thanks,
Pádraig

[PATCH] doc: poll: document poll of special files not supported on macOS

[Pádraig Brady]

* doc/posix-functions/poll.texi: Reinstate (updated) macOS info,
removed in recent cleanup re removal of support for Mac OS X <= 10.4.
* m4/poll.m4: Update macOS to latest tested version.
---
 ChangeLog | 7 +++
 doc/posix-functions/poll.texi | 2 +-
 m4/poll.m4| 2 +-
 3 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 90664d16a5..aa24e4d999 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,10 @@
+2022-12-02  Pádraig Brady  
+
+   doc: poll: document poll of special files not supported on macOS
+   * doc/posix-functions/poll.texi: Reinstate (updated) macOS info,
+   removed in recent cleanup re removal of support for Mac OS X <= 10.4.
+   * m4/poll.m4: Update macOS to latest tested version.
+
 2022-11-29  Pádraig Brady  
 
add new ronna and quetta SI prefixes
diff --git a/doc/posix-functions/poll.texi b/doc/posix-functions/poll.texi
index c1efe87ad9..1fad8f85a9 100644
--- a/doc/posix-functions/poll.texi
+++ b/doc/posix-functions/poll.texi
@@ -14,7 +14,7 @@ mingw, MSVC 14, HP NonStop.
 @item
 This function doesn't work on special files like @file{/dev/null} and ttys like
 @file{/dev/tty} on some platforms:
-AIX 5.3.
+macOS 10.15, AIX 5.3.
 @end itemize
 
 Portability problems not fixed by Gnulib:
diff --git a/m4/poll.m4 b/m4/poll.m4
index eb3b78617f..4a9a6a8d8a 100644
--- a/m4/poll.m4
+++ b/m4/poll.m4
@@ -15,7 +15,7 @@ AC_DEFUN([gl_FUNC_POLL],
   else
 AC_CHECK_FUNC([poll],
   [# Check whether poll() works on special files (like /dev/null) and
-   # and ttys (like /dev/tty). On Mac OS X 10.4.0 and AIX 5.3, it doesn't.
+   # and ttys (like /dev/tty). On macOS 10.15 and AIX 5.3, it doesn't.
AC_RUN_IFELSE([AC_LANG_SOURCE([[
 #include 
 #include 
-- 
2.26.2

Re: [PATCH] add new ronna and quetta SI prefixes

[Pádraig Brady]

On 29/11/2022 17:37, Bruno Haible wrote:

The code looks good; thanks!


As voted for in Nov 2022 but the BIPM:


What does 'but' mean here? Did you mean "by the BIPM"?


Right, had just fixed that locally :)

thanks,
Pádraig

[PATCH] add new ronna and quetta SI prefixes

[Pádraig Brady]

As voted for in Nov 2022 but the BIPM:
https://www.bipm.org/en/cgpm-2022/resolution-3

* lib/human.c: Add Ronna (10^27), and Quetta (10^30) to the prefix list.
* lib/xstrtol.c (__xstrtol): Likewise.
---
 ChangeLog |  9 +
 lib/human.c   |  4 +++-
 lib/xstrtol.c | 10 +-
 3 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index e1b00bf5c9..12887e3917 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,12 @@
+2022-11-29  Pádraig Brady  
+
+   add new ronna and quetta SI prefixes
+   As voted for in Nov 2022 but the BIPM:
+   https://www.bipm.org/en/cgpm-2022/resolution-3
+
+   * lib/human.c: Add Ronna (10^27), and Quetta (10^30) to the prefix list.
+   * lib/xstrtol.c (__xstrtol): Likewise.
+
 2022-11-29  Bruno Haible  
 
Update users.txt.
diff --git a/lib/human.c b/lib/human.c
index df537271bd..0ce23a6b1d 100644
--- a/lib/human.c
+++ b/lib/human.c
@@ -43,7 +43,9 @@ static const char power_letter[] =
   'P',  /* peta or pebi */
   'E',  /* exa or exbi */
   'Z',  /* zetta or 2**70 */
-  'Y'   /* yotta or 2**80 */
+  'Y',  /* yotta or 2**80 */
+  'R',  /* ronna or 2**90 */
+  'Q'   /* quetta or 2**100 */
 };
 
 
diff --git a/lib/xstrtol.c b/lib/xstrtol.c
index e0a692ff2c..0c9910bbff 100644
--- a/lib/xstrtol.c
+++ b/lib/xstrtol.c
@@ -140,7 +140,7 @@ __xstrtol (const char *s, char **ptr, int strtol_base,
   switch (**p)
 {
 case 'E': case 'G': case 'g': case 'k': case 'K': case 'M': case 'm':
-case 'P': case 'T': case 't': case 'Y': case 'Z':
+case 'P': case 'Q': case 'R': case 'T': case 't': case 'Y': case 'Z':
 
   /* The "valid suffix" '0' is a special flag meaning that
  an optional second suffix is allowed, which can change
@@ -205,6 +205,14 @@ __xstrtol (const char *s, char **ptr, int strtol_base,
   overflow = bkm_scale_by_power (, base, 5);
   break;
 
+case 'Q': /* quetta or 2**100 */
+  overflow = bkm_scale_by_power (, base, 10);
+  break;
+
+case 'R': /* ronna or 2**90 */
+  overflow = bkm_scale_by_power (, base, 9);
+  break;
+
 case 'T': /* tera or tebi */
 case 't': /* 't' is undocumented; for compatibility only */
   overflow = bkm_scale_by_power (, base, 4);
-- 
2.26.2

Re: [PATCH] maintainer-makefile: Fix Apple Xcode 'make syntax-check'.

[Pádraig Brady]

On 01/11/2022 08:12, Simon Josefsson via Gnulib discussion list wrote:

Bruno Haible  writes:


Hi Simon,


+   @if ! indent --version 2> /dev/null | grep -q 'GNU indent'; then\


As mentioned in the Autoconf manual [1], the grep option '-q' is not portable.
E.g. on Solaris 10:

$ echo | grep -q x
grep: illegal option -- q
Usage: grep -hblcnsviw pattern file . . .

The portable alternative is
   grep 'GNU indent' > /dev/null


Thank you!  I installed these two fixes, which caught the unportable
usages in scripts for some packages.


> +sc_unportable_grep_q:
> +  @prohibit='grep -q' halt="unportable 'grep -q', use >/dev/null instead" \
> +$(_sc_search_regexp)
> +

Note the above isn't equivalent as -q will exit early upon first match
(as defined by POSIX)

Note I confirmed that Solaris 11 does support -q, while Solaris 10 does not.
I see Solaris 10 falls out of vendor support end of 2023.

cheers,
Pádraig

Re: [PATCH] fix descriptions for AT_NO_AUTOMOUNT

[Pádraig Brady]

On 14/03/2022 13:24, Alejandro Colomar (man-pages) wrote:

[Added a few CCs from the relevant kernel commits]

Hi Pádraig,

On 3/10/22 14:46, Pádraig Brady wrote:

On 10/03/2022 07:44, Andreas Schwab wrote:

On Mär 09 2022, Paul Eggert wrote:


I audited gnulib's uses of fstatat and found one fishy one that doesn't
use AT_NO_AUTOMOUNT, namely, in fts.c where the follow-symlink branch
uses
'stat' whereas the no-follow-symlink branch uses fstatat without
AT_NO_AUTOMOUNT. I installed the first patch to cause it be
consistent in
using AT_NO_AUTOMOUNT, which is also consistent with what glibc does


??? In glibc, stat is the same as fstatat(,,,0).


Indeed. It looks like the man page for fstatat is out of date.
After looking at the kernel code, it seems that:
   fstatat() did _not_ imply AT_NO_AUTOMOUNT from 2.6.38 -> 4.11
     I'm not sure it even honored the AT_NO_AUTOMOUNT flag before 4.11
   fstatat() did imply AT_NO_AUTOMOUNT since 4.11

The attached patch clarifies this is the fstatat and statx man pages.

sorry for the confusion,
Pádraig


---


Subject: [PATCH] fix descriptions for AT_NO_AUTOMOUNT

Don't mention AT_NO_AUTOMOUNT for fstatat.2
as it's implied since v4.11-rc7-14-gdeccf497d804


Even though it's implied, since code may pass it,
and especially code written based on the old manual page,
it would be good to keep the paragraph in fstatat.2,
even if the text is replaced by something like
"Before Linux x.xx, this flag was ignored.
After Linux y.yy, this flag is implied."

Does it make sense to you?


Yes good point.
I went through the git history and the summary is fstatat()
honored the flag since 2.6.38,
ignored the flag since 3.1,
implied the flag since 4.11,

I'll add that info to fstatat(2), and the details to the commit message.


Don't mention commit v4.13-9318-g42f461482178 as it was reverted


Please also mention v4.15-rc1-50-g5d38f049cee1 as the commit in which it
was reverted.
Since it was present in some kernel releases, we might want to mention
it in the manual page?


Well since the flag for fstatat() doesn't change anything since 3.1
it's probably best not to mention this old, short lived, and very specific info.


Mention that stat(), lstat(), and fstatat()
imply AT_NO_AUTOMOUNT, on the statx.2 man page


Please sign the patch with "Signed-off-by: ..."
<https://www.kernel.org/doc/man-pages/patches.html>


Done in the attached.

thanks!
Pádraig
From f45367d494d9e97fa8a18d8e477e1facf259688c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= 
Date: Thu, 10 Mar 2022 13:37:11 +
Subject: [PATCH] fix descriptions for AT_NO_AUTOMOUNT
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

fstatat(..., AT_NO_AUTOMOUNT) has had the following history in Linux:
  v2.6.37-7314-g6f45b65672c8
add AT_NO_AUTOMOUNT and fstatat honors it
  v3.1-rc7-68-gb6c8069d3577
ignore AT_NO_AUTOMOUNT since default operation now less eagerly mounts
  v4.10-11255-ga528d35e8bfc
adds statx which reinstated 2.6.38 behavior for fstatat (not released)
  v4.11-rc7-14-gdeccf497d804
adjust fstatat so that AT_NO_AUTOMOUNT always specified (to statx)

* man2/stat.2:
Adjust AT_NO_AUTOMOUNT description for fstatat.2 as per the above,
to indicate AT_NO_AUTOMOUNT should be avoided with fstatat() since
it's ignored since 3.1 and implied since 4.11.

Don't mention commit v4.13-9318-g42f461482178 as it was reverted,
and moot anyway since we can't adjust AT_NO_AUTOMOUNT since 3.1.

* man2/statx.2:
Mention that stat(), lstat(), and fstatat() imply AT_NO_AUTOMOUNT.

Signed-off-by: Pádraig Brady 
---
 man2/stat.2  | 31 +++
 man2/statx.2 | 18 +++---
 2 files changed, 18 insertions(+), 31 deletions(-)

diff --git a/man2/stat.2 b/man2/stat.2
index 016c1f47d..9000b2ca6 100644
--- a/man2/stat.2
+++ b/man2/stat.2
@@ -319,34 +319,9 @@ to obtain its definition.
 .TP
 .BR AT_NO_AUTOMOUNT " (since Linux 2.6.38)"
 Don't automount the terminal ("basename") component of
-.I pathname
-if it is a directory that is an automount point.
-This allows the caller to gather attributes of an automount point
-(rather than the location it would mount).
-Since Linux 4.14,
-.\" commit 42f46148217865a545e129612075f3d828a2c4e4
-also don't instantiate a nonexistent name in an
-on-demand directory such as used for automounter indirect maps.
-This
-flag has no effect if the mount point has already been mounted over.
-.IP
-Both
-.BR stat ()
-and
-.BR lstat ()
-act as though
-.B AT_NO_AUTOMOUNT
-was set.
-.IP
-The
-.B AT_NO_AUTOMOUNT
-can be used in tools that scan directories
-to prevent mass-automounting of a directory of automount points.
-.IP
-This flag is Linux-specific; define
-.B _GNU_SOURCE
-.\" Before glibc 2.16, defining _ATFILE_SOURCE sufficed
-to obtain its definition.
+.I pathname.
+Since Linux 3.1 this flag is ignored.
+Since Linux 4.11 this flag is implied.
 .TP
 .B AT_SYMLINK_NOFOLLOW
 

Re: [PATCH] fix descriptions for AT_NO_AUTOMOUNT

[Pádraig Brady]

On 10/03/2022 19:29, Paul Eggert wrote:

On 3/10/22 05:46, Pádraig Brady wrote:

After looking at the kernel code, it seems that:
    fstatat() did _not_ imply AT_NO_AUTOMOUNT from 2.6.38 -> 4.11
      I'm not sure it even honored the AT_NO_AUTOMOUNT flag before 4.11
    fstatat() did imply AT_NO_AUTOMOUNT since 4.11


Ouch, so this whole thing has been a false alarm? Well, in some sense
that's a relief; in another sense I wonder whether we should undo some
of the recent Gnulib changes.


The changes are a net improvement I think since fewer interfaces are used.

I would remove the AT_NO_AUTOMOUNT parameters to fstatat() though,
since they're redundant it seems, and would only result in confusion
if the patch is applied to remove that flag from the fstatat(2) man page.

thanks,
Pádraig

[PATCH] fix descriptions for AT_NO_AUTOMOUNT

[Pádraig Brady]

On 10/03/2022 07:44, Andreas Schwab wrote:

On Mär 09 2022, Paul Eggert wrote:


I audited gnulib's uses of fstatat and found one fishy one that doesn't
use AT_NO_AUTOMOUNT, namely, in fts.c where the follow-symlink branch uses
'stat' whereas the no-follow-symlink branch uses fstatat without
AT_NO_AUTOMOUNT. I installed the first patch to cause it be consistent in
using AT_NO_AUTOMOUNT, which is also consistent with what glibc does


??? In glibc, stat is the same as fstatat(,,,0).


Indeed. It looks like the man page for fstatat is out of date.
After looking at the kernel code, it seems that:
  fstatat() did _not_ imply AT_NO_AUTOMOUNT from 2.6.38 -> 4.11
I'm not sure it even honored the AT_NO_AUTOMOUNT flag before 4.11
  fstatat() did imply AT_NO_AUTOMOUNT since 4.11

The attached patch clarifies this is the fstatat and statx man pages.

sorry for the confusion,
PádraigFrom d5c356f18b18cceb245ae9df175322760f32fb2a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= 
Date: Thu, 10 Mar 2022 13:37:11 +
Subject: [PATCH] fix descriptions for AT_NO_AUTOMOUNT

Don't mention AT_NO_AUTOMOUNT for fstatat.2
as it's implied since v4.11-rc7-14-gdeccf497d804

Don't mention commit v4.13-9318-g42f461482178 as it was reverted

Mention that stat(), lstat(), and fstatat()
imply AT_NO_AUTOMOUNT, on the statx.2 man page
---
 man2/stat.2  | 31 ---
 man2/statx.2 | 18 +++---
 2 files changed, 15 insertions(+), 34 deletions(-)

diff --git a/man2/stat.2 b/man2/stat.2
index 016c1f47d..43ab5b777 100644
--- a/man2/stat.2
+++ b/man2/stat.2
@@ -317,37 +317,6 @@ This flag is Linux-specific; define
 .\" Before glibc 2.16, defining _ATFILE_SOURCE sufficed
 to obtain its definition.
 .TP
-.BR AT_NO_AUTOMOUNT " (since Linux 2.6.38)"
-Don't automount the terminal ("basename") component of
-.I pathname
-if it is a directory that is an automount point.
-This allows the caller to gather attributes of an automount point
-(rather than the location it would mount).
-Since Linux 4.14,
-.\" commit 42f46148217865a545e129612075f3d828a2c4e4
-also don't instantiate a nonexistent name in an
-on-demand directory such as used for automounter indirect maps.
-This
-flag has no effect if the mount point has already been mounted over.
-.IP
-Both
-.BR stat ()
-and
-.BR lstat ()
-act as though
-.B AT_NO_AUTOMOUNT
-was set.
-.IP
-The
-.B AT_NO_AUTOMOUNT
-can be used in tools that scan directories
-to prevent mass-automounting of a directory of automount points.
-.IP
-This flag is Linux-specific; define
-.B _GNU_SOURCE
-.\" Before glibc 2.16, defining _ATFILE_SOURCE sufficed
-to obtain its definition.
-.TP
 .B AT_SYMLINK_NOFOLLOW
 If
 .I pathname
diff --git a/man2/statx.2 b/man2/statx.2
index 04b3e5075..d4e638756 100644
--- a/man2/statx.2
+++ b/man2/statx.2
@@ -195,11 +195,23 @@ Don't automount the terminal ("basename") component of
 if it is a directory that is an automount point.
 This allows the caller to gather attributes of an automount point
 (rather than the location it would mount).
-This flag can be used in tools that scan directories
-to prevent mass-automounting of a directory of automount points.
+This
+flag has no effect if the mount point has already been mounted over.
+.IP
 The
 .B AT_NO_AUTOMOUNT
-flag has no effect if the mount point has already been mounted over.
+flag can be used in tools that scan directories
+to prevent mass-automounting of a directory of automount points.
+.IP
+All of
+.BR stat () ,
+.BR lstat () ,
+and
+.BR fstatat ()
+act as though
+.B AT_NO_AUTOMOUNT
+was set.
+.IP
 This flag is Linux-specific; define
 .B _GNU_SOURCE
 .\" Before glibc 2.16, defining _ATFILE_SOURCE sufficed
-- 
2.31.1

[PATCH] fcntl-h: add AT_NO_AUTOMOUNT

[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

Re: [PATCH] argmatch: add variants that only match full argument

[Pádraig Brady]

On 30/01/2022 18:58, Bruno Haible wrote:

Pádraig Brady wrote:

* lib/argmatch.h (argmatch_exact, [X]ARGMATCH_EXACT): New interfaces
that don't support abbreviations.
* lib/argmatch.c (argmatch_exact, __xargmatch_exact_internal): Likewise.
* tests/test-argmatch.c: Add tests.


The code looks correct. But I see some code duplication:
__xargmatch_exact_internal and __xargmatch_internal are very similar.
Given that both of these functions are only invoked through macros, how
about merging them into a single function? Namely, by adding an argument
of type 'bool exactp'.

Bruno


Good call.
I pushed with that adjustment.

Thanks for the review.
Pádraig

argmatch: add variants that only match full argument

[Pádraig Brady]

I would like a more constrained argument matching
to improve forward compatibility and robustness.

For example I would like `cksum -a sha3` to _not_
be equivalent to `cksum -a sha386`, so that a user
specifying `-a sha3` on an older cksum would not be surprised.
Also argmatch() is used when parsing tags from lines like:
SHA3 (filename) = abcedf
so it's more robust that older cksum instances to fail
earlier in the parsing process, when parsing output from
possible future cksum implementations that might support SHA3.

[PATCH] argmatch: add variants that only match full argument

[Pádraig Brady]

* lib/argmatch.h (argmatch_exact, [X]ARGMATCH_EXACT): New interfaces
that don't support abbreviations.
* lib/argmatch.c (argmatch_exact, __xargmatch_exact_internal): Likewise.
* tests/test-argmatch.c: Add tests.
---
 ChangeLog |  8 
 lib/argmatch.c| 37 +
 lib/argmatch.h| 17 +
 tests/test-argmatch.c | 23 +++
 4 files changed, 85 insertions(+)

diff --git a/ChangeLog b/ChangeLog
index 4c9a1e7989..e51a00190a 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,11 @@
+2022-01-30  Pádraig Brady  
+
+   argmatch: add variants that only match full argument
+   * lib/argmatch.h (argmatch_exact, [X]ARGMATCH_EXACT): New interfaces
+   that don't support abbreviations.
+   * lib/argmatch.c (argmatch_exact, __xargmatch_exact_internal): Likewise.
+   * tests/test-argmatch.c: Add tests.
+
 2022-01-30  Bruno Haible  
 
terminfo: Add tests.
diff --git a/lib/argmatch.c b/lib/argmatch.c
index 9e3232f947..71059e8f7e 100644
--- a/lib/argmatch.c
+++ b/lib/argmatch.c
@@ -120,6 +120,24 @@ argmatch (const char *arg, const char *const *arglist,
 return matchind;
 }
 
+ptrdiff_t
+argmatch_exact (const char *arg, const char *const *arglist)
+{
+  size_t i;
+
+  /* Test elements for exact match.  */
+  for (i = 0; arglist[i]; i++)
+{
+  if (!strcmp (arglist[i], arg))
+{
+  /* Exact match found.  */
+  return i;
+}
+}
+
+  return -1;
+}
+
 /* Error reporting for argmatch.
CONTEXT is a description of the type of entity that was being matched.
VALUE is the invalid value that was given.
@@ -189,6 +207,25 @@ __xargmatch_internal (const char *context,
   return -1; /* To please the compilers. */
 }
 
+ptrdiff_t
+__xargmatch_exact_internal (const char *context,
+const char *arg, const char *const *arglist,
+const void *vallist, size_t valsize,
+argmatch_exit_fn exit_fn)
+{
+  ptrdiff_t res = argmatch_exact (arg, arglist);
+  if (res >= 0)
+/* Success. */
+return res;
+
+  /* We failed.  Explain why. */
+  argmatch_invalid (context, arg, res);
+  argmatch_valid (arglist, vallist, valsize);
+  (*exit_fn) ();
+
+  return -1; /* To please the compilers. */
+}
+
 /* Look for VALUE in VALLIST, an array of objects of size VALSIZE and
return the first corresponding argument in ARGLIST */
 const char *
diff --git a/lib/argmatch.h b/lib/argmatch.h
index a2364b5a2e..c6d24d981c 100644
--- a/lib/argmatch.h
+++ b/lib/argmatch.h
@@ -52,9 +52,15 @@ extern "C" {
 ptrdiff_t argmatch (char const *arg, char const *const *arglist,
 void const *vallist, size_t valsize) _GL_ATTRIBUTE_PURE;
 
+ptrdiff_t argmatch_exact (char const *arg, char const *const *arglist)
+  _GL_ATTRIBUTE_PURE;
+
 # define ARGMATCH(Arg, Arglist, Vallist) \
   argmatch (Arg, Arglist, (void const *) (Vallist), sizeof *(Vallist))
 
+# define ARGMATCH_EXACT(Arg, Arglist) \
+  argmatch_exact (Arg, Arglist)
+
 /* xargmatch calls this function when it fails.  This function should not
return.  By default, this is a function that calls ARGMATCH_DIE which
in turn defaults to 'exit (exit_failure)'.  */
@@ -91,6 +97,11 @@ ptrdiff_t __xargmatch_internal (char const *context,
 void const *vallist, size_t valsize,
 argmatch_exit_fn exit_fn);
 
+ptrdiff_t __xargmatch_exact_internal (char const *context,
+char const *arg, char const *const *arglist,
+void const *vallist, size_t valsize,
+argmatch_exit_fn exit_fn);
+
 /* Programmer friendly interface to __xargmatch_internal. */
 
 # define XARGMATCH(Context, Arg, Arglist, Vallist)  \
@@ -99,6 +110,12 @@ ptrdiff_t __xargmatch_internal (char const *context,
 sizeof *(Vallist),  \
 argmatch_die)])
 
+# define XARGMATCH_EXACT(Context, Arg, Arglist, Vallist)\
+  ((Vallist) [__xargmatch_exact_internal (Context, Arg, Arglist,\
+(void const *) (Vallist),   \
+sizeof *(Vallist),  \
+argmatch_die)])
+
 /* Convert a value into a corresponding argument. */
 
 char const *argmatch_to_argument (void const *value,
diff --git a/tests/test-argmatch.c b/tests/test-argmatch.c
index 8345150002..46a7f07153 100644
--- a/tests/test-argmatch.c
+++ b/tests/test-argmatch.c
@@ -125,37 +125,60 @@ main (int argc, char *argv[])
   } \
   } while (0)
 
+#define CHECK_EXACT(Input, Output)  \
+  do {  \
+ASSERT (ARGMATCH

Re: [PATCH] copy-file-range: work around Linux kernel bug

[Pádraig Brady]

On 16/01/2022 18:06, Paul Eggert wrote:

On 1/16/22 05:37, Pádraig Brady wrote:

This looks like the replacement will only be used when the build system
uses an older kernel?
If so this seems brittle. Consider the case where el7 rpms are being
built on
central build systems with newer kernels.


It doesn't run any code on the newer kernels, right? It merely compiles.
If the compilation environment says "this is being compiled for kernel
5.3", surely the generated code can assume 5.3 or later.


Oh right. I guess the build farm would use the oldest kernel headers for this 
reason.
I.e. the replacement code would be built as long as appropriate kernel headers
were installed.  This may be a gotcha for some build / deployment setups,
but that would be unusual, so this is the right default.
I'm just extra paranoid about ensuring copy routines are robust.

Sorry for the noise.

thanks,
Pádraig

Re: [PATCH] copy-file-range: work around Linux kernel bug

[Pádraig Brady]

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

--- a/m4/copy-file-range.m4
+++ b/m4/copy-file-range.m4
@@ -7,6 +7,7 @@ dnl with or without modifications, as long as this notice is 
preserved.
  AC_DEFUN([gl_FUNC_COPY_FILE_RANGE],
  [
AC_REQUIRE([gl_UNISTD_H_DEFAULTS])
+  AC_REQUIRE([AC_CANONICAL_HOST])
  
dnl Persuade glibc  to declare copy_file_range.

AC_REQUIRE([AC_USE_SYSTEM_EXTENSIONS])
@@ -21,7 +22,7 @@ AC_DEFUN([gl_FUNC_COPY_FILE_RANGE],
 [AC_LANG_PROGRAM(
[[#include 
]],
-  [[ssize_t (*func) (int, off_t *, int, off_t, size_t, unsigned)
+  [[ssize_t (*func) (int, off_t *, int, off_t *, size_t, unsigned)
= copy_file_range;
  return func (0, 0, 0, 0, 0, 0) & 127;
]])
@@ -32,5 +33,27 @@ AC_DEFUN([gl_FUNC_COPY_FILE_RANGE],
  
if test "$gl_cv_func_copy_file_range" != yes; then

  HAVE_COPY_FILE_RANGE=0
+  else
+AC_DEFINE([HAVE_COPY_FILE_RANGE], 1,
+  [Define to 1 if the function copy_file_range exists.])
+
+case $host_os in
+  linux*)
+AC_CACHE_CHECK([whether copy_file_range is known to work],
+  [gl_cv_copy_file_range_known_to_work],
+  [AC_COMPILE_IFELSE(
+ [AC_LANG_PROGRAM(
+[[#include 
+]],
+[[#if LINUX_VERSION_CODE < KERNEL_VERSION (5, 3, 0)
+   #error "copy_file_range is buggy"
+  #endif
+]])],
+ [gl_cv_copy_file_range_known_to_work=yes],
+ [gl_cv_copy_file_range_known_to_work=no])])
+if test "$gl_cv_copy_file_range_known_to_work" = no; then
+  REPLACE_COPY_FILE_RANGE=1
+fi;;
+esac
fi
  ])


This looks like the replacement will only be used when the build system uses an 
older kernel?
If so this seems brittle. Consider the case where el7 rpms are being built on
central build systems with newer kernels.

cheers,
Pádraig

[PATCH] maintainer-makefile: add 'can' to sc_prohibit_doubled_word

[Pádraig Brady]

* top/maint.mk (sc_prohibit_doubled_word): Check for "can can".
---
 ChangeLog| 5 +
 top/maint.mk | 2 +-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/ChangeLog b/ChangeLog
index 7c56ed2a2..a7fd469f4 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,8 @@
+2021-09-09  Pádraig Brady  
+
+   maintainer-makefile: add 'can' to sc_prohibit_doubled_word
+   * top/maint.mk (sc_prohibit_doubled_word): Check for "can can".
+
 2021-09-08  Paul Eggert  
 
strerror_r-posix: port even better to Android
diff --git a/top/maint.mk b/top/maint.mk
index 72070a7b0..0aa63773c 100644
--- a/top/maint.mk
+++ b/top/maint.mk
@@ -1031,7 +1031,7 @@ perl_filename_lineno_text_ =  
\
 -e '  }'
 
 prohibit_doubled_words_ = \
-the then in an on if is it but for or at and do to
+the then in an on if is it but for or at and do to can
 # expand the regex before running the check to avoid using expensive captures
 prohibit_doubled_word_expanded_ = \
 $(join $(prohibit_doubled_words_),$(addprefix 
\s+,$(prohibit_doubled_words_)))
-- 
2.26.2

Re: Seeking input from developers: glibc copyright assignment policy.

[Pádraig Brady]

On 15/06/2021 13:03, Eric Blake wrote:

On Mon, Jun 14, 2021 at 01:39:26PM -0700, Paul Eggert wrote:

A proposal to change the glibc copyright assignment policy is being
circulated on libc-alpha. The email thread starts at
, and the
text of the email seeking input is at the end of this message.

I'm sending this to bug-gnulib because we copy some files directly from
glibc and eventually I expect these files to be affected. The simplest
approach I see for Gnulib is to adopt glibc's policy, at least for files or
code copied from glibc.


I would like to make sure the FSF legal department doesn't see any
holes in the plan, but I'm certainly okay as going on record as being
in favor of the plan.


I am in favor of the plan also.
I feel copyright assignment is a significant _initial_ hurdle to contributions,
where the potential to deter contribution outweighs any potential advantages.


I recall how long it took for me to get
permission to sign assignment papers from my previous employer, for
work I was doing in my spare time, and being able to use the DCO
instead would have made my efforts easier at that time.  (My current
employer already has a blanket copyright assignment on file for all
employees, but not everyone has a company that willing to promote open
source involvement.)


Yes the fact that one needs to repeat this process
as one changes employers is very awkward.
(In my case it was the reverse, in going from a company with
blanket copyright assignment, to one where I needed to
navigate the internal processes to get an assignment.)

I do wonder how diligent people are in general in this regard anyway.
I.e. how valid all current assignments are given the
frequency of employer changes and the awkwardness involved.

thanks,
Pádraig

Re: [PATCH] mountlist: recognize fuse.portal as dummy file system

[Pádraig Brady]

On 07/06/2021 13:43, Kamil Dudka wrote:

This was originally proposed at:

 https://lists.gnu.org/archive/html/bug-gnulib/2021-02/msg00053.html

As the full review might take some time, would it be possible to apply
at least the part related to fuse.portal file systems?  They started to
cause problems recently:

 https://bugs.launchpad.net/ubuntu/+source/xdg-desktop-portal/+bug/1905623
 https://github.com/muesli/duf/issues/35
 https://bugzilla.redhat.com/1913358
---
  lib/mountlist.c | 1 +
  1 file changed, 1 insertion(+)

diff --git a/lib/mountlist.c b/lib/mountlist.c
index f5d1364c1b..e4c1779829 100644
--- a/lib/mountlist.c
+++ b/lib/mountlist.c
@@ -169,6 +169,7 @@
 || strcmp (Fs_type, "debugfs") == 0  \
 || strcmp (Fs_type, "devpts") == 0   \
 || strcmp (Fs_type, "fusectl") == 0  \
+   || strcmp (Fs_type, "fuse.portal") == 0  \
 || strcmp (Fs_type, "mqueue") == 0   \
 || strcmp (Fs_type, "rpc_pipefs") == 0   \
 || strcmp (Fs_type, "sysfs") == 0\



Pushed.
"fuse.portal" should be fine to always consider as dummy.

thanks,
Pádraig

Re: *alloc-gnu on glibc

[Pádraig Brady]

On 15/05/2021 16:49, Bruno Haible wrote:

Hi Pádraig,


On glibc-2.31-5.fc32.x86_64 I'm seeing this test failure with coreutils,
since the tests are now checking for a specific errno.
I just checked a later Fedora 34 system which has the same issue.


For me, a testdir created through

./gnulib-tool --create-testdir --dir=../testdir --single-configure \
   malloc-gnu realloc-gnu calloc-gnu reallocarray

passes all tests on Fedora 32 (glibc 2.31) and Fedora 33 (glibc 2.32).


Oh. Ah this issue is restricted to glibc's MALLOC_CHECK_ implementation.
The attached avoids this part of the test if it's being used.
I've reported the glibc bug at:
https://sourceware.org/bugzilla/show_bug.cgi?id=27870

With this applied, your command above passes all tests.

cheers,
Pádraig
>From defe5bebcd1e2bb75f15eb54ae1135afaae3c477 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= 
Date: Sat, 15 May 2021 17:50:33 +0100
Subject: [PATCH] realloc-gnu: avoid glibc MALLOC_CHECK_ issue

* tests/test-realloc-gnu.c (main): if MALLOC_CHECK_ env var
is set then don't check ENOMEM is returned from realloc().
See https://sourceware.org/bugzilla/show_bug.cgi?id=27870
Note it doesn't suffice to unsetenv() this var within the program,
as the hooks have already been set up at that stage.
---
 ChangeLog| 9 +
 tests/test-realloc-gnu.c | 4 +++-
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/ChangeLog b/ChangeLog
index b802233cf..30663cb38 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,12 @@
+2021-05-15  Pádraig Brady  
+
+	realloc-gnu: avoid glibc MALLOC_CHECK_ issue
+	* tests/test-realloc-gnu.c (main): if MALLOC_CHECK_ env var
+	is set then don't check ENOMEM is returned from realloc().
+	See https://sourceware.org/bugzilla/show_bug.cgi?id=27870
+	Note it doesn't suffice to unsetenv() this var within the program,
+	as the hooks have already been set up at that stage.
+
 2021-05-14  Paul Eggert  
 
 	c-stack: work around Solaris 11 bugs
diff --git a/tests/test-realloc-gnu.c b/tests/test-realloc-gnu.c
index 3a787ed91..5534f2ea1 100644
--- a/tests/test-realloc-gnu.c
+++ b/tests/test-realloc-gnu.c
@@ -38,7 +38,9 @@ main (int argc, char **argv)
   size_t one = argc != 12345;
   p = realloc (p, PTRDIFF_MAX + one);
   ASSERT (p == NULL);
-  ASSERT (errno == ENOMEM);
+  /* Avoid https://sourceware.org/bugzilla/show_bug.cgi?id=27870  */
+  if (!getenv ("MALLOC_CHECK_"))
+ASSERT (errno == ENOMEM);
 }
 
   free (p);
-- 
2.26.2

Re: *alloc-gnu on glibc

[Pádraig Brady]

On glibc-2.31-5.fc32.x86_64 I'm seeing this test failure with coreutils,
since the tests are now checking for a specific errno.
I just checked a later Fedora 34 system which has the same issue.

Specifically test-realloc-gnu is enabled in coreutils
and it's failing as realloc is returning NULL as expected, but errno is 0.
Specifically on glibc realloc(NULL, PTRDIFF_MAX+1) does return 0,errno=ENOMEM,
but realloc(non_null, PTRDIFF_MAX+1) returns 0,errno=0.

I'm wondering should we be more relaxed here as,
there is only one standardised ENOMEM error from realloc,
so code doesn't have to behave differently based on what the errno is.
Especially if it's using xrealloc etc.
Now I accept that error messages may be more accurate,
but one should opt into that I think.

Should we adjust things so depending on realloc-gnu
would not have the more stringent requirement of ENOMEM
being returned from realloc(), and for projects that wanted that,
they could depend on the realloc-posix module instead?

In any case I'd be in favor of relaxing the test,
rather than replacing realloc everywhere.

some notes on solaris below...

On 14/05/2021 18:04, Bruno Haible wrote:

On Solaris 11.3 (the machine gcc211.fsffrance.org, in 64-bit mode), there
are test failures:
   FAIL: test-calloc-gnu
   FAIL: test-malloc-gnu
   FAIL: test-realloc-gnu
   FAIL: test-reallocarray
Each of these tests takes about 30 seconds before failing, and is not
immediately reactive to Ctrl-C.


These EAGAIN failure modes sound like the value passed to realloc() etc.
is too small to ensure ENOMEM was return immediately.
I've not looked into how the compiler for the m4 test is invoked,
but I did check the code in isolation on a 64 bit solaris 10 system.
The following shows that it's important to ensure the compiler
is explicitly running in 64 bit mode, as the default is 32 bit,
resulting in 32 bit longs and hence too small of values passed in:

> gcc malloc-m4.c
> ./a.out
PTRDIFF_MAX = 2147483647
p=20f90 errno=0
p=0 errno=12
p=0 errno=12

> gcc -m64 malloc-m4.c
> ./a.out
PTRDIFF_MAX = 9223372036854775807
p=0 errno=12
p=0 errno=12
p=0 errno=12

Both the above returned immediately for me.
Though I also notice the existing note in malloc.m4 on this
check being too dangerous to run in general due to this issue.

cheers,
Pádraig

Re: Suggested fix to mountlist.c: Define fuse.portal, devtmpfs and squashfs mounts as dummy

[Pádraig Brady]

On 13/02/2021 01:40, Daniel Bonner wrote:

Hi gnulib developers,
At present many gnulib users encounter this error:
Running 'df' as user (not root) causes results in output that includes
this error message:
"df: /run/user/1000/doc: Operation not permitted"
I would like to pass on to you a suggested fix:
@@ -164,7 +164,10 @@
  #define ME_DUMMY_0(Fs_name, Fs_type)\
(strcmp (Fs_type, "autofs") == 0  \
+   || strcmp (Fs_type, "devtmpfs") == 0 \
+   || strcmp (Fs_type, "fuse.portal") == 0  \
 || strcmp (Fs_type, "proc") == 0 \
+   || strcmp (Fs_type, "squashfs") == 0 \
 || strcmp (Fs_type, "subfs") == 0\
 /* for Linux 2.6/3.x */  \
 || strcmp (Fs_type, "debugfs") == 0  \
Julian Klode, from Ubuntu, deserves credit for this suggested fix. He
published a patch that has been included in the Ubuntu Hirsute 21.04
package: coreutils 8.32-4ubuntu2.  See
1. https://bugs.launchpad.net/ubuntu/+source/xdg-desktop-portal/+bug/19056232.
2. 
https://launchpadlibrarian.net/510003931/coreutils_8.32-4ubuntu1_8.32-4ubuntu2.diff.gz
I have cc'ed Julian in this email.
The patch addresses a problem that particularly affects gnulib users
with fuse.portal mounts, such as users who have installed Flatpak.
This is a significant problem for users who depend on df not returning
an error exit code in their scripts.  Julian's  bug report said:
"/run/user/1000/doc is a fuse.portal mount point, but statfs() return
EPERM, hence df produces an error message. Maybe statfs() is not
implemented, but it would be good to quieten this down (df even does
not allow me to ignore it, probably because it looks at statfs to find
out fs type, so my fs type ignoring doesn't work)."
This problem has also been reported on Fedora 32 here:
flatpak/xdg-desktop-portal#512
Kind regards,
Daniel



I'll look into this in a bit more detail to see if it's general enough.
I.e. that there are no cases where these file system types may
have usable space.  squashfs is read-only so that is probably ok.

thanks!
Pádraig

Re: different CFLAGS for gnulib code?

[Pádraig Brady]

On 15/01/2021 08:55, Bruno Haible wrote:

I always thought that GNU package maintainers want their entire package to
be compiled with the same CFLAGS and CPPFLAGS. Would compiling the gnulib
part with options for fewer warnings be OK with you?

Paul, Pádraig, Jim, Paul, Akim, Simon, all: what's your opinion?


Re coreutils we already have different warnings,
specifically a subset of warnings are used with gnulib.

gnulib is necessarily more general/portable than a particular project,
and so could not be as generally stringent in its warnings I think.
Also a new project has more flexibility to enable a stricter set
from the start, than a mature code base like gnulib.

cheers,
Pádraig

Re: [PATCH] free: preserve errno

[Pádraig Brady]

On 17/12/2020 09:54, Paul Eggert wrote:

* lib/free.c (rpl_free): Preserve errno.  Check for null only if
CANNOT_FREE_NULL is defined, as an optimization for POSIX 2008
platforms that do not preserve errno.
* m4/free.m4 (gl_FUNC_FREE): Check whether free preserves errno.
Also, define CANNOT_FREE_NULL if free cannot free NULL.
* modules/free (configure.ac): Also replace 'free' if
it does not preserve errno.


coreutils is now failing to build with:

  lib/free.c:28:1: error: no previous declaration
for 'rpl_free' [-Werror=missing-declarations]
   28 | rpl_free (void *p)
  | ^~~~

In early 2008 coreutils removed use of the then deprecated "free" module,
but I now see 'canonicalize' explicitly depends on this now undeprecated module.

I've worked around the issue,
but I'm not sure how you'd prefer to handle it in gnulib.

thanks,
Pádraig

[PATCH] selinux-at, selinux-h: use const correct declarations

[Pádraig Brady]

* lib/se-selinux.in.h: Use const for "set" functions,
to match current selinux, and support cleaner user code.
* lib/selinux-at.c: Likewise.
* lib/selinux-at.h: Likewise.
---
 ChangeLog   |  8 
 lib/se-selinux.in.h | 18 +-
 lib/selinux-at.c|  4 ++--
 lib/selinux-at.h|  4 ++--
 4 files changed, 21 insertions(+), 13 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 044b12d8c..a17f2b208 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,11 @@
+2020-11-23  Pádraig Brady  
+
+   selinux-at, selinux-h: use const correct declarations
+   * lib/se-selinux.in.h: Use const for "set" functions,
+   to match current selinux, and support cleaner user code.
+   * lib/selinux-at.c: Likewise.
+   * lib/selinux-at.h: Likewise.
+
 2020-11-22  Paul Eggert  
 
canonicalize-lgpl: fix memory leak
diff --git a/lib/se-selinux.in.h b/lib/se-selinux.in.h
index a6c194aa0..67d034d0f 100644
--- a/lib/se-selinux.in.h
+++ b/lib/se-selinux.in.h
@@ -56,7 +56,7 @@ SE_SELINUX_INLINE int
 getfscreatecon (char **con _GL_UNUSED_PARAMETER)
   { errno = ENOTSUP; return -1; }
 SE_SELINUX_INLINE int
-setfscreatecon (char *con _GL_UNUSED_PARAMETER)
+setfscreatecon (char const *con _GL_UNUSED_PARAMETER)
   { errno = ENOTSUP; return -1; }
 SE_SELINUX_INLINE int
 matchpathcon (char const *file _GL_UNUSED_PARAMETER,
@@ -76,29 +76,29 @@ fgetfilecon (int fd, char **con _GL_UNUSED_PARAMETER)
   { errno = ENOTSUP; return -1; }
 SE_SELINUX_INLINE int
 setfilecon (char const *file _GL_UNUSED_PARAMETER,
-char *con _GL_UNUSED_PARAMETER)
+char const *con _GL_UNUSED_PARAMETER)
   { errno = ENOTSUP; return -1; }
 SE_SELINUX_INLINE int
 lsetfilecon (char const *file _GL_UNUSED_PARAMETER,
- char *con _GL_UNUSED_PARAMETER)
+ char const *con _GL_UNUSED_PARAMETER)
   { errno = ENOTSUP; return -1; }
 SE_SELINUX_INLINE int
 fsetfilecon (int fd _GL_UNUSED_PARAMETER,
- char *con _GL_UNUSED_PARAMETER)
+ char const *con _GL_UNUSED_PARAMETER)
   { errno = ENOTSUP; return -1; }
 
 SE_SELINUX_INLINE int
-security_check_context (char *con _GL_UNUSED_PARAMETER)
+security_check_context (char const *con _GL_UNUSED_PARAMETER)
   { errno = ENOTSUP; return -1; }
 SE_SELINUX_INLINE int
-security_check_context_raw (char *con _GL_UNUSED_PARAMETER)
+security_check_context_raw (char const *con _GL_UNUSED_PARAMETER)
   { errno = ENOTSUP; return -1; }
 SE_SELINUX_INLINE int
-setexeccon (char *con _GL_UNUSED_PARAMETER)
+setexeccon (char const *con _GL_UNUSED_PARAMETER)
   { errno = ENOTSUP; return -1; }
 SE_SELINUX_INLINE int
-security_compute_create (char *scon _GL_UNUSED_PARAMETER,
- char *tcon _GL_UNUSED_PARAMETER,
+security_compute_create (char const *scon _GL_UNUSED_PARAMETER,
+ char const *tcon _GL_UNUSED_PARAMETER,
  security_class_t tclass _GL_UNUSED_PARAMETER,
  char **newcon _GL_UNUSED_PARAMETER)
   { errno = ENOTSUP; return -1; }
diff --git a/lib/selinux-at.c b/lib/selinux-at.c
index 105a9f9d5..e1d214c2a 100644
--- a/lib/selinux-at.c
+++ b/lib/selinux-at.c
@@ -52,7 +52,7 @@
 
 #define AT_FUNC_NAME setfileconat
 #define AT_FUNC_F1 setfilecon
-#define AT_FUNC_POST_FILE_PARAM_DECLS , char *con
+#define AT_FUNC_POST_FILE_PARAM_DECLS , char const *con
 #define AT_FUNC_POST_FILE_ARGS, con
 #include "at-func.c"
 #undef AT_FUNC_NAME
@@ -62,7 +62,7 @@
 
 #define AT_FUNC_NAME lsetfileconat
 #define AT_FUNC_F1 lsetfilecon
-#define AT_FUNC_POST_FILE_PARAM_DECLS , char *con
+#define AT_FUNC_POST_FILE_PARAM_DECLS , char const *con
 #define AT_FUNC_POST_FILE_ARGS, con
 #include "at-func.c"
 #undef AT_FUNC_NAME
diff --git a/lib/selinux-at.h b/lib/selinux-at.h
index 50537f80f..9b331cb18 100644
--- a/lib/selinux-at.h
+++ b/lib/selinux-at.h
@@ -42,11 +42,11 @@ int lgetfileconat (int dir_fd, char const *file, char 
**con);
the file specified by DIR_FD and FILE to CON.  DIR_FD and FILE are
interpreted as for fstatat[*].  Upon success, return 0.
Otherwise, return -1 and set errno.  */
-int  setfileconat (int dir_fd, char const *file, char *con);
+int  setfileconat (int dir_fd, char const *file, char const *con);
 
 /* dir-fd-relative lsetfilecon.  This function is just like setfileconat,
except that rather than dereferencing a symlink, this function affects it. 
*/
 /* dir-fd-relative lsetfilecon.  This function is just like setfileconat,
except when DIR_FD and FILE specify a symlink:  lsetfileconat operates on
the symlink, while setfileconat operates on the referent of the symlink.  */
-int lsetfileconat (int dir_fd, char const *file, char *con);
+int lsetfileconat (int dir_fd, char const *file, char const *con);
-- 
2.26.2

Re: [PATCH] selinux-h: add stubs for selabel_open etc.

[Pádraig Brady]

On 22/11/2020 17:56, Pádraig Brady wrote:

On 22/11/2020 14:27, Bernhard Voelker wrote:

On 11/22/20 1:28 PM, Kamil Dudka wrote:

The coreutils patch introduced tab-indented lines into src/mv.c, which
otherwise does not use tabs for indentation.


This is caught by a syntax-check rule as well.
Fixed with the attached patch - as well as 2 other sc failures.

I can see 3 tests failing
on Fedora 32, which seems to be related.  My test-suite.log is attached.


I didn't have a look at the failures - I don't get them here as
my system skips all those SELinux-related tests.


Thanks for the testing.
It seems selabel_lookup requires absolute paths.
Reinstating that code with the attached,
gets all tests to pass here on Fedora 32
with selinux enabled.


Non leaky version attached.

cheers,
Pádraig
>From 1289bb62d6138e80884fd87b3ede48c4a2a2c518 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= 
Date: Sun, 22 Nov 2020 17:46:52 +
Subject: [PATCH] maint: use absolute paths with selabel_lookup

* src/selinux.c: selabel_lookup requires absolute paths
(while only older matchpathcon before libselinux < 2.1.5 2011-0826 did).
* po/POTFILES.in: Readd src/selinux.c since we now have
a translatable error message.
---
 po/POTFILES.in |  1 +
 src/selinux.c  | 33 +++--
 2 files changed, 32 insertions(+), 2 deletions(-)

diff --git a/po/POTFILES.in b/po/POTFILES.in
index 5ccc0e9a9..074322393 100644
--- a/po/POTFILES.in
+++ b/po/POTFILES.in
@@ -109,6 +109,7 @@ src/remove.c
 src/rm.c
 src/rmdir.c
 src/runcon.c
+src/selinux.c
 src/seq.c
 src/set-fields.c
 src/shred.c
diff --git a/src/selinux.c b/src/selinux.c
index 10fa9d8c6..50efb0aec 100644
--- a/src/selinux.c
+++ b/src/selinux.c
@@ -21,7 +21,9 @@
 #include 
 #include 
 
+#include "die.h"
 #include "system.h"
+#include "canonicalize.h"
 #include "xfts.h"
 #include "selinux.h"
 
@@ -113,6 +115,16 @@ defaultcon (struct selabel_handle *selabel_handle,
   context_t scontext = 0, tcontext = 0;
   const char *contype;
   char *constr;
+  char *newpath = NULL;
+
+  if (! IS_ABSOLUTE_FILE_NAME (path))
+{
+  newpath = canonicalize_filename_mode (path, CAN_MISSING);
+  if (! newpath)
+die (EXIT_FAILURE, errno, _("error canonicalizing %s"),
+ quoteaf (path));
+  path = newpath;
+}
 
   if (selabel_lookup (selabel_handle, , path, mode) < 0)
 {
@@ -120,7 +132,7 @@ defaultcon (struct selabel_handle *selabel_handle,
  when processing files, when in fact it was the
  associated default context that was not found.
  Therefore map the error to something more appropriate
- to the context in which we're using matchpathcon().  */
+ to the context in which we're using selabel_lookup().  */
   if (errno == ENOENT)
 errno = ENODATA;
   goto quit;
@@ -146,6 +158,7 @@ quit:
   context_free (tcontext);
   freecon (scon);
   freecon (tcon);
+  free (newpath);
   return rc;
 }
 
@@ -269,8 +282,23 @@ bool
 restorecon (struct selabel_handle *selabel_handle,
 char const *path, bool recurse)
 {
+  char *newpath = NULL;
+
+  if (! IS_ABSOLUTE_FILE_NAME (path))
+{
+  newpath = canonicalize_filename_mode (path, CAN_MISSING);
+  if (! newpath)
+die (EXIT_FAILURE, errno, _("error canonicalizing %s"),
+ quoteaf (path));
+  path = newpath;
+}
+
   if (! recurse)
-return restorecon_private (selabel_handle, path) == 0;
+{
+  bool ok = restorecon_private (selabel_handle, path) != -1;
+  free (newpath);
+  return ok;
+}
 
   char const *ftspath[2] = { path, NULL };
   FTS *fts = xfts_open ((char *const *) ftspath, FTS_PHYSICAL, NULL);
@@ -286,6 +314,7 @@ restorecon (struct selabel_handle *selabel_handle,
   if (fts_close (fts) != 0)
 err = errno;
 
+  free (newpath);
   return !err;
 }
 #endif
-- 
2.26.2

Re: [PATCH] selinux-h: add stubs for selabel_open etc.

[Pádraig Brady]

On 22/11/2020 14:27, Bernhard Voelker wrote:

On 11/22/20 1:28 PM, Kamil Dudka wrote:

The coreutils patch introduced tab-indented lines into src/mv.c, which
otherwise does not use tabs for indentation.


This is caught by a syntax-check rule as well.
Fixed with the attached patch - as well as 2 other sc failures.

I can see 3 tests failing
on Fedora 32, which seems to be related.  My test-suite.log is attached.


I didn't have a look at the failures - I don't get them here as
my system skips all those SELinux-related tests.


Thanks for the testing.
It seems selabel_lookup requires absolute paths.
Reinstating that code with the attached,
gets all tests to pass here on Fedora 32
with selinux enabled.

cheers,
Pádraig
>From 031469f54c6614e02754c34b6ef0faec0281691d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= 
Date: Sun, 22 Nov 2020 17:46:52 +
Subject: [PATCH] maint: use absolute paths with selabel_lookup

* src/selinux.c: selabel_lookup requires absolute paths
(while only older matchpathcon before libselinux < 2.1.5 2011-0826 did).
* po/POTFILES.in: Readd src/selinux.c since we now have
a translatable error message.
---
 po/POTFILES.in |  1 +
 src/selinux.c  | 25 +
 2 files changed, 26 insertions(+)

diff --git a/po/POTFILES.in b/po/POTFILES.in
index 5ccc0e9a9..074322393 100644
--- a/po/POTFILES.in
+++ b/po/POTFILES.in
@@ -109,6 +109,7 @@ src/remove.c
 src/rm.c
 src/rmdir.c
 src/runcon.c
+src/selinux.c
 src/seq.c
 src/set-fields.c
 src/shred.c
diff --git a/src/selinux.c b/src/selinux.c
index 10fa9d8c6..432e03018 100644
--- a/src/selinux.c
+++ b/src/selinux.c
@@ -21,7 +21,9 @@
 #include 
 #include 
 
+#include "die.h"
 #include "system.h"
+#include "canonicalize.h"
 #include "xfts.h"
 #include "selinux.h"
 
@@ -113,6 +115,16 @@ defaultcon (struct selabel_handle *selabel_handle,
   context_t scontext = 0, tcontext = 0;
   const char *contype;
   char *constr;
+  char *newpath = NULL;
+
+  if (! IS_ABSOLUTE_FILE_NAME (path))
+{
+  newpath = canonicalize_filename_mode (path, CAN_MISSING);
+  if (! newpath)
+die (EXIT_FAILURE, errno, _("error canonicalizing %s"),
+ quoteaf (path));
+  path = newpath;
+}
 
   if (selabel_lookup (selabel_handle, , path, mode) < 0)
 {
@@ -146,6 +158,7 @@ quit:
   context_free (tcontext);
   freecon (scon);
   freecon (tcon);
+  free (newpath);
   return rc;
 }
 
@@ -269,6 +282,17 @@ bool
 restorecon (struct selabel_handle *selabel_handle,
 char const *path, bool recurse)
 {
+  char *newpath = NULL;
+
+  if (! IS_ABSOLUTE_FILE_NAME (path))
+{
+  newpath = canonicalize_filename_mode (path, CAN_MISSING);
+  if (! newpath)
+die (EXIT_FAILURE, errno, _("error canonicalizing %s"),
+ quoteaf (path));
+  path = newpath;
+}
+
   if (! recurse)
 return restorecon_private (selabel_handle, path) == 0;
 
@@ -286,6 +310,7 @@ restorecon (struct selabel_handle *selabel_handle,
   if (fts_close (fts) != 0)
 err = errno;
 
+  free (newpath);
   return !err;
 }
 #endif
-- 
2.26.2