bug#63245: Potential regression: cp --preserve=mode or --preserve=all fails to copy files from subdirectory

[Kamil Dudka]

On Friday, May 5, 2023 12:29:16 AM CEST Pádraig Brady wrote:
> On 04/05/2023 12:27, Pádraig Brady wrote:
> > On 04/05/2023 07:27, Schlomo Schapiro wrote:
> >> Hi Pádraig,
> >>
> >> thank you, that will not yet fix the problem in the older distros? What
> >> about the RPM world with Fedora/RHEL and Archlinux? As far as I can tell
> >> all cp version 9.x are affected by this.
> > 
> > I'll handle the Fedora 37/38 fixes.
> > RHEL/Centos and current Archlinux are not affected AFAICS.
> 
> Fedora 37 and 38 updates are now pending (cc Kamil):
> https://bodhi.fedoraproject.org/updates/FEDORA-2023-65365355b3
> https://bodhi.fedoraproject.org/updates/FEDORA-2023-4beb422aac

Thank you for taking care of it, Pádraig!

Kamil

> The Fedora patches should also apply to the debian bookworm package,
> so I've opened bugs accordingly at:
> 
> https://bugs.debian.org/1035530
> https://bugs.debian.org/1035531
> 
> Schlomo, the second one is the one you're particularly interested in.
> 
> Marking this bug as completed upstream.
> 
> cheers,
> Pádraig

bug#61035: [PATCH] cp: improve help regarding ACLs

[Kamil Dudka]

On Tuesday, January 31, 2023 10:56:54 PM CET Paul Eggert wrote:
> On 1/31/23 13:23, Ondrej Valousek wrote:
> > Yes, that's nice, thanks!
> > However I think it would be also fair to mention that ACLs will not be 
> > converted/translated (I.e. no posix to nfsv4 or vice versa). We are not 
> > that clever.
> 
> OK, I installed that along with the attached further patch to talk about 
> ACL translation (this can be in the manual rather than the man page as 
> it's reasonably obscure).

Looks good to me.  Thank you both!

Kamil

bug#61035: [PATCH] cp: improve help regarding ACLs

[Kamil Dudka]

On Sunday, January 29, 2023 11:04:22 PM CET Paul Eggert wrote:
> On 2023-01-29 03:06, Kamil Dudka wrote:
> > On Wednesday, January 25, 2023 11:01:45 PM CET Paul Eggert wrote:
> >> On 2023-01-25 13:56, Ondrej Valousek wrote:
> >>> But it's not the same meaning. What I am trying to explain here is that
> >>>
> >>> Cp -p (or cp --preserve=mode) also retains ACLs. This fact is not 
> >>> obvious, but yet it's happening
> >> Then I'm afraid I don't understand. In what sense do ACLs differ from
> >> xattr here?
> > As I understand it, `cp -p` now preserves ACLs but not xattr (unlike `cp 
> > -a`).
> 
> OK, the light is slowly dawning on me. Though I'm still confused.
> 
> Why are ACLs treated differently from extended attributes? Shouldn't the 
> two be treated the same (assuming they're both supported)?
> 
> In other words, what's the underlying model and motivation here? It's 
> more important to document that, than to document little pieces of it.

I cannot speak for Ondrej.  My understanding is that ACLs can be seen as
extension over permission bits whereas the extended attributes can store
pretty much anything.  I am not saying which approach is (more) correct
though.

Kamil

bug#61035: [PATCH] cp: improve help regarding ACLs

[Kamil Dudka]

On Wednesday, January 25, 2023 11:01:45 PM CET Paul Eggert wrote:
> On 2023-01-25 13:56, Ondrej Valousek wrote:
> > But it's not the same meaning. What I am trying to explain here is that
> > 
> > Cp -p (or cp --preserve=mode) also retains ACLs. This fact is not obvious, 
> > but yet it's happening
> 
> Then I'm afraid I don't understand. In what sense do ACLs differ from 
> xattr here?

As I understand it, `cp -p` now preserves ACLs but not xattr (unlike `cp -a`).

Kamil

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

[Kamil Dudka]

On Monday, March 7, 2022 3:26:27 PM CET Pádraig Brady wrote:
> Updated patch for ls attached as per discussion.
> Added a NEWS entry.

Works as expected.  Thank you both!

Kamil

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

[Kamil Dudka]

On Monday, March 7, 2022 3:40:25 PM CET Pádraig Brady wrote:
> New stat patch attached to apply AT_NO_AUTOMOUNT
> unless --cached=never is specified.
> 
> See https://bugs.gnu.org/54286 for more general discussion.
> Marking this as done.
> 
> cheers,
> Pádraig

Works as expected.  Thank you both!

Kamil

bug#52481: chown of coreutils may delete the suid of file

[Kamil Dudka]

On Tuesday, December 14, 2021 3:49:37 AM CET 21625039 wrote:
> I encountered a problem with chown on my fedora34 as the version of
> coreutils is 8.32.
> 
> 
> 
> The reproduce process could see the steps blow:
> 
> [root@fedora ~]# ll test.txt
> 
> -rw-r--r--. 1 root root 0 Dec 13 21:13 test.txt
> 
> [root@fedora ~]# chmod 4750 test.txt
> 
> [root@fedora ~]# ll test.txt
> 
> -rwsr-x---. 1 root root 0 Dec 13 21:13 test.txt
> 
> [root@fedora ~]# chown root:root test.txt
> 
> [root@fedora ~]# ll test.txt
> 
> -rwxr-x---. 1 root root 0 Dec 13 21:13 test.txt

I believe this is already documented [1]:

"The chown command sometimes clears the set-user-ID or set-group-ID
permission bits. This behavior depends on the policy and functionality
of the underlying chown system call, which may make system-dependent
file mode modifications outside the control of the chown command."

Kamil

[1] 
https://www.gnu.org/software/coreutils/manual/html_node/chown-invocation.html

> [root@fedora ~]# rpm -qa coreutils
> 
> coreutils-8.32-19.fc34.x86_64
> 
> [root@fedora ~]# cat /etc/fedora-release
> 
> Fedora release 34 (Thirty Four)
> 
> 
> 
> Looking forward to hearing from you!
> 
> Thanks.

bug#52115: Suggestion: LN command should swap TARGET and LINK_NAME if LINK_NAME already exists

[Kamil Dudka]

On Friday, November 26, 2021 12:10:36 AM CET Warren Parad wrote:
> except mv(1) and cp(1) are both "FROM" and then "TO", but ln is backwards
> from thi, it is "TO" then "FROM", the least the command could do is put
> these in the correct order.
> 
> >  it is a one-time effort to learn the order
> 
> Opinion, do you want proof that people can't learn this, because they
> haven't.
> 
> > The synopsis is already complex and confusing enough:
> Opinion, it is as complex as it allows, sounds like you are saying "LN
> Sucks, we really need 4 commands which are all simpler", sure okay we can
> have another command, but doing the right thing ALWAYS takes precedence
> over "I have an opinion"
> 
> > what happens if another (malicious?) user B creates LINK_TARGET while
> 
> user A is typing the command?
> While typing before entering? Then it doesn't matter if they are reversed
> since the command would still fail because both exist, that should result
> in the only real failure. I'm not suggesting removing the error in all
> cases.
> 
> >   $ ln -nsvf somename othername
> 
> WTF, yeah let's tell everyone that gets this wrong to delete the file they
> want to link, that's a genius idea.

Seriously, such experiments do not belong to the system implementation of 
ln(1).  If you really need this behavior, you can implement it as a shell 
function.  Sooner or later you will regret that you did it.

Kamil

bug#49298: [PATCH] df: do not print duplicated entires with NFS and bind mounts

[Kamil Dudka]

As originally reported in ,
df invoked without -a printed duplicated entries for NFS mounts
of bind mounts.  This is a regression from commit v8.25-54-g1c17f61ef99,
which introduced the use of a hash table.

The proposed patch makes sure that the devlist entry seen the last time
is used for comparison when eliminating duplicated mount entries.  This
way it worked before introducing the hash table.

Patch co-authored by Roberto Bergantinos.

* src/ls.c (struct devlist): Introduce the seen_last pointer.
(devlist_for_dev): Return the devlist entry seen the last time if found.
(filter_mount_list): Remember the devlist entry seen the last time for
each hashed item.
---
 src/df.c | 15 +--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/src/df.c b/src/df.c
index aadc3483f22..16a7b5524df 100644
--- a/src/df.c
+++ b/src/df.c
@@ -54,6 +54,7 @@ struct devlist
   dev_t dev_num;
   struct mount_entry *me;
   struct devlist *next;
+  struct devlist *seen_last; /* valid for hashed devlist entries only */
 };
 
 /* Filled with device numbers of examined file systems to avoid
@@ -681,7 +682,13 @@ devlist_for_dev (dev_t dev)
 return NULL;
   struct devlist dev_entry;
   dev_entry.dev_num = dev;
-  return hash_lookup (devlist_table, &dev_entry);
+
+  struct devlist *found = hash_lookup (devlist_table, &dev_entry);
+  if (found == NULL)
+return NULL;
+
+  /* Return the last devlist entry we have seen with this dev_num */
+  return found->seen_last;
 }
 
 static void
@@ -799,8 +806,12 @@ filter_mount_list (bool devices_only)
   devlist->dev_num = buf.st_dev;
   devlist->next = device_list;
   device_list = devlist;
-  if (hash_insert (devlist_table, devlist) == NULL)
+
+  struct devlist *inserted = hash_insert (devlist_table, devlist);
+  if (inserted == NULL)
 xalloc_die ();
+  /* Remember the last devlist entry we have seen with this dev_num */
+  inserted->seen_last = devlist;
 
   me = me->me_next;
 }
-- 
2.31.1

bug#49239: Unexpected results with sort -V

[Kamil Dudka]

On Monday, June 28, 2021 6:52:14 PM CEST Michael Debertol wrote:
> I was trying to say that the regex is not followed in two cases:
> 
> - when there are two dots followed by [A-Za-z~], the second dot should
> be matched, but it is not.
> 
> An example is "foo..a": In this case ".a" should be matched, but it is
> not (nothing is matched)
> 
> - when there's a trailing dot, the trailing dot is matched even though
> it is not followed by anything
> 
> e.g. "foo." matches the "." as the file ending, but it should not match
> a file ending in this case.
> 
> I hope it's clearer now,
> 
> Michael

You are right.  The matching algorithm was not implemented correctly and
the patch you attached fixes it.  Sorry for missing it in my previous reply.

Kamil

bug#49239: Unexpected results with sort -V

[Kamil Dudka]

On Sunday, June 27, 2021 12:04:53 AM CEST Michael wrote:
> Hi,
> I found some unexpected results with sort -V. I hope this is the correct
> place to send a bug report to [1].
> They are caused by a bug in filevercmp inside gnulib, specifically in the
> function match_suffix.
> I assume it should, as documented, match a file ending as defined by this
> regex: /(\.[A-Za-z~][A-Za-z0-9~]*)*$/
> However, I found two cases where this does not happen:
> 1) Two consecutive dots. It is not checked if the character after a dot is
> a dot. This results in nothing being matched in a case like "a..a", even
> though it should match ".a" according to the regex.
> Testcase: printf "a..a\na.+" | sort -V # a..a should be before a.+ I think
> 2) A trailing dot. If there is no additional character after a dot, it is
> still matched (e.g. for "a." the . is matched).
> Testcase: printf "a.\na+" | sort -V # I think a+ should be before a.

As far as I understand, regex (\.[A-Za-z~][A-Za-z0-9~]*)*$ specifies that each 
dot has to be followed by [A-Za-z~] to be matched.  Am I missing anything?

I am not saying that the current behavior is perfect (a solution that works as 
expected in all scenarios is difficult to find in this case) but, at least, it 
seems to me that it works as it is described.

Kamil

bug#49209: coreutils: stack out-of-bounds write in tail --follow

[Kamil Dudka]

On Sunday, June 27, 2021 3:47:46 AM CEST Paul Eggert wrote:
> When looking into this I decided it was cleaner to fix coreutils by
> using 'poll' instead of 'select', as Kamil suggested. I installed the
> attached patches to do that. The last patch fixes the bug.

This works for me.  Thank you for taking care of the fix!

Kamil

bug#49209: coreutils: stack out-of-bounds write in tail --follow

[Kamil Dudka]

On Thursday, June 24, 2021 4:50:25 PM CEST Pádraig Brady wrote:
> Note the number of descriptors select() is waiting on in independent of the
> number of files. We should be able to inotify_init() earlier in the process
> to avoid this issue. I'll have a look.

Good idea!  This could make it work instead of throwing an error.  
Nevertheless, the boundary check should be added anyway as long as we use 
FD_SET(), because the number of first available file descriptor can still
be controlled from outside.

Kamil

bug#49209: coreutils: stack out-of-bounds write in tail --follow

[Kamil Dudka]

Hello,

As originally reported by Stepan Broz (CC'd), tail --follow crashes when it
is given too many files to follow, and ulimit -n is set to >1024.

FD_SET(wd, &rfd) in tail_forever_inotify() writes beyond the stack-allocated 
variable in case wd >= FD_SETSIZE.  Minimal example:

# mkdir dir
# cd dir
# touch {1..1021}
# ulimit -n 1025
# tail -f *

The out-of-bound write could be fixed like this:

--- a/src/tail.c
+++ b/src/tail.c
@@ -1647,28 +1647,32 @@ tail_forever_inotify (int wd, struct File_spec *f, 
size_t n_files,
   if (writer_is_dead)
 exit (EXIT_SUCCESS);

   writer_is_dead = (kill (pid, 0) != 0 && errno != EPERM);

   if (writer_is_dead)
 delay.tv_sec = delay.tv_usec = 0;
   else
 {
   delay.tv_sec = (time_t) sleep_interval;
   delay.tv_usec = 100 * (sleep_interval - delay.tv_sec);
 }
 }

+   if (FD_SETSIZE <= wd)
+ die (EXIT_FAILURE, 0,
+  _("too many open files to wait for inotify events"));
+
fd_set rfd;
FD_ZERO (&rfd);
FD_SET (wd, &rfd);
if (monitor_output)
  FD_SET (STDOUT_FILENO, &rfd);

int file_change = select (MAX (wd, STDOUT_FILENO) + 1,
  &rfd, NULL, NULL, pid ? &delay: NULL);

if (file_change == 0)
  continue;
else if (file_change == -1)
  die (EXIT_FAILURE, errno,
   _("error waiting for inotify and output events"));


Alternatively, we might rewrite the code to use poll() rather than select().

Kamil

bug#48036: [PATCH] copy: do not refuse to copy a swap file

[Kamil Dudka]

From: Zorro Lang 

* src/copy.c (sparse_copy): Fallback to read() if copy_file_range()
fails with ETXTBSY.  Otherwise it would be impossible to copy files
that are being used as swap.  This used to work before introducing
the support for copy_file_range() in coreutils.
---
 src/copy.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/copy.c b/src/copy.c
index 9dbc694ebee..b7ec9747fa8 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -290,7 +290,7 @@ sparse_copy (int src_fd, int dest_fd, char *buf, size_t 
buf_size,
 if (n_copied < 0)
   {
 if (errno == ENOSYS || errno == EINVAL
-|| errno == EBADF || errno == EXDEV)
+|| errno == EBADF || errno == EXDEV || errno == ETXTBSY)
   break;
 if (errno == EINTR)
   n_copied = 0;
-- 
2.30.2

bug#47383: [PATCH 2/2] ln: fix memory leaks in do_link()

[Kamil Dudka]

On Thursday, March 25, 2021 5:19:56 PM CET Paul Eggert wrote:
> Thanks, I installed that. I then changed "free(" to "free (" as per GNU
> style.

Thank you for incorporating both the patches.

Kamil

bug#47384: [PATCH 1/2] hostname: fix a memory leak with -Dlint

[Kamil Dudka]

On Thursday, March 25, 2021 5:09:44 PM CET Paul Eggert wrote:
> On 3/25/21 9:08 AM, Kamil Dudka wrote:
> > Wasn't that exactly what -Dlint was for when we discussed it the last
> > time?
> 
> Sorry, don't recall the last time.

I meant this thread on bug-gnulib ML:

https://lists.gnu.org/archive/html/bug-gnulib/2020-05/msg00114.html

> This is a borderline area, admittedly.

How does it differ from the following change, which you did not block?


https://git.savannah.gnu.org/gitweb/?p=coreutils.git;a=commitdiff;h=v8.24-21-ga4b640549df

Kamil

bug#47384: [PATCH 1/2] hostname: fix a memory leak with -Dlint

[Kamil Dudka]

On Thursday, March 25, 2021 4:56:15 PM CET Paul Eggert wrote:
> On 3/25/21 3:57 AM, Kamil Dudka wrote:
> > +#ifdef lint
> > +  free(hostname);
> > +#endif
> 
> Let's not do this one. The program is about to exit so there's no need
> to free, and any static-checking tool that complains about a missing
> 'free' here is issuing a false alarm. On this particular issue it's
> better to fix the tools than to clutter upb source code to pacify them.

Wasn't that exactly what -Dlint was for when we discussed it the last time?
I am totally lost in your reasoning again.  But thank you for having a look.

Kamil

bug#47383: [PATCH 2/2] ln: fix memory leaks in do_link()

[Kamil Dudka]

* src/ln.c (do_link): Free memory allocated by convert_abs_rel()
on all code paths.
---
 src/ln.c | 19 +--
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/src/ln.c b/src/ln.c
index e79ca5e7ade..368b109daf0 100644
--- a/src/ln.c
+++ b/src/ln.c
@@ -229,14 +229,14 @@ do_link (char const *source, int destdir_fd, char const 
*dest_base,
   if (errno != ENOENT)
 {
   error (0, errno, _("failed to access %s"), quoteaf (dest));
-  return false;
+  goto fail;
 }
   force = false;
 }
   else if (S_ISDIR (dest_stats.st_mode))
 {
   error (0, 0, _("%s: cannot overwrite directory"), quotef (dest));
-  return false;
+  goto fail;
 }
   else if (seen_file (dest_set, dest, &dest_stats))
 {
@@ -245,7 +245,7 @@ do_link (char const *source, int destdir_fd, char const 
*dest_base,
   error (0, 0,
  _("will not overwrite just-created %s with %s"),
  quoteaf_n (0, dest), quoteaf_n (1, source));
-  return false;
+  goto fail;
 }
   else
 {
@@ -274,7 +274,7 @@ do_link (char const *source, int destdir_fd, char const 
*dest_base,
 {
   error (0, 0, _("%s and %s are the same file"),
  quoteaf_n (0, source), quoteaf_n (1, dest));
-  return false;
+  goto fail;
 }
 }
 
@@ -285,7 +285,10 @@ do_link (char const *source, int destdir_fd, char const 
*dest_base,
   fprintf (stderr, _("%s: replace %s? "),
program_name, quoteaf (dest));
   if (!yesno ())
-return true;
+{
+  free(rel_source);
+  return true;
+}
 }
 
   if (backup_type != no_backups)
@@ -304,7 +307,7 @@ do_link (char const *source, int destdir_fd, char const 
*dest_base,
{
   error (0, rename_errno, _("cannot backup %s"),
  quoteaf (dest));
-  return false;
+  goto fail;
 }
   force = false;
 }
@@ -397,6 +400,10 @@ do_link (char const *source, int destdir_fd, char const 
*dest_base,
   free (backup_base);
   free (rel_source);
   return link_errno <= 0;
+
+fail:
+  free (rel_source);
+  return false;
 }
 
 void
-- 
2.26.3

bug#47384: [PATCH 1/2] hostname: fix a memory leak with -Dlint

[Kamil Dudka]

* src/hostname.c (main): Free allocated memory when compiled
with -Dlint.
---
 src/hostname.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/hostname.c b/src/hostname.c
index 7d13575d4e9..8ec9aad4d34 100644
--- a/src/hostname.c
+++ b/src/hostname.c
@@ -104,6 +104,9 @@ main (int argc, char **argv)
   if (hostname == NULL)
 die (EXIT_FAILURE, errno, _("cannot determine hostname"));
   printf ("%s\n", hostname);
+#ifdef lint
+  free(hostname);
+#endif
 }
 
   if (optind + 1 < argc)
-- 
2.26.3

bug#46613: [PATCH] stat: add support for the exfat file system

[Kamil Dudka]

* src/stat.c (human_fstype): Add case for the 'exfat' file system type.
* NEWS: Mention the Improvement.

Bug: https://bugzilla.redhat.com/1921427
---
 NEWS   | 2 +-
 src/stat.c | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/NEWS b/NEWS
index 79bb25e860..08a58d56a6 100644
--- a/NEWS
+++ b/NEWS
@@ -75,7 +75,7 @@ GNU coreutils NEWS-*- 
outline -*-
   df now recognizes these file systems as remote:
   acfs, coda, fhgfs, gpfs, ibrix, ocfs2, and vxfs.
 
-  stat and tail now know about the "devmem", "vboxsf", and "zonefs"
+  stat and tail now know about the "devmem", "exfat", "vboxsf", and "zonefs"
   file system types.  stat -f -c%T now reports the file system type,
   and tail -f uses polling for "vboxsf" and inotify for the others.
 
diff --git a/src/stat.c b/src/stat.c
index 9baa36392a..7bd1044cb4 100644
--- a/src/stat.c
+++ b/src/stat.c
@@ -361,6 +361,8 @@ human_fstype (STRUCT_STATVFS const *statfsbuf)
   return "efs";
 case S_MAGIC_EROFS_V1: /* 0xE0F5E1E2 local */
   return "erofs";
+case S_MAGIC_EXFAT: /* 0x2011BAB0 local */
+  return "exfat";
 case S_MAGIC_EXFS: /* 0x45584653 local */
   return "exfs";
 case S_MAGIC_EXOFS: /* 0x5DF5 local */
-- 
2.26.2

bug#44308: md5sum (and other *sum utils) path dependent output

[Kamil Dudka]

On Thursday, October 29, 2020 11:41:03 AM CET Alessandro Forghieri wrote:
> Greetings.
> 
> Just stumbled upon this:
> 
> *# md5sum
> ./Development/rss/rss/STAGING/BATTITI_del_27_11_2016_-_Battiti_Live:_Arsene_
> Duevi\\Roberto_Zanisi.mp3 \d41d8cd98f00b204e9800998ecf8427e
> ./Development/rss/rss/STAGING/BATTITI_del_27_11_2016_-_Battiti_Live:_Arsene_
> Duevi\\Roberto_Zanisi.mp3*
> 
> *# cp
> ./Development/rss/rss/STAGING/BATTITI_del_27_11_2016_-_Battiti_Live:_Arsene_
> Duevi\\Roberto_Zanisi.mp3 /tmp/qlo
> # md5sum /tmp/qlo
> d41d8cd98f00b204e9800998ecf8427e  /tmp/qlo**
> *
> 
> IE, in the first instance a '\' character is prepended to the output.
> The file itself is an empty file, so I guess the file content is
> irrelevant to the apparent bug.
> 
> Running coreutils-8.31-6.fc30.x86_64 on Linux 5.6.13-100.fc30.x86_64+debug

This is the documented behavior of md5sum [1]:

"Without ‘--zero’, if FILE contains a backslash or newline, the line is
started with a backslash, and each problematic character in the file name
is escaped with a backslash, making the output unambiguous even in the
presence of arbitrary file names."

Kamil

[1] 
https://www.gnu.org/software/coreutils/manual/html_node/md5sum-invocation.html

> To communicate, cc me, as I am not a ML subscriber.
> 
> Cheers,

bug#44235: [PATCH] dd: drop old workaround for lseek() bug in Linux kernel

[Kamil Dudka]

On Monday, October 26, 2020 2:11:59 PM CET Pádraig Brady wrote:
> On 26/10/2020 10:44, Kamil Dudka wrote:
> > The workaround triggers warnings with new kernel versions in case
> > a user does not have sufficient privileges for the MTIOCGET ioctl.
> > 
> > * src/dd.c (skip_via_lseek): Drop wrapper function no longer needed.
> > (skip): Use lseek() directly.
> > (advance_input_after_read_error): Use lseek() directly.
> > 
> > Reported-by: Nir Soffer
> > Bug: https://bugzilla.redhat.com/1876840
> 
> To clarify, the kernel is warning, not dd.
> In general the kernel should be disallowing rather than warning I think.
> But yes this is old code that should no longer be needed.
> I'll apply the patch.
> 
> thanks,
> Pádraig

Perfect.  Thank you for taking a quick action on this!

Kamil

bug#44235: [PATCH] dd: drop old workaround for lseek() bug in Linux kernel

[Kamil Dudka]

The workaround triggers warnings with new kernel versions in case
a user does not have sufficient privileges for the MTIOCGET ioctl.

* src/dd.c (skip_via_lseek): Drop wrapper function no longer needed.
(skip): Use lseek() directly.
(advance_input_after_read_error): Use lseek() directly.

Reported-by: Nir Soffer
Bug: https://bugzilla.redhat.com/1876840
---
 src/dd.c | 59 +++-
 1 file changed, 3 insertions(+), 56 deletions(-)

diff --git a/src/dd.c b/src/dd.c
index 4b0cddc9df..be8ccc299b 100644
--- a/src/dd.c
+++ b/src/dd.c
@@ -1784,59 +1784,6 @@ advance_input_offset (uintmax_t offset)
 input_offset_overflow = true;
 }
 
-/* This is a wrapper for lseek.  It detects and warns about a kernel
-   bug that makes lseek a no-op for tape devices, even though the kernel
-   lseek return value suggests that the function succeeded.
-
-   The parameters are the same as those of the lseek function, but
-   with the addition of FILENAME, the name of the file associated with
-   descriptor FDESC.  The file name is used solely in the warning that's
-   printed when the bug is detected.  Return the same value that lseek
-   would have returned, but when the lseek bug is detected, return -1
-   to indicate that lseek failed.
-
-   The offending behavior has been confirmed with an Exabyte SCSI tape
-   drive accessed via /dev/nst0 on both Linux 2.2.17 and 2.4.16 kernels.  */
-
-#if defined __linux__ && HAVE_SYS_MTIO_H
-
-# include 
-
-# define MT_SAME_POSITION(P, Q) \
-   ((P).mt_resid == (Q).mt_resid \
-&& (P).mt_fileno == (Q).mt_fileno \
-&& (P).mt_blkno == (Q).mt_blkno)
-
-static off_t
-skip_via_lseek (char const *filename, int fdesc, off_t offset, int whence)
-{
-  struct mtget s1;
-  struct mtget s2;
-  bool got_original_tape_position = (ioctl (fdesc, MTIOCGET, &s1) == 0);
-  /* known bad device type */
-  /* && s.mt_type == MT_ISSCSI2 */
-
-  off_t new_position = lseek (fdesc, offset, whence);
-  if (0 <= new_position
-  && got_original_tape_position
-  && ioctl (fdesc, MTIOCGET, &s2) == 0
-  && MT_SAME_POSITION (s1, s2))
-{
-  if (status_level != STATUS_NONE)
-error (0, 0, _("warning: working around lseek kernel bug for file "
-   "(%s)\n  of mt_type=0x%0lx -- "
-   "see  for the list of types"),
-   filename, s2.mt_type + 0Lu);
-  errno = 0;
-  new_position = -1;
-}
-
-  return new_position;
-}
-#else
-# define skip_via_lseek(Filename, Fd, Offset, Whence) lseek (Fd, Offset, 
Whence)
-#endif
-
 /* Throw away RECORDS blocks of BLOCKSIZE bytes plus BYTES bytes on
file descriptor FDESC, which is open with read permission for FILE.
Store up to BLOCKSIZE bytes of the data at a time in IBUF or OBUF, if
@@ -1858,7 +1805,7 @@ skip (int fdesc, char const *file, uintmax_t records, 
size_t blocksize,
 
   errno = 0;
   if (records <= OFF_T_MAX / blocksize
-  && 0 <= skip_via_lseek (file, fdesc, offset, SEEK_CUR))
+  && 0 <= lseek (fdesc, offset, SEEK_CUR))
 {
   if (fdesc == STDIN_FILENO)
 {
@@ -1893,7 +1840,7 @@ skip (int fdesc, char const *file, uintmax_t records, 
size_t blocksize,
  Or it may not have been done at all (> OFF_T_MAX).
  Therefore try to seek to the end of the file,
  to avoid redundant reading.  */
-  if ((skip_via_lseek (file, fdesc, 0, SEEK_END)) >= 0)
+  if ((lseek (fdesc, 0, SEEK_END)) >= 0)
 {
   /* File is seekable, and we're at the end of it, and
  size <= OFF_T_MAX. So there's no point using read to advance.  */
@@ -1998,7 +1945,7 @@ advance_input_after_read_error (size_t nbytes)
   diff = input_offset - offset;
   if (! (0 <= diff && diff <= nbytes) && status_level != STATUS_NONE)
 error (0, 0, _("warning: invalid file offset after failed read"));
-  if (0 <= skip_via_lseek (input_file, STDIN_FILENO, diff, SEEK_CUR))
+  if (0 <= lseek (STDIN_FILENO, diff, SEEK_CUR))
 return true;
   if (errno == 0)
 error (0, 0, _("cannot work around kernel bug after all"));
-- 
2.25.4

bug#43998: 8.32: test suite is failing

[Kamil Dudka]

On Wednesday, October 14, 2020 11:43:08 PM CEST Tomasz Kłoczko wrote:
> Hi,
> 
> Two units are failing in my build env:
> 
> FAIL: tests/cp/fiemap-FMR
> =
> 
> ==420069== Syscall param ioctl(generic) points to unaddressable byte(s)
> ==420069==at 0x49B94AB: ioctl (in /usr/lib64/libc-2.32.9000.so)
> ==420069==by 0x11D851: UnknownInlinedFun (copy.c:371)
> ==420069==by 0x11D851: copy_reg (copy.c:1239)
> ==420069==by 0x11F0CA: copy_internal.constprop.0 (copy.c:2716)
> ==420069==by 0x122DEA: copy.constprop.0 (copy.c:3024)
> ==420069==by 0x123172: do_copy (cp.c:780)
> ==420069==by 0x10DAA9: main (cp.c:1232)
> ==420069==  Address 0x3 is not stack'd, malloc'd or (recently) free'd
> ==420069==
> FAIL tests/cp/fiemap-FMR.sh (exit status: 1)

This is a problem in valgrind which does not know how to handle FICLONE
ioctl.  valgrind treats the operand of FICLONE as address 0x3 while the
ioctl is taking a valid file descriptor 3 in this case.

Kamil

bug#37702: Suggestion for 'df' utility

[Kamil Dudka]

On Sunday, May 31, 2020 2:49:24 PM CEST Pádraig Brady wrote:
> On 31/05/2020 10:36, Bernhard Voelker wrote:
> > What about to start with this?
> > 
> >$ GIT_PAGER= git -C gnulib diff
> >diff --git a/lib/mountlist.c b/lib/mountlist.c
> >index 7abe0248e..5f6249dec 100644
> >--- a/lib/mountlist.c
> >+++ b/lib/mountlist.c
> >@@ -164,6 +164,9 @@
> >
> > #define ME_DUMMY_0(Fs_name, Fs_type)\
> > 
> >   (strcmp (Fs_type, "autofs") == 0  \
> >
> >+   || strcmp (Fs_type, "tmpfs") == 0\
> 
> tmpfs is consumable, so wouldn't be appropriate to add I think

+1

No space left on a tmpfs file system mounted e.g. on /tmp is a common problem
that should be easily visible from the default output of df.

Kamil

bug#41563: Possible bug with 'sort -Vr' version sorting

[Kamil Dudka]

On Thursday, May 28, 2020 11:02:43 AM CEST Erik Auerswald wrote:
> On Thu, May 28, 2020 at 08:48:16AM +0200, Kamil Dudka wrote:
> > It is the underscore in the .x86_64 suffix what breaks the version compare
> > algorithm.  If you replace the underscore by an alphabetic character, it
> > sorts as you expect:
> > 
> > # ls -t /boot/vmlinuz-* | sed "s/\/boot\/vmlinuz-//g" | grep -v rescue | \
> > 
> > sed 's/x86_64/x86X64/' | sort -Vr | sed 's/x86X64/x86_64/'
> > 
> > 3.10.0-1127.8.2.el7.x86_64
> > 3.10.0-1127.el7.x86_64
> > 3.10.0-1062.18.1.el7.x86_64
> 
> That is interesting.  The underscore can be replaced by a digit or even
> removed as well.  Replacing it with a dot (.)  does not help.

If there is no underscore, the .el7.x86X64 suffix is recognized as file
extension.  See the corresponding documentation:

https://www.gnu.org/software/coreutils/manual/html_node/Special-handling-of-file-extensions.html

> This differs from Debian's "dpkg --compare-versions", where the results
> of the comparison do not change by replacing the underscore with a
> digit or character, or by removing it (the underscore is identified as
> problematic, though):

The problem is that `dpkg --compare-versions` expects version numbers only.
It does not work well if you feed it with file names including extensions:

$ dpkg --compare-versions 3.10.0-1127.8.2 '>>' 3.10.0-1127 && echo '>>' || echo 
'<='
>>
$ dpkg --compare-versions 3.10.0-1127.8.2.bz2 '>>' 3.10.0-1127.bz2 && echo '>>' 
|| echo '<='
<=

> $ dpkg --compare-versions 3.10.0-1127.8.2.el7.x86_64 lt
> 3.10.0-1127.el7.x86_64 && echo less dpkg: warning: version
> '3.10.0-1127.8.2.el7.x86_64' has bad syntax: invalid character in revision
> number dpkg: warning: version '3.10.0-1127.el7.x86_64' has bad syntax:
> invalid character in revision number less
> $ dpkg --compare-versions 3.10.0-1127.8.2.el7.x86.64 lt
> 3.10.0-1127.el7.x86.64 && echo less less
> $ dpkg --compare-versions 3.10.0-1127.8.2.el7.x86X64 lt
> 3.10.0-1127.el7.x86X64 && echo less less
> $ dpkg --compare-versions 3.10.0-1127.8.2.el7.x86164 lt
> 3.10.0-1127.el7.x86164 && echo less less
> $ dpkg --compare-versions 3.10.0-1127.8.2.el7.x8664 lt
> 3.10.0-1127.el7.x8664 && echo less less
> 
> The way I read the GNU Coreutils documentation, removing the underscore
> should not affect the version sort comparison result.

Not really.  See the link above to the documentation that covers this part.

Kamil

> Thanks,
> Erik

bug#41563: Possible bug with 'sort -Vr' version sorting

[Kamil Dudka]

On Wednesday, May 27, 2020 2:07:32 PM CEST Danie de Jager via GNU coreutils 
Bug Reports wrote:
> Hi,
> 
> I use sort -Vr to sort version numbers. I noticed this discrepancy on
> the latest kernel version from Centos 7.8.
> 
> command to get output:
> # ls -t /boot/vmlinuz-* | sed "s/\/boot\/vmlinuz-//g" | grep -v rescue
> 
> | sort -Vr
> 
> 3.10.0-1127.el7.x86_64
> 3.10.0-1127.8.2.el7.x86_64
> 3.10.0-1062.18.1.el7.x86_64

It is the underscore in the .x86_64 suffix what breaks the version compare 
algorithm.  If you replace the underscore by an alphabetic character, it
sorts as you expect:

# ls -t /boot/vmlinuz-* | sed "s/\/boot\/vmlinuz-//g" | grep -v rescue | \
sed 's/x86_64/x86X64/' | sort -Vr | sed 's/x86X64/x86_64/'

3.10.0-1127.8.2.el7.x86_64
3.10.0-1127.el7.x86_64
3.10.0-1062.18.1.el7.x86_64

Kamil

> I'd expect the middle value to be the highest version number. Is this
> by design or a bug? If it is a bug please let me know if I must log it
> somewhere.
> 
> Version details:
> # sort --version
> sort (GNU coreutils) 8.22
> Copyright (C) 2013 Free Software Foundation, Inc.
> License GPLv3+: GNU GPL version 3 or later
> . This is free software: you are free to
> change and redistribute it. There is NO WARRANTY, to the extent permitted
> by law.
> 
> Written by Mike Haertel and Paul Eggert.
> 
> Regards,
> Danie de Jager

bug#39929: coreutils-8.32 fails to build on aarch64

[Kamil Dudka]

On Thursday, March 5, 2020 6:39:23 PM CET Pádraig Brady wrote:
> On 05/03/2020 16:21, Kamil Dudka wrote:
> > While trying to build coreutils-8.32 for Fedora on aarch64, I got the
> > following compilation failure:
> > 
> > ../src/ls.c: In function 'print_dir':
> > ../src/ls.c:3026:24: error: 'SYS_getdents' undeclared (first use in this
> > function); did you mean 'SYS_getdents64'?> 
> >   3026 |   if (syscall (SYS_getdents, dirfd (dirp), NULL, 0) == -1
> >   
> >|^~~~
> >|SYS_getdents64
> > 
> > ../src/ls.c:3026:24: note: each undeclared identifier is reported only
> > once for each function it appears in
> > 
> > 
> > Sorry for reporting it late.  I tried to build 8.31.90-cc4c and
> > 8.31.99-f2034 in Fedora copr but forgot to enable aarch64, so these
> > builds succeeded  :-/
> Ah well.
> Does the attached address this for you.

Thanks!  I confirm that the ls/removed-directory test passes with the patch
on all architectures that Fedora builds for:

https://koji.fedoraproject.org/koji/taskinfo?taskID=42242960

Kamil

bug#39929: coreutils-8.32 fails to build on aarch64

[Kamil Dudka]

While trying to build coreutils-8.32 for Fedora on aarch64, I got the following
compilation failure:

../src/ls.c: In function 'print_dir':
../src/ls.c:3026:24: error: 'SYS_getdents' undeclared (first use in this 
function); did you mean 'SYS_getdents64'?
 3026 |   if (syscall (SYS_getdents, dirfd (dirp), NULL, 0) == -1
  |^~~~
  |SYS_getdents64
../src/ls.c:3026:24: note: each undeclared identifier is reported only once for 
each function it appears in


Sorry for reporting it late.  I tried to build 8.31.90-cc4c and 8.31.99-f2034
in Fedora copr but forgot to enable aarch64, so these builds succeeded  :-/

Kamil

bug#39485: [PATCH] make tests/cp/preserve-gid work with single binary

[Kamil Dudka]

On Saturday, February 8, 2020 12:53:52 PM CET Pádraig Brady wrote:
> Considering --enable-single-binary-exceptions may exclude cp,
> and also that coreutils(1) may be compiled during
> non --enable-single-binary build when building
> all programs for `make syntax-check` etc.
> we should add an extra guard.
> 
> Also we can simplify by just copying coreutils to cp.
> 
> I'll amend your change with the following and push later.
> Marking this as done.
> 
> thanks!
> Pádraig
> 
> diff --git a/tests/cp/preserve-gid.sh b/tests/cp/preserve-gid.sh
> index 41cdbc6ca..bba09df09 100755
> --- a/tests/cp/preserve-gid.sh
> +++ b/tests/cp/preserve-gid.sh
> @@ -110,10 +110,11 @@ cleanup_() { rm -rf "$tmp_path"; }
>   # is not readable by our nameless IDs.
>   test -d /tmp && TMPDIR=/tmp
>   tmp_path=$(mktemp -d) || fail_ "failed to create temporary directory"
> -if test -x "$abs_path_dir_/coreutils"; then
> +if test -x "$abs_path_dir_/coreutils" &&
> +   { test -l "$abs_path_dir_/cp" ||

This should be spelled `test -L` I guess.

> + test $(wc -l < "$abs_path_dir_/cp") = 1; } then
> # if configured with --enable-single-binary we need to use the single
> binary -  cp "$abs_path_dir_/coreutils" "$tmp_path"
> -  echo "#!$tmp_path/coreutils --coreutils-prog-shebang=cp" > "$tmp_path/cp"
> +  cp "$abs_path_dir_/coreutils" "$tmp_path/cp" || framework_failure_ else
> cp "$abs_path_dir_/cp" "$tmp_path"
>   fi

Otherwise looks good.  Thank you for fixing it!

Kamil

bug#39485: [PATCH] make tests/cp/preserve-gid work with single binary

[Kamil Dudka]

* tests/cp/preserve-gid.sh: If configured with --enable-single-binary
copy the coreutils single binary, instead of the cp one-line launcher.

Bug: https://bugzilla.redhat.com/1800597
---
 tests/cp/preserve-gid.sh | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/tests/cp/preserve-gid.sh b/tests/cp/preserve-gid.sh
index e48584c1e..41cdbc6ca 100755
--- a/tests/cp/preserve-gid.sh
+++ b/tests/cp/preserve-gid.sh
@@ -110,7 +110,13 @@ cleanup_() { rm -rf "$tmp_path"; }
 # is not readable by our nameless IDs.
 test -d /tmp && TMPDIR=/tmp
 tmp_path=$(mktemp -d) || fail_ "failed to create temporary directory"
-cp "$abs_path_dir_/cp" "$tmp_path"
+if test -x "$abs_path_dir_/coreutils"; then
+  # if configured with --enable-single-binary we need to use the single binary
+  cp "$abs_path_dir_/coreutils" "$tmp_path"
+  echo "#!$tmp_path/coreutils --coreutils-prog-shebang=cp" > "$tmp_path/cp"
+else
+  cp "$abs_path_dir_/cp" "$tmp_path"
+fi
 chmod -R a+rx "$tmp_path"
 
 t1() {
-- 
2.21.1

bug#39357: tests/cp/proc-short-read.sh fails in modern build environment

[Kamil Dudka]

On Tuesday, February 4, 2020 1:45:06 AM CET Pádraig Brady wrote:
> On 30/01/2020 13:53, Kamil Dudka wrote:
> > tests/cp/proc-short-read.sh expects that a pair of subsequent reads from
> > /proc/kallsyms will always return the same content.  This does not seem to
> > be a safe assumption any more.  The test has started to fail in our build
> > environment.  I am not sure how to fix the test.  We could probably make
> > it use another file from /proc but most of them are much smaller than
> > kallsyms and/or suffer from the same problem.  Output of the failing test
> > follows.
> > 
> > Kamil
> > 
> > 
> > FAIL: tests/cp/proc-short-read
> > ==
> > 
> > + compare_ 1 2
> > + diff -u 1 2
> > --- 1   2020-01-29 12:04:36.923963121 +
> > +++ 2   2020-01-29 12:04:37.026963484 +
> > @@ -114819,81 +114819,132 @@
> > 
> >    t nfs_file_direct_read.cold  [nfs]
> >    t nfs_file_direct_write.cold [nfs]
> >    r .LC0   [nfs]
> > 
> > - r .LC2[nfs]
> > - r __ksymtab_nfs_pgio_current_mirror   [nfs]
> > - r __kstrtab_nfs_pgio_current_mirror   [nfs]
> > 
> > + r __func__.87038  [nfs]
> > + t __nfs_revalidate_inode.cold [nfs]
> > + t nfs_revalidate_mapping.cold [nfs]
> > + d nfs_net_ops [nfs]
> > + t exit_nfs_fs [nfs]
> > + r __param_enable_ino64[nfs]
> > + r __param_str_enable_ino64[nfs]
> > + r .LC15   [nfs]
> > + r __ksymtab_nfs_fs_type   [nfs]
> > + r __kstrtab_nfs_fs_type   [nfs]
> > 
> > + fail=1
> > + md5sum /proc/kallsyms
> > + md5sum 2
> > + sed 's/ .*//' 3
> > + sed 's/ .*//' 4
> > + compare sum.proc sum.2
> > + compare_dev_null_ sum.proc sum.2
> > + test 2 = 2
> > + test xsum.proc = x/dev/null
> > + test xsum.2 = x/dev/null
> > + return 2
> > + case $? in
> > + compare_ sum.proc sum.2
> > + diff -u sum.proc sum.2
> > --- sum.proc2020-01-29 12:04:37.172963999 +
> > +++ sum.2   2020-01-29 12:04:37.175964009 +
> > @@ -1 +1 @@
> > -226cd09830f68c56edda0b9272be66e4
> > +37d7e78173b2a31d5f27cc66aa52e72a
> > + fail=1
> 
> Interesting.
> The attached changes to /proc/cpuinfo
> which is a bit more awkward, but should be a valid test most of the time,
> and is also the file for which the original bug report was against.
> 
> cheers,
> Pádraig

Neither the content of /proc/cpuinfo is guaranteed to be immutable because 
CPUs can go online/offline at run time.  Anyway, the proposed patch has
passed my quick test.  So I think it is an improvement over status quo. 

Thanks!

Kamil

bug#39357: tests/cp/proc-short-read.sh fails in modern build environment

[Kamil Dudka]

tests/cp/proc-short-read.sh expects that a pair of subsequent reads from
/proc/kallsyms will always return the same content.  This does not seem to
be a safe assumption any more.  The test has started to fail in our build
environment.  I am not sure how to fix the test.  We could probably make
it use another file from /proc but most of them are much smaller than
kallsyms and/or suffer from the same problem.  Output of the failing test
follows.

Kamil


FAIL: tests/cp/proc-short-read
==
++ initial_cwd_=/builddir/build/BUILD/coreutils-8.31/separate
+++ testdir_prefix_
+++ printf gt
++ pfx_=gt
+++ mktempd_ /builddir/build/BUILD/coreutils-8.31/separate 
gt-proc-short-read.sh.
+++ case $# in
+++ destdir_=/builddir/build/BUILD/coreutils-8.31/separate
+++ template_=gt-proc-short-read.sh.
+++ MAX_TRIES_=4
+++ case $destdir_ in
+++ destdir_slash_=/builddir/build/BUILD/coreutils-8.31/separate/
+++ case $template_ in
 unset TMPDIR
+++ d=/builddir/build/BUILD/coreutils-8.31/separate/gt-proc-short-read.sh.lINR
+++ case $d in
+++ :
+++ test -d 
/builddir/build/BUILD/coreutils-8.31/separate/gt-proc-short-read.sh.lINR
 ls -dgo 
/builddir/build/BUILD/coreutils-8.31/separate/gt-proc-short-read.sh.lINR
+++ perms='drwx--. 2 4096 Jan 29 12:04 
/builddir/build/BUILD/coreutils-8.31/separate/gt-proc-short-read.sh.lINR'
+++ case $perms in
+++ :
+++ echo 
/builddir/build/BUILD/coreutils-8.31/separate/gt-proc-short-read.sh.lINR
+++ return
++ 
test_dir_=/builddir/build/BUILD/coreutils-8.31/separate/gt-proc-short-read.sh.lINR
++ cd /builddir/build/BUILD/coreutils-8.31/separate/gt-proc-short-read.sh.lINR
++ gl_init_sh_nl_='
'
++ IFS='
'
++ for sig_ in 1 2 3 13 15
+++ expr 1 + 128
++ eval 'trap '\''Exit 129'\'' 1'
+++ trap 'Exit 129' 1
++ for sig_ in 1 2 3 13 15
+++ expr 2 + 128
++ eval 'trap '\''Exit 130'\'' 2'
+++ trap 'Exit 130' 2
++ for sig_ in 1 2 3 13 15
+++ expr 3 + 128
++ eval 'trap '\''Exit 131'\'' 3'
+++ trap 'Exit 131' 3
++ for sig_ in 1 2 3 13 15
+++ expr 13 + 128
++ eval 'trap '\''Exit 141'\'' 13'
+++ trap 'Exit 141' 13
++ for sig_ in 1 2 3 13 15
+++ expr 15 + 128
++ eval 'trap '\''Exit 143'\'' 15'
+++ trap 'Exit 143' 15
++ trap remove_tmp_ 0
+ path_prepend_ ./src
+ test 1 '!=' 0
+ path_dir_=./src
+ case $path_dir_ in
+ abs_path_dir_=/builddir/build/BUILD/coreutils-8.31/separate/./src
+ case $abs_path_dir_ in
+ 
PATH=/builddir/build/BUILD/coreutils-8.31/separate/./src:/builddir/build/BUILD/coreutils-8.31/separate/src:/usr/bin:/bin:/usr/sbin:/sbin:/usr/local/sbin
+ create_exe_shims_ /builddir/build/BUILD/coreutils-8.31/separate/./src
+ case $EXEEXT in
+ return 0
+ shift
+ test 0 '!=' 0
+ export PATH
+ print_ver_ cp
+ require_built_ cp
+ skip_=no
+ for i in "$@"
+ case " $built_programs " in
+ test no = yes
+ test yes = yes
+ local i
+ for i in $*
+ env cp --version
cp (GNU coreutils) 8.31
Copyright (C) 2019 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later .
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.
Written by Torbjorn Granlund, David MacKenzie, and Jim Meyering.
+ kall=/proc/kallsyms
+ test -r /proc/kallsyms
+ cp /proc/kallsyms 1
+ cat /proc/kallsyms
+ compare 1 2
+ compare_dev_null_ 1 2
+ test 2 = 2
+ test x1 = x/dev/null
+ test x2 = x/dev/null
+ return 2
+ case $? in
+ compare_ 1 2
+ diff -u 1 2
--- 1   2020-01-29 12:04:36.923963121 +
+++ 2   2020-01-29 12:04:37.026963484 +
@@ -114819,81 +114819,132 @@
  t nfs_file_direct_read.cold   [nfs]
  t nfs_file_direct_write.cold  [nfs]
  r .LC0[nfs]
- r .LC2[nfs]
- r __ksymtab_nfs_pgio_current_mirror   [nfs]
- r __kstrtab_nfs_pgio_current_mirror   [nfs]
- r __ksymtab_nfs_pgheader_init [nfs]
- r __kstrtab_nfs_pgheader_init [nfs]
- r __ksymtab_nfs_async_iocounter_wait  [nfs]
- r __kstrtab_nfs_async_iocounter_wait  [nfs]
- r __ksymtab_nfs_release_request   [nfs]
- r __kstrtab_nfs_release_request   [nfs]
- r __ksymtab_nfs_wait_on_request   [nfs]
- r __kstrtab_nfs_wait_on_request   [nfs]
- r __ksymtab_nfs_generic_pg_test   [nfs]
- r __kstrtab_nfs_generic_pg_test   [nfs]
- r __ksymtab_nfs_pgio_header_alloc [nfs]
- r __kstrtab_nfs_pgio_header_alloc [nfs]
- r __ksymtab_nfs_pgio_header_free  [nfs]
- r __kstrtab_nfs_pgio_header_free  [nfs]
- r __ksymtab_nfs_initiate_pgio [nfs]
- r __kstrtab_nfs_initiate_pgio [nfs]
- r __ksymtab_nfs_generic_pgio  [nfs]
- r __kstrtab_nfs_generic_pgio  [nfs]
- r __ksymtab_nfs_pageio_resend [nfs]
- r

bug#37702: Suggestion for 'df' utility

[Kamil Dudka]

On Monday, October 14, 2019 8:06:47 AM CEST Paul Eggert wrote:
> On 10/13/19 3:00 PM, Assaf Gordon wrote:
> > I'm not sure if it's easy to find a set of criteria
> > that would work well while having minimal unexpected side effects of
> > hiding
> > entries people in other systems do expect to see.
> 
> No matter what we do (even if we do nothing), there will be problems.

This is not an excuse to introduce new problems.

> But doing
> nothing is clearly a bad idea, as the output of plain df is quite bad
> right now in typical use. We can do better than that, even if we cannot be
> perfect and we cause problems by changing defaults.

I would prefer _not_ to change the default behavior (including the behavior
of commonly used options).  There are downstream consumers who care about 
compatibility in the first place.

Kamil

> > Out of curiosity,
> > can you share the output of the following commands on the same system?
> 
> Sure, here it is:
> 
> $ lsblk
> NAME  MAJ:MIN RM   SIZE RO TYPE  MOUNTPOINT
> loop0   7:00 140.7M  1 loop  /snap/gnome-3-26-1604/90
> loop1   7:10  44.2M  1 loop  /snap/gtk-common-themes/1353
> loop2   7:20 149.9M  1 loop  /snap/gnome-3-28-1804/71
> loop3   7:30   3.7M  1 loop  /snap/gnome-system-monitor/100
> loop4   7:40  14.8M  1 loop  /snap/gnome-characters/317
> loop5   7:50   956K  1 loop  /snap/gnome-logs/73
> loop6   7:60 149.9M  1 loop  /snap/gnome-3-28-1804/67
> loop7   7:70   3.7M  1 loop  /snap/gnome-system-monitor/95
> loop8   7:80 4M  1 loop  /snap/gnome-calculator/406
> loop9   7:90  54.5M  1 loop  /snap/core18/1192
> loop10  7:10   089M  1 loop  /snap/core/7713
> loop11  7:11   0  42.8M  1 loop  /snap/gtk-common-themes/1313
> loop12  7:12   0  14.8M  1 loop  /snap/gnome-characters/296
> loop13  7:13   0 140.7M  1 loop  /snap/gnome-3-26-1604/92
> loop14  7:14   0  89.1M  1 loop  /snap/core/7917
> loop15  7:15   0   956K  1 loop  /snap/gnome-logs/81
> loop16  7:16   0   4.2M  1 loop  /snap/gnome-calculator/501
> loop17  7:17   0  54.4M  1 loop  /snap/core18/1144
> sda 8:00 111.8G  0 disk
> ├─sda1  8:10  96.9G  0 part  /
> ├─sda2  8:20 1K  0 part
> └─sda5  8:5015G  0 part  [SWAP]
> sdb 8:16   0   2.7T  0 disk
> └─sdb1  8:17   0   2.7T  0 part
>└─md127   9:127  0   2.7T  0 raid1
>  └─md127p1 259:00   2.7T  0 md/home
> sdc 8:32   0   2.7T  0 disk
> └─sdc1  8:33   0   2.7T  0 part
>└─md127   9:127  0   2.7T  0 raid1
>  └─md127p1 259:00   2.7T  0 md/home
> sdd 8:48   1   7.5G  0 disk
> └─sdd1  8:49   1   7.5G  0 part  /media/eggert/B827-D456
> sr011:01  1024M  0 rom
> $ df -x tmpfs -x devtmpfs -x squashfs
> Filesystem  1K-blocks   Used  Available Use% Mounted on
> /dev/sda199431552   11740452   82597212  13% /
> /dev/md127p1   2884021472 1326329584 1411168908  49% /home
> /dev/sdd1 781286447051363107728  61% /media/eggert/B827-D456

bug#35531: problem with ls in coreutils

[Kamil Dudka]

On Friday, May 3, 2019 5:56:35 PM CEST Viktors Berstis wrote:
> I don't think the problem has anything to do with sorting or -U1.

It was unclear what you meant by "the problem" so I pointed out the only 
inefficiency that was immediately obvious to me.

> When ls is taking over 5 minutes for something that should run in a
> couple of seconds, the task manager shows that it is using nearly no
> CPU it is doing a lot of  "other I/O".

You can try to use some profiling/tracing tools to debug the root cause.

> It doesn't look like the build you referenced is designed to be
> compileable for Windows.  Is there one that is?  Thanks.

I would suggest to build the latest upstream release (coreutils-8.31 now) 
from:

https://www.gnu.org/software/coreutils/

Kamil

bug#35531: problem with ls in coreutils

[Kamil Dudka]

On Friday, May 3, 2019 5:43:20 AM CEST Viktors Berstis wrote:
> I downloaded it from
> https://sourceforge.net/projects/gnuwin32/files/coreutils/5.3.0/coreutils-5.
> 3.0.exe/download The help said "Report bugs to "
> which is what I did. The build is so old that I suspect none of the
> original players are around. Do you know of a windows binary or windows
> source that is newer
> anywhere?  Thanks.
> 
> - Viktors Berstis

`ls -U1` will not run significantly faster than `ls` on powerful hardware.  
The key difference is that `ls -U1` prints the results continuously as the 
list of files is read from file system whereas `ls` will be silent until
the complete list is read.  You need to use a new enough version of coreutils 
for this to work properly.  This optimisation was introduced in coreutils-7.5:

https://git.savannah.gnu.org/cgit/coreutils.git/commit/?id=v7.0~113
https://git.savannah.gnu.org/cgit/coreutils.git/commit/?id=v7.5~49

Kamil

> Paul Eggert wrote:
> > On 5/2/19 5:41 PM, Viktors Berstis wrote:
> >> The newer version of "ls" built for Windows has the problem.
> > 
> > Ah, then you'll have to talk to whoever built that version, which is not
> > me (and generally speaking they don't hang out on this mailing list).

bug#35531: problem with ls in coreutils

[Kamil Dudka]

On Thursday, May 2, 2019 12:03:31 AM CEST Viktors Berstis wrote:
> When running "ls" or "ls -U" on a windows directory containing 5
> files, ls takes forever.  Something seems to be highly inefficient in there.

Could you please try it with ls -U -1?

Kamil

> This is for the 64 bit version build 4/20/2005 11:41AM.  The exe size is
> 180736 bytes.
> 
> Thanks.
> 
> - Viktors Berstis

bug#34856: [PATCH] *sum --help: add note about binary/text mode

[Kamil Dudka]

On Monday, March 18, 2019 9:11:59 AM CET Bernhard Voelker wrote:
> On 3/14/19 1:48 PM, Kamil Dudka wrote:
> > +Note: There is no difference between binary mode and text mode on GNU
> > systems.\
> Thanks for the patch - I see this is a downstream patch in Fedora since
> several years.

Yes, our long term plan is to move all such patches upstream or drop them.

> I changed the Subject line to conform to our 'commit-msg' hook:
> 
>   md5sum,b2sum,sha*sum: --help: add note about binary/text mode
> 
> and pushed at:
> 
>   https://git.sv.gnu.org/cgit/coreutils.git/commit/?id=ae61b10663

Thanks!

Kamil

> Have a nice day,
> Berny

bug#33646: [PATCH] doc: improve wording of the --kibibytes option description

[Kamil Dudka]

On Friday, March 15, 2019 7:30:55 PM CET Assaf Gordon wrote:
> Pushed here:
> https://git.savannah.gnu.org/cgit/coreutils.git/commit/?id=6bd78f27fdc2df89b
> 1219921c6f5735885f15e37
> 
> -assaf

Thanks!

Kamil

bug#33646: [PATCH] doc: improve wording of the --kibibytes option description

[Kamil Dudka]

On Friday, January 18, 2019 8:26:18 AM CET Assaf Gordon wrote:
> Hello,
> 
> On 2018-12-06 6:32 a.m., Kamil Dudka wrote:
> > Bug: https://bugzilla.redhat.com/1527391
> > ---
> > 
> >   doc/coreutils.texi | 8 +---
> >   1 file changed, 5 insertions(+), 3 deletions(-)
> > 
> > diff --git a/doc/coreutils.texi b/doc/coreutils.texi
> > index f8339d73f..e93fe71a0 100644
> > --- a/doc/coreutils.texi
> > +++ b/doc/coreutils.texi
> > @@ -7975,9 +7975,11 @@ Append @samp{*} for executable regular files,
> > otherwise behave as for> 
> >   @opindex --kibibytes
> >   Set the default block size to its normal value of 1024 bytes,
> >   overriding any contrary specification in environment variables
> > 
> > -(@pxref{Block size}).  This option is in turn overridden by the
> > -@option{--block-size}, @option{-h} or @option{--human-readable}, and
> > -@option{--si} options.
> > +(@pxref{Block size}).  If @option{--block-size}, @option{-h},
> > +@option{--human-readable}, or @option{--si} options are used,
> > +they take precedence over @option{-k} or @option{--kibibytes}
> > +even if @option{-k} or @option{--kibibytes} is placed after
> > +the other options.
> > 
> >   The @option{-k} or @option{--kibibytes} option affects the
> >   per-directory block count written by the @option{-l} and similar
> 
> I'm ok with this improvement - if there are no comments
> I'll push in the next few days.
> 
> -assaf

I can see no more comments on this.  Could you please proceed to push it?

Kamil

bug#34856: [PATCH] *sum --help: add note about binary/text mode

[Kamil Dudka]

* src/md5sum.c (usage): Make it clear that there is no difference
between binary mode and text mode on GNU systems.

Bug: https://bugzilla.redhat.com/406981
Bug: https://bugzilla.redhat.com/1688740
---
 src/md5sum.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/md5sum.c b/src/md5sum.c
index 3532f7b7a..f75b6de02 100644
--- a/src/md5sum.c
+++ b/src/md5sum.c
@@ -287,7 +287,10 @@ The following five options are useful only when verifying 
checksums:\n\
 The sums are computed as described in %s.  When checking, the input\n\
 should be a former output of this program.  The default mode is to print a\n\
 line with checksum, a space, a character indicating input mode ('*' for 
binary,\
-\n' ' for text or where binary is insignificant), and name for each FILE.\n"),
+\n' ' for text or where binary is insignificant), and name for each FILE.\n\
+\n\
+Note: There is no difference between binary mode and text mode on GNU systems.\
+\n"),
   DIGEST_REFERENCE);
   emit_ancillary_info (PROGRAM_NAME);
 }
-- 
2.17.2

bug#33644: [PATCH] cp --preserve=xattr: preserve NFSv4 ACL extended attributes

[Kamil Dudka]

On Sunday, March 3, 2019 3:07:53 AM CET Pádraig Brady wrote:
> So attr_copy_file() copies all except those defined in /etc/xattr.conf

... which is, however, not how xattr.conf is currently documented:

# /etc/xattr.conf
#
# Format:
#  
#
# Actions:
#   permissions - copy when trying to preserve permissions.
#   skip - do not copy.

> ACL xattrs are listed in that file with the rationale from a comment in
> libattr being:
> 
>  "ACLs are excluded by default because copying them between
>   file systems with and without ACL support needs some
>   additional logic so that no unexpected permissions result."
> 
> So the ACL handling specifically is deferred to libacl.
> Now system.posix_acl_access is handled by libacl,
> but system.nfs4_acl is not.

True.

> So I think the correct fix here is to remove the
> nfs entries from /etc/xattr.conf, and then cp will copy.

OK, I will propose it on the acl-devel mailing list, together with updating 
the documentation of xattr.conf.  We will see what attr/acl upstream thinks 
about it.

> This has the advantage of being configurable,
> and also removes nfs4 specific handling from cp.
> Any nfs4 specific handling should be in libacl.

So you think it should.  Nevertheless, in reality, libacl is not aware
of NFSv4 ACLs at all.  And I am not aware of any plans to change this.

Kamil

> thanks,
> Pádraig

bug#33644: [PATCH] cp --preserve=xattr: preserve NFSv4 ACL extended attributes

[Kamil Dudka]

On Monday, February 11, 2019 7:30:42 PM CET Pádraig Brady wrote:
> On 11/02/19 03:50, Kamil Dudka wrote:
> > I think that the information in xattr.conf is correct.  system.nfs4_acl is
> > really an attribute one wants to copy when trying to preserve permissions.
> 
> Right. What I was getting at was attr_copy_file() from libattr seems
> to skip all entries in xattr.conf by default. I need to dig in to
> see what's preserving system.posix_acl_access (these might be
> implicitly generated upon attr reading for example).

I do not know the reasoning behind the default behavior of attr_copy_file().
There is a comment before the function definition but it does not talk about
NFSv4 ACLs:

http://git.savannah.nongnu.org/cgit/attr.git/tree/libattr/attr_copy_file.c?id=cb4786f1#n54

> My question was why does coreutils need to explicitly handle
> the nfs4 acls if it doesn't need to handle the posix ones.

I think the answer is obvious.  cp is able preserve POSIX ACLs at a higher
level (using gnulib's acl module, which uses libacl internally on Linux).
There is, unfortunately, no such module (neither library) for NFSv4 ACLs.
So copying the value of the low-level attribute is currently the only way
to make cp preserve NFSv4 ACLs.

Kamil

bug#33644: [PATCH] cp --preserve=xattr: preserve NFSv4 ACL extended attributes

[Kamil Dudka]

On Monday, February 11, 2019 6:07:18 AM CET Pádraig Brady wrote:
> On 06/12/18 05:08, Kamil Dudka wrote:
> > ... which cannot be preserved by other means
> > 
> > Bug: https://bugzilla.redhat.com/1031423#c4
> > ---
> > 
> >  src/copy.c | 22 +-
> >  1 file changed, 17 insertions(+), 5 deletions(-)
> > 
> > diff --git a/src/copy.c b/src/copy.c
> > index 3221b9997..754c5e1aa 100644
> > --- a/src/copy.c
> > +++ b/src/copy.c
> > @@ -640,6 +640,17 @@ copy_attr_free (struct error_context *ctx _GL_UNUSED,
> > 
> >  {
> >  }
> > 
> > +/* Include NFSv4 ACL extended attributes, which cannot be preserved by
> > +   other means.  Otherwise honor attributes configured for exclusion
> > +   in /etc/xattr.conf.  Return zero to skip.  */
> > +static int
> > +check_not_nfs4_acl (const char *name, struct error_context *ctx)
> > +{
> > +  return attr_copy_check_permissions(name, ctx)
> > + || !STRNCMP_LIT (name, "system.nfs4_acl")
> > + || !STRNCMP_LIT (name, "system.nfs4acl");
> > +}
> > +
> > 
> >  /* Exclude SELinux extended attributes that are otherwise handled,
> >  
> > and are problematic to copy again.  Also honor attributes
> > configured for exclusion in /etc/xattr.conf.
> > 
> > @@ -649,7 +660,7 @@ static int
> > 
> >  check_selinux_attr (const char *name, struct error_context *ctx)
> >  {
> >  
> >return STRNCMP_LIT (name, "security.selinux")
> > 
> > - && attr_copy_check_permissions (name, ctx);
> > + && check_not_nfs4_acl (name, ctx);
> > 
> >  }
> >  
> >  /* If positive SRC_FD and DST_FD descriptors are passed,
> > 
> > @@ -663,6 +674,9 @@ copy_attr (char const *src_path, int src_fd,
> > 
> >bool all_errors = (!x->data_copy_required ||
> >x->require_preserve_xattr);
> >bool some_errors = (!all_errors && !x->reduce_diagnostics);
> >bool selinux_done = (x->preserve_security_context ||
> >x->set_security_context);> 
> > +  int (*check) (const char *, struct error_context *) = (selinux_done)
> > +? check_selinux_attr
> > +: check_not_nfs4_acl;
> > 
> >struct error_context ctx =
> >{
> >
> >  .error = all_errors ? copy_attr_allerror : copy_attr_error,
> > 
> > @@ -670,12 +684,10 @@ copy_attr (char const *src_path, int src_fd,
> > 
> >  .quote_free = copy_attr_free
> >
> >};
> >if (0 <= src_fd && 0 <= dst_fd)
> > 
> > -ret = attr_copy_fd (src_path, src_fd, dst_path, dst_fd,
> > -selinux_done ? check_selinux_attr : NULL,
> > +ret = attr_copy_fd (src_path, src_fd, dst_path, dst_fd, check,
> > 
> >  (all_errors || some_errors ? &ctx : NULL));
> >
> >else
> > 
> > -ret = attr_copy_file (src_path, dst_path,
> > -  selinux_done ? check_selinux_attr : NULL,
> > +ret = attr_copy_file (src_path, dst_path, check,
> > 
> >(all_errors || some_errors ? &ctx : NULL));
> >
> >return ret == 0;
> 
> This patch is confusing to read, though looks functional.

I can submit deduplication of the `selinux_done ? check_selinux_attr : NULL` 
code as a separate patch if you prefer it.

> It's clearer of you rename check_not_nfs4_acl() to
> check_but_allow_nfs4_acl().

Fine by me.

> So in summary, any xattr in /etc/xattr.conf is _not_ copied.
> You want to essentially ignore the nfs4 entries in that config file.
> So why not just remove the entries from that file?

See how xattr.conf is documented:

# Actions:
#   permissions - copy when trying to preserve permissions.
#   skip - do not copy.

The fact that coreutils handles `persmissions` equally as `skip` is IMO a 
problem of coreutils, not a problem of xattr.conf.

> Is that something that could be done in attr.git?

I think that the information in xattr.conf is correct.  system.nfs4_acl is 
really an attribute one wants to copy when trying to preserve permissions.

> Why would one want to treat nfs4 attrs differently to the posix_acl_access
> attrs?

It was written in the commit message.  One can use `cp --preserve=mode`
to preserve POSIX ACLs whereas the only way to preserve NFSv4 ACLs was
`cp --preserve=xattr`.

> thanks,
> Pádraig.

On Monday, February 11, 2019 6:21:49 AM CET Pádraig Brady wrote:
> BTW is there anything interesting behind this paywall I can't access?
> https://access.redhat.com/solutions/115043

It just says that `cp a b` does not preserve NFSv4 ACLs whereas `cp -a a b`,
`cp --preserve=all a b`, or `cp --preserve=xattr a b` does.  Unfortunately, 
this is currently true only for Red Hat Enterprise Linux.

Kamil

bug#33646: [PATCH] doc: improve wording of the --kibibytes option description

[Kamil Dudka]

On Friday, January 18, 2019 8:26:18 AM CET Assaf Gordon wrote:
> Hello,
> 
> On 2018-12-06 6:32 a.m., Kamil Dudka wrote:
> > Bug: https://bugzilla.redhat.com/1527391
> > ---
> > 
> >   doc/coreutils.texi | 8 +---
> >   1 file changed, 5 insertions(+), 3 deletions(-)
> > 
> > diff --git a/doc/coreutils.texi b/doc/coreutils.texi
> > index f8339d73f..e93fe71a0 100644
> > --- a/doc/coreutils.texi
> > +++ b/doc/coreutils.texi
> > @@ -7975,9 +7975,11 @@ Append @samp{*} for executable regular files,
> > otherwise behave as for> 
> >   @opindex --kibibytes
> >   Set the default block size to its normal value of 1024 bytes,
> >   overriding any contrary specification in environment variables
> > 
> > -(@pxref{Block size}).  This option is in turn overridden by the
> > -@option{--block-size}, @option{-h} or @option{--human-readable}, and
> > -@option{--si} options.
> > +(@pxref{Block size}).  If @option{--block-size}, @option{-h},
> > +@option{--human-readable}, or @option{--si} options are used,
> > +they take precedence over @option{-k} or @option{--kibibytes}
> > +even if @option{-k} or @option{--kibibytes} is placed after
> > +the other options.
> > 
> >   The @option{-k} or @option{--kibibytes} option affects the
> >   per-directory block count written by the @option{-l} and similar
> 
> I'm ok with this improvement - if there are no comments
> I'll push in the next few days.
> 
> -assaf

Sounds good.  Thank you for taking care of it!

Kamil

bug#33646: [PATCH] doc: improve wording of the --kibibytes option description

[Kamil Dudka]

Bug: https://bugzilla.redhat.com/1527391
---
 doc/coreutils.texi | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/doc/coreutils.texi b/doc/coreutils.texi
index f8339d73f..e93fe71a0 100644
--- a/doc/coreutils.texi
+++ b/doc/coreutils.texi
@@ -7975,9 +7975,11 @@ Append @samp{*} for executable regular files, otherwise 
behave as for
 @opindex --kibibytes
 Set the default block size to its normal value of 1024 bytes,
 overriding any contrary specification in environment variables
-(@pxref{Block size}).  This option is in turn overridden by the
-@option{--block-size}, @option{-h} or @option{--human-readable}, and
-@option{--si} options.
+(@pxref{Block size}).  If @option{--block-size}, @option{-h},
+@option{--human-readable}, or @option{--si} options are used,
+they take precedence over @option{-k} or @option{--kibibytes}
+even if @option{-k} or @option{--kibibytes} is placed after
+the other options.
 
 The @option{-k} or @option{--kibibytes} option affects the
 per-directory block count written by the @option{-l} and similar
-- 
2.17.2

bug#33644: [PATCH] cp --preserve=xattr: preserve NFSv4 ACL extended attributes

[Kamil Dudka]

... which cannot be preserved by other means

Bug: https://bugzilla.redhat.com/1031423#c4
---
 src/copy.c | 22 +-
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/src/copy.c b/src/copy.c
index 3221b9997..754c5e1aa 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -640,6 +640,17 @@ copy_attr_free (struct error_context *ctx _GL_UNUSED,
 {
 }
 
+/* Include NFSv4 ACL extended attributes, which cannot be preserved by
+   other means.  Otherwise honor attributes configured for exclusion
+   in /etc/xattr.conf.  Return zero to skip.  */
+static int
+check_not_nfs4_acl (const char *name, struct error_context *ctx)
+{
+  return attr_copy_check_permissions(name, ctx)
+ || !STRNCMP_LIT (name, "system.nfs4_acl")
+ || !STRNCMP_LIT (name, "system.nfs4acl");
+}
+
 /* Exclude SELinux extended attributes that are otherwise handled,
and are problematic to copy again.  Also honor attributes
configured for exclusion in /etc/xattr.conf.
@@ -649,7 +660,7 @@ static int
 check_selinux_attr (const char *name, struct error_context *ctx)
 {
   return STRNCMP_LIT (name, "security.selinux")
- && attr_copy_check_permissions (name, ctx);
+ && check_not_nfs4_acl (name, ctx);
 }
 
 /* If positive SRC_FD and DST_FD descriptors are passed,
@@ -663,6 +674,9 @@ copy_attr (char const *src_path, int src_fd,
   bool all_errors = (!x->data_copy_required || x->require_preserve_xattr);
   bool some_errors = (!all_errors && !x->reduce_diagnostics);
   bool selinux_done = (x->preserve_security_context || 
x->set_security_context);
+  int (*check) (const char *, struct error_context *) = (selinux_done)
+? check_selinux_attr
+: check_not_nfs4_acl;
   struct error_context ctx =
   {
 .error = all_errors ? copy_attr_allerror : copy_attr_error,
@@ -670,12 +684,10 @@ copy_attr (char const *src_path, int src_fd,
 .quote_free = copy_attr_free
   };
   if (0 <= src_fd && 0 <= dst_fd)
-ret = attr_copy_fd (src_path, src_fd, dst_path, dst_fd,
-selinux_done ? check_selinux_attr : NULL,
+ret = attr_copy_fd (src_path, src_fd, dst_path, dst_fd, check,
 (all_errors || some_errors ? &ctx : NULL));
   else
-ret = attr_copy_file (src_path, dst_path,
-  selinux_done ? check_selinux_attr : NULL,
+ret = attr_copy_file (src_path, dst_path, check,
   (all_errors || some_errors ? &ctx : NULL));
 
   return ret == 0;
-- 
2.17.2

bug#33622: coreutils v. 8.30 – Tail prints the first row in 'tail -n '

[Kamil Dudka]

On Wednesday, December 5, 2018 11:51:09 AM CET Ricky Tigg wrote:
> OS: *Fedora*. Component: coreutils.x86_64 8.30-6.fc29 @System
> 
> Tail prints the first row in 'tail -n '
> 
> Command executed:
> $ dnf repoquery --requires bash --recursive --resolve | grep -E
> '.x86_64$|.noarch$' | tail -n 1
> Last metadata expiration check: 0:28:25 ago on Wed Dec  5 11:09:16 2018.
> tzdata-0:2018g-1.fc29.noarch
> 
> Expected result: command to only print N rows, which means without printing
> the first row.

The line "Last metadata expiration check: [...]" is not printed by tail
at all.  It is printed by dnf to standard _error_ output.  You will see
it even if you redirect dnf's standard output to /dev/null:

$ dnf repoquery --requires bash --recursive --resolve >/dev/null
Last metadata expiration check: [...]

You can use `dnf --quiet ...` to suppress the message.

bug#33523: broken URL in install(1) man page

[Kamil Dudka]

On Tuesday, November 27, 2018 11:21:03 PM CET Bernhard Voelker wrote:
> On 11/27/18 5:28 PM, Kamil Dudka wrote:
> > The SEE ALSO section of install(1) man page contains a broken URL:
> > Full documentation at:
> > <https://www.gnu.org/software/coreutils/install>
> > 
> > The URL returns:
> > 404 - Page Not Found
> > 
> > I suspect it is caused by the build-time rename ginstall -> install.  If
> > one substitutes 'install' by 'ginstall' in the above URL, then it
> > redirects to the> 
> > following URL:
> > https://www.gnu.org/software/coreutils/manual/html_node/ginstall-invoc
> > ation.html#ginstall-invocation> 
> > ... which does not work either.  It should be redirected to the following
> > URL> 
> > instead:
> > https://www.gnu.org/software/coreutils/manual/html_node/install-invoca
> > tion.html#install-invocation> 
> > Unfortunately, I do not know where the redirections are maintained.  This
> > was> 
> > originally reported in Red Hat Bugzilla:
> > https://bugzilla.redhat.com/1649774
> > 
> > Kamil
> 
> Nice catch!
> I've fixed this in the .htaccess file in coreutils' web-cvs repo.
> Marking this as done.
> 
> Have a nice day,
> Berny

Perfect.  Thanks for the quick fix!

Kamil

bug#33523: broken URL in install(1) man page

[Kamil Dudka]

The SEE ALSO section of install(1) man page contains a broken URL:

Full documentation at: 

The URL returns:

404 - Page Not Found

I suspect it is caused by the build-time rename ginstall -> install.  If one
substitutes 'install' by 'ginstall' in the above URL, then it redirects to the
following URL:


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

... which does not work either.  It should be redirected to the following URL
instead:


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

Unfortunately, I do not know where the redirections are maintained.  This was
originally reported in Red Hat Bugzilla:

https://bugzilla.redhat.com/1649774

Kamil

bug#33287: [PATCH] sync: add missing brackets in sync_arg()

[Kamil Dudka]

On Wednesday, November 7, 2018 12:33:45 AM CET Bernhard Voelker wrote:
> On 11/6/18 7:35 PM, Paul Eggert wrote:
> > Thanks, I installed that and am closing the bug report.
> 
> That was a real bug, i.e., not only a resource leak, wasn't it?
> 
> If the calling user has -r+w permissions on the file, then sync previously
> exited with 1 without actually syncing:
> 
>   $ install -m 0200 /dev/null /tmp/file
> 
>   $ ls -log /tmp/file
>   --w--- 1 0 Nov  7 00:06 /tmp/file
> 
>   $ strace -v /usr/bin/sync /tmp/file 2>&1 | tail -n6
>   openat(AT_FDCWD, "/tmp/file", O_RDONLY|O_NONBLOCK) = -1 EACCES (Permission
> denied) openat(AT_FDCWD, "/tmp/file", O_WRONLY|O_NONBLOCK) = 3
>   close(1)= 0
>   close(2)= 0
>   exit_group(1)   = ?
>   +++ exited with 1 +++
> 
> With the patch, fsync is called, and sync terminated with success:
> 
>   $ strace -v src/sync /tmp/file 2>&1 | tail
>   openat(AT_FDCWD, "/tmp/file", O_RDONLY|O_NONBLOCK) = -1 EACCES (Permission
> denied) openat(AT_FDCWD, "/tmp/file", O_WRONLY|O_NONBLOCK) = 3
>   fcntl(3, F_GETFL)   = 0x8801 (flags
> O_WRONLY|O_NONBLOCK|O_LARGEFILE) fcntl(3, F_SETFL, O_WRONLY|O_LARGEFILE) =
> 0
>   fsync(3)= 0
>   close(3)= 0
>   close(1)= 0
>   close(2)= 0
>   exit_group(0)   = ?
>   +++ exited with 0 +++
> 
> Should we add a NEWS entry and a test - see attached?

Looks good.  Thanks for the follow-up patch!

Kamil

> Thanks & have a nice day,
> Berny

bug#33287: [PATCH] sync: add missing brackets in sync_arg()

[Kamil Dudka]

Detected by Coverity Analysis:

Error: RESOURCE_LEAK (CWE-772):
coreutils-8.30/src/sync.c:112: open_fn: Returning handle opened by "open".
coreutils-8.30/src/sync.c:112: var_assign: Assigning: "fd" = handle returned 
from "open(file, 2049)".
coreutils-8.30/src/sync.c:115: leaked_handle: Handle variable "fd" going out of 
scope leaks the handle.
113| if (fd < 0)
114|   error (0, rd_errno, _("error opening %s"), quoteaf (file));
115|->   return false;
116|   }
117|
---
 src/sync.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/sync.c b/src/sync.c
index bd3671a19..607fa8f7f 100644
--- a/src/sync.c
+++ b/src/sync.c
@@ -111,8 +111,10 @@ sync_arg (enum sync_mode mode, char const *file)
   if (open_flags != (O_WRONLY | O_NONBLOCK))
 fd = open (file, O_WRONLY | O_NONBLOCK);
   if (fd < 0)
-error (0, rd_errno, _("error opening %s"), quoteaf (file));
-  return false;
+{
+  error (0, rd_errno, _("error opening %s"), quoteaf (file));
+  return false;
+}
 }
 
   /* We used O_NONBLOCK above to not hang with fifos,
-- 
2.17.2

bug#31644: tests/ls/abmon-align does not work today (May 29th)

[Kamil Dudka]

On Tuesday, May 29, 2018 7:16:43 PM CEST Pádraig Brady wrote:
> On 29/05/18 09:16, Kamil Dudka wrote:
> > Re-posting with a fresh subject.  debbugs.gnu.org rejected my original
> > post
> > because the original bug has been closed and has received no comments for
> > more than 28 days:
> > 
> > https://debbugs.gnu.org/cgi/bugreport.cgi?bug=30814
> > 
> > On Wednesday, March 14, 2018 7:40:31 PM CEST Pádraig Brady wrote:
> >> Given the increase in supported size should only impact relatively few
> >> languages it probably makes sense to increase to 12. The attached does
> >> that
> >> and also augments the test to find ambiguous cases.
> > 
> > The ls/abmon-align test does not work today (May 29th) because of the
> > added
> > 
> > check.  The problem is that:
> > touch '-d+N month' for N={01..12}
> > 
> > ... in general does not create 12 files with timestamps in different
> > months.
> > 
> > If you run it today (May 29th), touch '-d+09 month' results in March 1st
> > while touch '-d+10 month' results in March 29th.  Consequently, both
> > 09.ts and 10.ts have the same month (March) to begin with and the check
> > for duplicates has to
> > fail:
> That was a latent issue with the original test, which I've
> now pushed a fix for at:
> https://git.sv.gnu.org/gitweb/?p=coreutils.git;a=commitdiff;h=c8eb21c

This works for me.  Thanks!

Kamil

> I've a local prototype to address the more general gotcha here
> wrt relative date adjustments, which I hope to push soon.
> 
> thanks!
> Pádraig

bug#31644: tests/ls/abmon-align does not work today (May 29th)

[Kamil Dudka]

Re-posting with a fresh subject.  debbugs.gnu.org rejected my original post 
because the original bug has been closed and has received no comments for more 
than 28 days:

https://debbugs.gnu.org/cgi/bugreport.cgi?bug=30814


On Wednesday, March 14, 2018 7:40:31 PM CEST Pádraig Brady wrote:
> Given the increase in supported size should only impact relatively few
> languages it probably makes sense to increase to 12. The attached does that
> and also augments the test to find ambiguous cases.

The ls/abmon-align test does not work today (May 29th) because of the added 
check.  The problem is that:

touch '-d+N month' for N={01..12}

... in general does not create 12 files with timestamps in different months. 

If you run it today (May 29th), touch '-d+09 month' results in March 1st while 
touch '-d+10 month' results in March 29th.  Consequently, both 09.ts and 10.ts 
have the same month (March) to begin with and the check for duplicates has to 
fail:

++ echo 'Jun
Jul
Aug
Sep
Oct
Nov
Dec
Jan
Mar
Mar
Apr
May'
++ sort
++ uniq -d
++ wc -l
+ n_dupes=1
+ test 1 = 1
+ test 1 = 0
+ fail=1
+ break 2
+ test 1 = 1
+ echo 'misalignment or ambiguous output in C locale:'
misalignment or ambiguous output in C locale:
+ LC_ALL=C
+ TIME_STYLE=+%b
+ ls -lgG 01.ts 02.ts 03.ts 04.ts 05.ts 06.ts 07.ts 08.ts 09.ts 10.ts 11.ts 
12.ts
-rw-rw-r-- 1 0 Jun 01.ts
-rw-rw-r-- 1 0 Jul 02.ts
-rw-rw-r-- 1 0 Aug 03.ts
-rw-rw-r-- 1 0 Sep 04.ts
-rw-rw-r-- 1 0 Oct 05.ts
-rw-rw-r-- 1 0 Nov 06.ts
-rw-rw-r-- 1 0 Dec 07.ts
-rw-rw-r-- 1 0 Jan 08.ts
-rw-rw-r-- 1 0 Mar 09.ts
-rw-rw-r-- 1 0 Mar 10.ts
-rw-rw-r-- 1 0 Apr 11.ts
-rw-rw-r-- 1 0 May 12.ts

bug#31439: [PATCH] fts: avoid a memory leak edge case

[Kamil Dudka]

On Monday, May 14, 2018 3:51:02 AM CEST Pádraig Brady wrote:
> @@ -122,9 +139,10 @@ main (void)
>  perror_exit (base, 6);
>while ((e = fts_read (ftsp)))
>  needles_seen += strcmp (e->fts_name, "needle") == 0;
> -  fflush (stdout);
>if (errno)
>  perror_exit ("fts_read", 7);
> +  if (fts_close (ftsp) != 0)
> +perror_exit (base, 8);
>  
>/* Report an error if we did not find the needles.  */
>if (needles_seen != needles)

Why are you removing fflush (stdout) from the test without any explanation?

Kamil

bug#30907: mv return value.

[Kamil Dudka]

On Friday, March 23, 2018 2:00:01 PM CET Eric Blake wrote:
> I don't know if mv exposes RENAME_NOREPLACE semantics yet, but it should
> be taught to do so, where such semantics are available.

mv was recently patched to use renameat2(..., RENAME_NOREPLACE):

http://git.savannah.gnu.org/cgit/coreutils.git/commit/?id=v8.29-9-g29baf25aa

Kamil

bug#29961: [PATCH] mv -n: do not overwrite the destination

[Kamil Dudka]

On Wednesday, January 10, 2018 9:53:07 AM CET Paul Eggert wrote:
> No further comment, so I installed the proposed patches and I'm closing this
> bug report.

Sorry.  There were just too many changes in your patches for me to review
them quickly enough (and most of the changes were unrelated to my initial 
submission anyway).

Thank you for fixing it!

Kamil

bug#29961: [PATCH] mv -n: do not overwrite the destination

[Kamil Dudka]

On Friday, January 5, 2018 4:29:55 PM CET Pádraig Brady wrote:
> Thanks to both of you.
> The approaches can be summarized as:
> 
> Orig:
> -
> stat() => ENOENT
> 
> rename()  may clobber file
> 
> Kamil's:
> -
> stat() => ENOENT
> 
> renameat()  doesn't clobber file if -n
> exit early if -n && errno==EEXIST
> 
> Paul's:
> -
> renameat2() => EEXIST
>  -n => exit early
> if (renameat2_failed)
>  unless EEXIST && -n
>   stat()
> if (renameat2_failed)
>  rename()
> 
> I think both patches ensure rename() doesn't clobber when -n is specified.

Thanks for the summary!

> Paul's is more encompassing for the non -n case.
> For example if a directory was _created_ externally
> after the stat() in Kamil's logic above,
> it could be erroneously clobbered.

Do you mean without -n?

I am getting the following error message in that case:

mv: cannot move 'a' to 'b': Is a directory

... which is consistent with the original behavior.

> Paul's also avoids a stat() in the common case
> where the initial renameat2() succeeds.

At the cost of _not_ avoiding the renameat2() call in the most common case.
I think both the solutions are equivalent performance-wise.

> Both versions are still susceptible to erroneous clobbering
> if the destination file was externally _replaced_
> by a directory for example, after the stat().
> Though that is less likely.
> 
> Paul's fix looks good to apply here,
> since it's more encompassing.

No problem with that.  Anyway, I will go with my conservative (or even the 
documentation-only) patch for RHEL-7, which was the trigger of this effort, 
because Paul's patch comes with changes of behavior observable beyond the 
reported scenario.

Kamil

> cheers,
> Pádraig

bug#29961: [PATCH] mv -n: do not overwrite the destination

[Kamil Dudka]

On Friday, January 5, 2018 2:00:52 AM CET Paul Eggert wrote:
> On 01/04/2018 03:01 AM, Kamil Dudka wrote:
> > On Thursday, January 4, 2018 10:48:56 AM CET Paul Eggert wrote:
> >> Kamil Dudka wrote:
> >>> -  if (rename (src_name, dst_name) == 0)
> >>> +  int flags = 0;
> >>> +  if (x->interactive == I_ALWAYS_NO)
> >>> +/* do not replace DST_NAME if it was created since our last
> >>> check
> >>> */ +flags = RENAME_NOREPLACE;
> >> 
> >> By then it's too late, as multiple decisions have been made on the basis
> >> of
> >> stat/lstat calls that are still subject to races.
> > 
> > Do you mean in the case of mv -n?  Which decisions exactly?
> 
> Mostly mv -n, but I suspect problems also for mv without -n.

My patch changes nothing but the 'mv -n' behavior.  It could hardly break
(or even change behavior of) anything else.  Your patch reworks the logic
of copy_internal(), which itself is very fragile, as you learned from the 
first version of your patch.

> It's all
> the decisions that depend on the result of lstat of dst_name, before
> abandon_move decides whether to skip the rename.

I am only fixing the case where the destination file is created after the 
lstat() call.  In that case, the only result is ENOENT, which is harmless.

> With the patch you
> proposed, mv -n could call lstat and get a failure (with errno ==
> ENOENT), then (after another process creates the file) call renameat2
> with RENAME_NOREPLACE and after this fails (with errno == EEXIST)

Exactly.  That is the only case that my patch intended to fix.

> report an error.

Nope.  It will silently succeed with my patch.  Did you try it?

> mv -n should silently succeed in that case.

I agree.

> > Sounds like a corner case. Please consider writing a separate patch
> > for that.
> 
> OK, that's pretty straightforward so I installed it. Please see the
> first attached patch.
> 
> > I had difficulties trying to evaluate the patch. It does not compile
> 
> That's what I get for sending an untested patch. Sorry. I fixed the bugs
> you mentioned and tested the result. Please see the second attached
> patch, which I have not installed.

Thanks for the fixup!

> There is an interesting behavior change with this second patch.
> Currently, 'mv -n a a' fails with a diagnostic "mv: 'a' and 'a' are the
> same file". With the patch, 'mv -n a a' silently succeeds. The coreutils
> documentation allows both behaviors. I doubt whether anyone cares, and
> doing it the new way avoids some syscalls so I left it that way.

If you decide to apply your patch anyway, please document this change
at least in the commit message.  Still my concern is that your patch also 
changes the behavior of 'mv' without '-n', which is neither tested, nor 
documented.

Kamil

bug#29961: [PATCH] mv -n: do not overwrite the destination

[Kamil Dudka]

On Thursday, January 4, 2018 10:48:56 AM CET Paul Eggert wrote:
> Kamil Dudka wrote:
> > -  if (rename (src_name, dst_name) == 0)
> > +  int flags = 0;
> > +  if (x->interactive == I_ALWAYS_NO)
> > +/* do not replace DST_NAME if it was created since our last check
> > */ +flags = RENAME_NOREPLACE;
> 
> By then it's too late, as multiple decisions have been made on the basis of
> stat/lstat calls that are still subject to races.

Do you mean in the case of mv -n?  Which decisions exactly?

> It's better to try the
> renameat2 with RENAME_NOREPLACE first thing, and fall back on the existing
> code only if renameat2 fails with errno != EEXIST.
> 
> Plus, there are some other problems when combining -u and -n.

Sounds like a corner case.  Please consider writing a separate patch for that.

> How about the attached patch instead?

I had difficulties trying to evaluate the patch.  It does not compile
with gcc-7.2.1:

src/copy.c: In function 'copy_internal':
src/copy.c:2340:17: error: 'earlier_file' may be used uninitialized in this 
function [-Werror=maybe-uninitialized]
   if (! create_hard_link (earlier_file, dst_name, true, x->verbose,
 ^~~
   dereference))
   
src/copy.c:2050:14: error: 'return_now' may be used uninitialized in this 
function [-Werror=maybe-uninitialized]
   if (return_now)
  ^

So I added initializers to those two variables but then I saw multiple
test-cases failing because of the patch:

FAIL: tests/mv/hard-link-1
FAIL: tests/mv/mv-special-1
FAIL: tests/mv/part-fail
FAIL: tests/mv/part-hardlink
FAIL: tests/mv/part-rename
FAIL: tests/mv/part-symlink

Kamil

bug#29961: [PATCH] mv -n: do not overwrite the destination

[Kamil Dudka]

... if it is created by another process after mv has checked its
non-existence.

* src/copy.c (copy_internal): Use renameat2 (..., RENAME_NOREPLACE)
if called by mv -n.  If it fails with EEXIST in that case, pretend
successful rename as if the existing destination file was detected
by the preceding lstat call.
* NEWS: Mention the fix.
Fixes https://bugs.gnu.org/29961
---
 NEWS   |  3 +++
 src/copy.c | 17 -
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/NEWS b/NEWS
index b89254f7e..e463ce6db 100644
--- a/NEWS
+++ b/NEWS
@@ -18,6 +18,9 @@ GNU coreutils NEWS-*- 
outline -*-
   df no longer hangs when given a fifo argument.
   [bug introduced in coreutils-7.3]
 
+  mv -n no longer overwrites the destination if it is created by another
+  process after mv has checked its non-existence.
+
   ptx -S no longer infloops for a pattern which returns zero-length matches.
   [the bug dates back to the initial implementation]
 
diff --git a/src/copy.c b/src/copy.c
index 2a804945e..be4e357a8 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -53,6 +53,7 @@
 #include "ignore-value.h"
 #include "ioblksize.h"
 #include "quote.h"
+#include "renameat2.h"
 #include "root-uid.h"
 #include "same.h"
 #include "savedir.h"
@@ -2319,7 +2320,12 @@ copy_internal (char const *src_name, char const 
*dst_name,
 
   if (x->move_mode)
 {
-  if (rename (src_name, dst_name) == 0)
+  int flags = 0;
+  if (x->interactive == I_ALWAYS_NO)
+/* do not replace DST_NAME if it was created since our last check */
+flags = RENAME_NOREPLACE;
+
+  if (renameat2 (AT_FDCWD, src_name, AT_FDCWD, dst_name, flags) == 0)
 {
   if (x->verbose)
 {
@@ -2351,6 +2357,15 @@ copy_internal (char const *src_name, char const 
*dst_name,
   return true;
 }
 
+  if ((flags & RENAME_NOREPLACE) && (errno == EEXIST))
+{
+  /* Pretend the rename succeeded, so the caller (mv)
+ doesn't end up removing the source file.  */
+  if (rename_succeeded)
+*rename_succeeded = true;
+  return true;
+}
+
   /* FIXME: someday, consider what to do when moving a directory into
  itself but when source and destination are on different devices.  */
 
-- 
2.13.6

bug#29961: [PATCH] mv: document the missing atomicity of 'mv -n'

[Kamil Dudka]

On Wednesday, January 3, 2018 4:08:51 PM CET Pádraig Brady wrote:
> Eep, Seems like we should use RENAME_NOREPLACE in this case,
> rather than document the caveat?

Thanks for the suggestion!  I will give it a try...

Kamil

> This is already used in shred.
> 
> cheers,
> Pádraig

bug#29961: [PATCH] mv: document the missing atomicity of 'mv -n'

[Kamil Dudka]

* src/mv.c (usage): Add note about the missing atomicity of 'mv -n'.
* doc/coreutils.texi (mv invocation): Likewise.
---
 doc/coreutils.texi | 3 +++
 src/mv.c   | 2 ++
 2 files changed, 5 insertions(+)

diff --git a/doc/coreutils.texi b/doc/coreutils.texi
index 3fa083085..e7ca6a737 100644
--- a/doc/coreutils.texi
+++ b/doc/coreutils.texi
@@ -9660,6 +9660,9 @@ If the response is not affirmative, the file is skipped.
 Do not overwrite an existing file.
 @mvOptsIfn
 This option is mutually exclusive with @option{-b} or @option{--backup} option.
+Note that the operation is not atomic.  If @var{dest} appears after
+@command{mv} has checked its non-existence, it can be overwritten despite
+the @option{-n} option is used.
 
 @item -u
 @itemx --update
diff --git a/src/mv.c b/src/mv.c
index a8df730a7..a08e75649 100644
--- a/src/mv.c
+++ b/src/mv.c
@@ -306,6 +306,8 @@ Rename SOURCE to DEST, or move SOURCE(s) to DIRECTORY.\n\
   -i, --interactiveprompt before overwrite\n\
   -n, --no-clobber do not overwrite an existing file\n\
 If you specify more than one of -i, -f, -n, only the final one takes effect.\n\
+NOTE: The operation is not atomic.  If DEST appears after mv has checked its\n\
+non-existence, it can be overwritten despite the -n option is used.\n\
 "), stdout);
   fputs (_("\
   --strip-trailing-slashes  remove any trailing slashes from each SOURCE\n\
-- 
2.13.6

bug#29563: [PATCH] doc: fix default QUOTING_STYLE for %N format of stat(1)

[Kamil Dudka]

* doc/coreutils.texi (stat invocation): The default value
of QUOTING_STYLE for the %N format of 'stat --printf' is
'shell-escape-always'.

Reported by Christian Groessler at
https://bugzilla.redhat.com/1520399#c3
---
 doc/coreutils.texi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/doc/coreutils.texi b/doc/coreutils.texi
index 98ea4e228..09730f6d3 100644
--- a/doc/coreutils.texi
+++ b/doc/coreutils.texi
@@ -12246,7 +12246,7 @@ numbers unambiguously octal, you can use @samp{%#03a}.
 
 The @samp{%N} format can be set with the environment variable
 @env{QUOTING_STYLE}@.  If that environment variable is not set,
-the default value is @samp{shell-escape}.  Valid quoting styles are:
+the default value is @samp{shell-escape-always}.  Valid quoting styles are:
 @quotingStyles
 
 The @samp{%t} and @samp{%T} formats operate on the st_rdev member of
-- 
2.13.6

bug#24541: runcon tty hijacking via TIOCSTI ioctl

[Kamil Dudka]

On Monday, August 28, 2017 11:51:12 AM CEST Pádraig Brady wrote:
> On 29/09/16 08:15, Bernhard Voelker wrote:
> > On 09/26/2016 05:53 PM, Paul Eggert wrote:
> >>> "I don't think we need to fix this for runcon, as it isn't as
> >>> sandboxing tool like sandbox, and the loss of job control would likely
> >>> be much more noticeable for runcon."
> >> 
> >> Thanks, closing the debbugs bug report.
> > 
> > FWIW Karel just committed a workaround for su/runuser in util-linux
> > using libseccomp:
> > 
> > https://github.com/karelzak/util-linux/commit/8e492501

Note that the above mentioned commit was reverted long time ago:

https://github.com/karelzak/util-linux/commit/23f75093

Kamil

> I think this issue is worth addressing with libseccomp.
> That lib is a widely used dependency on SELinux systems
> so not a significant dependency to add.
> The attached uses libseccomp if available,
> and falls back to using setsid() in the edge cases where not.
> 
> cheers,
> Pádraig

bug#28120: [PATCH] ptx: fix a possible crash caused by integer overflow

[Kamil Dudka]

On Thursday, August 17, 2017 12:14:05 Paul Eggert wrote:
> On 08/17/2017 04:40 AM, Kamil Dudka wrote:
> > -typedef short int DELTA;   /* to hold displacement within one context */
> > +typedef int DELTA; /* to hold displacement within one context */
> 
> Thanks for the heads-up. Although that fixes things for that particular
> test case, it won't work for larger cases.

Do you have an example of the larger case?  We could add a test-case for it.

> The type should be ptrdiff_t instead of int.
> 
> As its FIXME comment says, ptx is riddled with integer-overflow bugs. I
> installed the attached patch to fix the bug that you mentioned along
> with the other low-hanging fruit that I found, and am marking the bug as
> fixed upstream. I expect some other integer-overflow bugs can still
> occur in practice, but at least this patch is a significant improvement.
> 
> This patch prefers signed integer types like ptrdiff_t to unsigned types
> like size_t, as signed types allow for better checking when compiled
> with sanitization.

Your patch introduces the following warnings:

Error: CONSTANT_EXPRESSION_RESULT:
src/ptx.c:1939: result_independent_of_operands: "tmp <= 9223372036854775807L" 
is always true regardless of the values of its operands. This occurs as the 
logical second operand of "&&".
# 1937|   intmax_t tmp;
# 1938|   if (! (xstrtoimax (optarg, NULL, 0, &tmp, NULL) == 
LONGINT_OK
# 1939|->&& 0 < tmp && tmp <= PTRDIFF_MAX))
# 1940| die (EXIT_FAILURE, 0, _("invalid gap width: %s"),
# 1941|  quote (optarg));

Error: CONSTANT_EXPRESSION_RESULT:
src/ptx.c:1966: result_independent_of_operands: "tmp <= 9223372036854775807L" 
is always true regardless of the values of its operands. This occurs as the 
logical second operand of "&&".
# 1964|   intmax_t tmp;
# 1965|   if (! (xstrtoimax (optarg, NULL, 0, &tmp, NULL) == 
LONGINT_OK
# 1966|->&& 0 < tmp && tmp <= PTRDIFF_MAX))
# 1967| die (EXIT_FAILURE, 0, _("invalid line width: %s"),
# 1968|  quote (optarg));

Anyway, it fixes the original bug so I am fine with the patch as it is.

Thank you for pushing the fix!

Kamil

bug#28120: [PATCH] ptx: fix a possible crash caused by integer overflow

[Kamil Dudka]

The crash might not be reproducible in all environments but I was able
to observe invalid reads by valgrind while running ptx on the reproducer
for :

% curl -s 'https://bugzilla.redhat.com/attachment.cgi?id=1314625' | gzip -cd | 
valgrind ptx > /dev/null
==4288== Memcheck, a memory error detector
==4288== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==4288== Using Valgrind-3.12.0 and LibVEX; rerun with -h for copyright info
==4288== Command: ptx
==4288==
==4288== Invalid read of size 1
==4288==at 0x402FA9: define_all_fields (ptx.c:1434)
==4288==by 0x402FA9: generate_all_output (ptx.c:1780)
==4288==by 0x402FA9: main (ptx.c:2155)
==4288==  Address 0x5328a6f is 2,287 bytes inside a block of size 16,352 free'd
==4288==at 0x4C2FC47: realloc (vg_replace_malloc.c:785)
==4288==by 0x408B65: xrealloc (xmalloc.c:61)
==4288==by 0x402857: find_occurs_in_text (ptx.c:955)
==4288==by 0x402857: main (ptx.c:2141)
==4288==  Block was alloc'd at
==4288==at 0x4C2FC47: realloc (vg_replace_malloc.c:785)
==4288==by 0x408B65: xrealloc (xmalloc.c:61)
==4288==by 0x402857: find_occurs_in_text (ptx.c:955)
==4288==by 0x402857: main (ptx.c:2141)
==4288==
==4288== Invalid read of size 1
==4288==at 0x4031AB: define_all_fields (ptx.c:1501)
==4288==by 0x4031AB: generate_all_output (ptx.c:1780)
==4288==by 0x4031AB: main (ptx.c:2155)
==4288==  Address 0x5328a6f is 2,287 bytes inside a block of size 16,352 free'd
==4288==at 0x4C2FC47: realloc (vg_replace_malloc.c:785)
==4288==by 0x408B65: xrealloc (xmalloc.c:61)
==4288==by 0x402857: find_occurs_in_text (ptx.c:955)
==4288==by 0x402857: main (ptx.c:2141)
==4288==  Block was alloc'd at
==4288==at 0x4C2FC47: realloc (vg_replace_malloc.c:785)
==4288==by 0x408B65: xrealloc (xmalloc.c:61)
==4288==by 0x402857: find_occurs_in_text (ptx.c:955)
==4288==by 0x402857: main (ptx.c:2141)
==4288==
==4288==
==4288== HEAP SUMMARY:
==4288== in use at exit: 2,340,851 bytes in 96 blocks
==4288==   total heap usage: 200 allocs, 104 frees, 6,478,142 bytes allocated
==4288==
==4288== LEAK SUMMARY:
==4288==definitely lost: 0 bytes in 0 blocks
==4288==indirectly lost: 0 bytes in 0 blocks
==4288==  possibly lost: 0 bytes in 0 blocks
==4288==still reachable: 2,340,851 bytes in 96 blocks
==4288== suppressed: 0 bytes in 0 blocks
==4288== Rerun with --leak-check=full to see details of leaked memory
==4288==
==4288== For counts of detected and suppressed errors, rerun with: -v
==4288== ERROR SUMMARY: 915 errors from 2 contexts (suppressed: 0 from 0)
---
 src/ptx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/ptx.c b/src/ptx.c
index c0c9733..f4ed7d4 100644
--- a/src/ptx.c
+++ b/src/ptx.c
@@ -224,7 +224,7 @@ static BLOCK *text_buffers; /* files to study */
start of the reference field, it is of type (DELTA) and usually
negative.  */
 
-typedef short int DELTA;   /* to hold displacement within one context */
+typedef int DELTA; /* to hold displacement within one context */
 
 typedef struct
   {
-- 
2.9.5

bug#25052: [PATCH] install, mkdir: fix handling of -DZ and -pZ, respectively

[Kamil Dudka]

On Monday, November 28, 2016 16:11:34 Pádraig Brady wrote:
> On 28/11/16 15:21, Kamil Dudka wrote:
> > ... in case two or more directories nested in each other are created and
> > each of them defaults to a different SELinux context.
> > 
> > * src/install.c (make_ancestor): When calling defaultcon(), give it the
> > same path that is given to mkdir().  The other path is not always valid
> > wrt. current working directory.
> > * src/mkdir.c (make_ancestor): Likewise.
> > * NEWS: Mention the bug fix.
> > 
> > Reported at https://bugzilla.redhat.com/1398913
> > ---
> > 
> >  NEWS  | 4 
> >  src/install.c | 2 +-
> >  src/mkdir.c   | 2 +-
> >  3 files changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/NEWS b/NEWS
> > index 6f7505f..e88e932 100644
> > --- a/NEWS
> > +++ b/NEWS
> > @@ -25,6 +25,10 @@ GNU coreutils NEWS   
> > -*- outline -*-> 
> >factor again outputs immediately when numbers are input interactively.
> >[bug introduced in coreutils-8.24]
> > 
> > +  install -DZ and mkdir -pZ now set default SELinux context correctly
> > even if +  two or more directories nested in each other are created and
> > each of them +  defaults to a different SELinux context.
> > +
> > 
> >ls --time-style no longer mishandles '%%b' in formats.
> >[bug introduced in coreutils-7.2]
> > 
> > diff --git a/src/install.c b/src/install.c
> > index 414d645..d79d597 100644
> > --- a/src/install.c
> > +++ b/src/install.c
> > @@ -427,7 +427,7 @@ static int
> > 
> >  make_ancestor (char const *dir, char const *component, void *options)
> >  {
> >  
> >struct cp_options const *x = options;
> > 
> > -  if (x->set_security_context && defaultcon (dir, S_IFDIR) < 0
> > +  if (x->set_security_context && defaultcon (component, S_IFDIR) < 0
> > 
> >&& ! ignorable_ctx_err (errno))
> >  
> >  error (0, errno, _("failed to set default creation context for %s"),
> >  
> > quoteaf (dir));
> > 
> > diff --git a/src/mkdir.c b/src/mkdir.c
> > index ccd923b..6b51292 100644
> > --- a/src/mkdir.c
> > +++ b/src/mkdir.c
> > @@ -123,7 +123,7 @@ make_ancestor (char const *dir, char const *component,
> > void *options)> 
> >  {
> >  
> >struct mkdir_options const *o = options;
> > 
> > -  if (o->set_security_context && defaultcon (dir, S_IFDIR) < 0
> > +  if (o->set_security_context && defaultcon (component, S_IFDIR) < 0
> > 
> >&& ! ignorable_ctx_err (errno))
> >  
> >  error (0, errno, _("failed to set default creation context for %s"),
> >  
> > quoteaf (dir));
> 
> Looks good.
> Good timing re release 8.26 which we're about to cut.

Thanks for merging it upstream!  Looking forward for the new release...

Kamil

> thanks!
> Pádraig

bug#25052: [PATCH] install, mkdir: fix handling of -DZ and -pZ, respectively

[Kamil Dudka]

... in case two or more directories nested in each other are created and
each of them defaults to a different SELinux context.

* src/install.c (make_ancestor): When calling defaultcon(), give it the
same path that is given to mkdir().  The other path is not always valid
wrt. current working directory.
* src/mkdir.c (make_ancestor): Likewise.
* NEWS: Mention the bug fix.

Reported at https://bugzilla.redhat.com/1398913
---
 NEWS  | 4 
 src/install.c | 2 +-
 src/mkdir.c   | 2 +-
 3 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/NEWS b/NEWS
index 6f7505f..e88e932 100644
--- a/NEWS
+++ b/NEWS
@@ -25,6 +25,10 @@ GNU coreutils NEWS-*- 
outline -*-
   factor again outputs immediately when numbers are input interactively.
   [bug introduced in coreutils-8.24]
 
+  install -DZ and mkdir -pZ now set default SELinux context correctly even if
+  two or more directories nested in each other are created and each of them
+  defaults to a different SELinux context.
+
   ls --time-style no longer mishandles '%%b' in formats.
   [bug introduced in coreutils-7.2]
 
diff --git a/src/install.c b/src/install.c
index 414d645..d79d597 100644
--- a/src/install.c
+++ b/src/install.c
@@ -427,7 +427,7 @@ static int
 make_ancestor (char const *dir, char const *component, void *options)
 {
   struct cp_options const *x = options;
-  if (x->set_security_context && defaultcon (dir, S_IFDIR) < 0
+  if (x->set_security_context && defaultcon (component, S_IFDIR) < 0
   && ! ignorable_ctx_err (errno))
 error (0, errno, _("failed to set default creation context for %s"),
quoteaf (dir));
diff --git a/src/mkdir.c b/src/mkdir.c
index ccd923b..6b51292 100644
--- a/src/mkdir.c
+++ b/src/mkdir.c
@@ -123,7 +123,7 @@ make_ancestor (char const *dir, char const *component, void 
*options)
 {
   struct mkdir_options const *o = options;
 
-  if (o->set_security_context && defaultcon (dir, S_IFDIR) < 0
+  if (o->set_security_context && defaultcon (component, S_IFDIR) < 0
   && ! ignorable_ctx_err (errno))
 error (0, errno, _("failed to set default creation context for %s"),
quoteaf (dir));
-- 
2.7.4

bug#24232: [PATCH] ls: postpone installation of signal handlers

[Kamil Dudka]

On Tuesday, September 06, 2016 19:07:13 Pádraig Brady wrote:
> On 06/09/16 18:18, Jim Meyering wrote:
> > On Tue, Sep 6, 2016 at 10:05 AM, Paul Eggert  wrote:
> >> On 09/06/2016 08:59 AM, Pádraig Brady wrote:
> >>> Will push later.
> >> 
> >> Before pushing, can you please change the name of "sigs" back to "sig"? I
> >> prefer the old name, as "sig[i]" clearly means "signal i", whereas
> >> "sigs[i]" is more confusing. (This is one of my pet peeves about array
> >> names.) Thanks.> 
> > I agree.
> > A good argument for "why?" is that we optimize for readability of the
> > more frequent use with an index, sig[i], rather than for the sole
> > declaration, or the less frequent uses of the array name without an
> > index.
> 
> Ok done.
> 
> I consolidated signal handler setup and restoration to a single function
> to allow that.

Thanks for review!  I am fine with all the proposals.  Please let me know
in case any resubmission is needed.

Kamil

bug#24232: [PATCH] ls: postpone installation of signal handlers

[Kamil Dudka]

... until they are actually needed.  That is right before the first
escape sequence is printed to a terminal.

* src/ls.c (sigs): Moved from main() to global scope to make it
available in put_indicator(), too.  Renamed to prevent identifier
clashes with parameters of subsequent function definitions.
(nsigs): Likewise.
(caught_sig): Likewise.
(main): Code installing signal handlers moved to put_indicator() in
order not to keep default signal handling untouched as long as possible.
Adjusted condition for restoring signal handlers to reflect the change.
(put_indicator): Install signal handlers if called for the very first
time.  It uses the same code that was in main() prior to this commit.
* NEWS: Mention the change.

See https://bugzilla.redhat.com/1365933
---
 NEWS |   3 ++
 src/ls.c | 167 +++
 2 files changed, 86 insertions(+), 84 deletions(-)

diff --git a/NEWS b/NEWS
index 736b95e..d4af96f 100644
--- a/NEWS
+++ b/NEWS
@@ -18,6 +18,9 @@ GNU coreutils NEWS-*- 
outline -*-
   System V style platforms where this information is available only
   in the global variable 'tzname'. [bug introduced in coreutils-8.24]
 
+  ls is now fully responsive to signals until the first escape sequence is
+  written to a terminal.
+
   nl now resets numbering for each page section rather than just for each page.
   [This bug was present in "the beginning".]
 
diff --git a/src/ls.c b/src/ls.c
index 3572060..c438f61 100644
--- a/src/ls.c
+++ b/src/ls.c
@@ -753,6 +753,36 @@ static char const *long_time_format[2] =
 N_("%b %e %H:%M")
   };
 
+/* The signals that are trapped, and the number of such signals.  */
+static int const sigs[] =
+{
+  /* This one is handled specially.  */
+  SIGTSTP,
+
+  /* The usual suspects.  */
+  SIGALRM, SIGHUP, SIGINT, SIGPIPE, SIGQUIT, SIGTERM,
+#ifdef SIGPOLL
+  SIGPOLL,
+#endif
+#ifdef SIGPROF
+  SIGPROF,
+#endif
+#ifdef SIGVTALRM
+  SIGVTALRM,
+#endif
+#ifdef SIGXCPU
+  SIGXCPU,
+#endif
+#ifdef SIGXFSZ
+  SIGXFSZ,
+#endif
+};
+enum { nsigs = ARRAY_CARDINALITY (sigs) };
+
+#if ! SA_NOCLDSTOP
+static bool caught_sig[nsigs];
+#endif
+
 /* The set of signals that are caught.  */
 
 static sigset_t caught_signals;
@@ -1251,36 +1281,6 @@ main (int argc, char **argv)
   struct pending *thispend;
   int n_files;
 
-  /* The signals that are trapped, and the number of such signals.  */
-  static int const sig[] =
-{
-  /* This one is handled specially.  */
-  SIGTSTP,
-
-  /* The usual suspects.  */
-  SIGALRM, SIGHUP, SIGINT, SIGPIPE, SIGQUIT, SIGTERM,
-#ifdef SIGPOLL
-  SIGPOLL,
-#endif
-#ifdef SIGPROF
-  SIGPROF,
-#endif
-#ifdef SIGVTALRM
-  SIGVTALRM,
-#endif
-#ifdef SIGXCPU
-  SIGXCPU,
-#endif
-#ifdef SIGXFSZ
-  SIGXFSZ,
-#endif
-};
-  enum { nsigs = ARRAY_CARDINALITY (sig) };
-
-#if ! SA_NOCLDSTOP
-  bool caught_sig[nsigs];
-#endif
-
   initialize_main (&argc, &argv);
   set_program_name (argv[0]);
   setlocale (LC_ALL, "");
@@ -1314,46 +1314,6 @@ main (int argc, char **argv)
   || (is_colored (C_EXEC) && color_symlink_as_referent)
   || (is_colored (C_MISSING) && format == long_format))
 check_symlink_color = true;
-
-  /* If the standard output is a controlling terminal, watch out
- for signals, so that the colors can be restored to the
- default state if "ls" is suspended or interrupted.  */
-
-  if (0 <= tcgetpgrp (STDOUT_FILENO))
-{
-  int j;
-#if SA_NOCLDSTOP
-  struct sigaction act;
-
-  sigemptyset (&caught_signals);
-  for (j = 0; j < nsigs; j++)
-{
-  sigaction (sig[j], NULL, &act);
-  if (act.sa_handler != SIG_IGN)
-sigaddset (&caught_signals, sig[j]);
-}
-
-  act.sa_mask = caught_signals;
-  act.sa_flags = SA_RESTART;
-
-  for (j = 0; j < nsigs; j++)
-if (sigismember (&caught_signals, sig[j]))
-  {
-act.sa_handler = sig[j] == SIGTSTP ? stophandler : sighandler;
-sigaction (sig[j], &act, NULL);
-  }
-#else
-  for (j = 0; j < nsigs; j++)
-{
-  caught_sig[j] = (signal (sig[j], SIG_IGN) != SIG_IGN);
-  if (caught_sig[j])
-{
-  signal (sig[j], sig[j] == SIGTSTP ? stophandler : 
sighandler);
-  siginterrupt (sig[j], 0);
-}
-}
-#endif
-}
 }
 
   if (dereference == DEREF_UNDEFINED)
@@ -1466,31 +1426,29 @@ main (int argc, char **argv)
   print_dir_name = true;
 }
 
-  if (print_with_color)
+  if (print_with_color && used_color)
 {
   int j;
 
-  if (used_color)
-{
-  /* Skip the restore when it would be a no-op, i.e.,
- when left is "\033[" and right is "m".  */
-  if (!(color_indicator[C_LEFT].len == 2
-&& m

bug#24232: ls --color cannot be interrupted by a signal

[Kamil Dudka]

On Monday, August 15, 2016 15:55:13 Pádraig Brady wrote:
> The signal catching functionality originated trying to restore terminal
> color:
> http://git.sv.gnu.org/gitweb/?p=coreutils.git;a=commitdiff;h=v4.5.3-89-g854
> 9068
> 
> That was adjusted to only outputting reset chars once for multiple signals,
> http://git.sv.gnu.org/gitweb/?p=coreutils.git;a=commitdiff;h=v5.2.1-357-geae
> 1b7f
> 
> and not outputting reset chars at arbitrary places as that messes up
> multi-byte chars:
> http://git.sv.gnu.org/gitweb/?p=coreutils.git;a=commitdiff;h=v5.2.1-368-gad
> c30a8
> 
> Maybe we should just buffer internally at put_indicator
> (&color_indicator[C_LEFT]); and output only at put_indicator
> (&color_indicator[C_RIGHT]) or equivalent. That wouldn't introduce
> significant overhead I think.

Internal buffering is certainly doable.  I guess there is some gnulib module 
that already implements such a buffer?

However, what to do with signal handling then?  Drop the SA_RESTART flag and 
implement EINTR loop only for the fwrite() call that writes data with escape 
sequences to the terminal?

Kamil

> Pádraig

bug#24232: ls --color cannot be interrupted by a signal

[Kamil Dudka]

If 'ls --color' outputs to a terminal and a syscall blocks (e.g. while reading 
a directory from unresponsive network file system), it cannot be interrupted 
by a signal.

This seems to be caused by the code of ls, which sets the SA_RESTART flag on 
terminating signals.  A possible solution would be to reset the flag prior to 
calling certain blocking syscalls and process the signals synchronously in an 
EINTR loop.

Alternatively, we can document this behavior as intended (or broken by design) 
and suggest users to disable color output or redirect the output off terminal 
to work around the issue.

Any other idea how to solve it?

Originally reported at: https://bugzilla.redhat.com/1365933

Kamil

bug#24015: [PATCH v2 1/3] sort: deduplicate code for traversing numbers

[Kamil Dudka]

On Monday, July 18, 2016 22:28:56 Pádraig Brady wrote:
> On 18/07/16 22:07, Pádraig Brady wrote:
> > Excellent. I'll push all three patches.
> > 
> > I'll adjust the first summary like s/sort:/maint: sort.c:/
> > since there is no functionality change
> > 
> > I'll also add the check for the sv_SE locale in the test.
> 
> Oops you'd done that already.
> 
> One change I did make to the test was to change
> the exp{1,3} bashism to exp1 exp3, as the tests need
> to run under any POSIX compliant shell.
> I tested that with:
> 
>   make SHELL=/bin/dash TESTS=tests/misc/sort-h-thousands-sep.sh SUBDIRS=.
> check

Perfect.  Thanks for the improvements!

Kamil

> cheers,
> Pádraig

bug#24015: [PATCH v2 3/3] sort: with -h, disallow thousands separator between number and unit

[Kamil Dudka]

* src/sort.c (traverse_raw_number): Accept thousands separator only
if it is immediately followed by a digit.
* tests/misc/sort-h-thousands-sep.sh: Cover the fix for this bug.

Suggested by Pádraig Brady in http://bugs.gnu.org/24015
---
 src/sort.c | 11 ++-
 tests/misc/sort-h-thousands-sep.sh | 24 
 2 files changed, 22 insertions(+), 13 deletions(-)

diff --git a/src/sort.c b/src/sort.c
index 038f6ae..6b2dc84 100644
--- a/src/sort.c
+++ b/src/sort.c
@@ -1895,6 +1895,7 @@ traverse_raw_number (char const **number)
   char const *p = *number;
   unsigned char ch;
   unsigned char max_digit = '\0';
+  bool ends_with_thousands_sep = false;
 
   /* Scan to end of number.
  Decimals or separators not followed by digits stop the scan.
@@ -1910,10 +1911,18 @@ traverse_raw_number (char const **number)
   /* Allow to skip only one occurrence of thousands_sep to avoid finding
  the unit in the next column in case thousands_sep matches as blank
  and is used as column delimiter.  */
-  if (*p == thousands_sep)
+  ends_with_thousands_sep = (*p == thousands_sep);
+  if (ends_with_thousands_sep)
 ++p;
 }
 
+  if (ends_with_thousands_sep)
+{
+  /* thousands_sep not followed by digit is not allowed.  */
+  *number = p - /* already incremented twice */ 2;
+  return max_digit;
+}
+
   if (ch == decimal_point)
 while (ISDIGIT (ch = *p++))
   if (max_digit < ch)
diff --git a/tests/misc/sort-h-thousands-sep.sh 
b/tests/misc/sort-h-thousands-sep.sh
index 17f1b6c..1168268 100755
--- a/tests/misc/sort-h-thousands-sep.sh
+++ b/tests/misc/sort-h-thousands-sep.sh
@@ -21,25 +21,25 @@ print_ver_ sort
 test "$(LC_ALL=sv_SE locale thousands_sep)" = ' ' \
   || skip_ 'The Swedish locale with blank thousands separator is unavailable.'
 
-tee exp1 > in << _EOF_
-1   1k  4 003   1M
-2k  2M  4 002   2
-3M  3   4 001   3k
+tee exp{1,3} > in << _EOF_
+1   1k  1 M 4 003   1M
+2k  2M  2 k 4 002   2
+3M  3   3 G 4 001   3k
 _EOF_
 
 cat > exp2 << _EOF_
-3M  3   4 001   3k
-1   1k  4 003   1M
-2k  2M  4 002   2
+3M  3   3 G 4 001   3k
+1   1k  1 M 4 003   1M
+2k  2M  2 k 4 002   2
 _EOF_
 
-cat > exp3 << _EOF_
-3M  3   4 001   3k
-2k  2M  4 002   2
-1   1k  4 003   1M
+cat > exp5 << _EOF_
+3M  3   3 G 4 001   3k
+2k  2M  2 k 4 002   2
+1   1k  1 M 4 003   1M
 _EOF_
 
-for i in 1 2 3; do
+for i in 1 2 3 5; do
   LC_ALL="sv_SE.utf8" sort -h -k $i "in" > "out${i}" || fail=1
   compare "exp${i}" "out${i}" || fail=1
 done
-- 
2.5.5

bug#24015: [PATCH v2 1/3] sort: deduplicate code for traversing numbers

[Kamil Dudka]

* src/sort.c (traverse_raw_number): New function for traversing numbers.
(find_unit_order): Use traverse_raw_number () instead of open-coding it.
(debug_key): Use traverse_raw_number () instead of open-coding it.
---
 src/sort.c | 63 ++
 1 file changed, 35 insertions(+), 28 deletions(-)

diff --git a/src/sort.c b/src/sort.c
index f717604..58c1167 100644
--- a/src/sort.c
+++ b/src/sort.c
@@ -1885,18 +1885,16 @@ static char const unit_order[UCHAR_LIM] =
 #endif
   };
 
-/* Return an integer that represents the order of magnitude of the
-   unit following the number.  The number may contain thousands
-   separators and a decimal point, but it may not contain leading blanks.
-   Negative numbers get negative orders; zero numbers have a zero order.  */
-
-static int _GL_ATTRIBUTE_PURE
-find_unit_order (char const *number)
+/* Traverse number given as *number consisting of digits, thousands_sep, and
+   decimal_point chars only.  Returns the highest digit found in the number,
+   or '\0' if no digit has been found.  Upon return *number points at the
+   character that immediately follows after the given number.  */
+static unsigned char
+traverse_raw_number (char const **number)
 {
-  bool minus_sign = (*number == '-');
-  char const *p = number + minus_sign;
-  int nonzero = 0;
+  char const *p = *number;
   unsigned char ch;
+  unsigned char max_digit = '\0';
 
   /* Scan to end of number.
  Decimals or separators not followed by digits stop the scan.
@@ -1907,16 +1905,34 @@ find_unit_order (char const *number)
   do
 {
   while (ISDIGIT (ch = *p++))
-nonzero |= ch - '0';
+if (max_digit < ch)
+  max_digit = ch;
 }
   while (ch == thousands_sep);
 
   if (ch == decimal_point)
 while (ISDIGIT (ch = *p++))
-  nonzero |= ch - '0';
+  if (max_digit < ch)
+max_digit = ch;
+
+  *number = p - 1;
+  return max_digit;
+}
+
+/* Return an integer that represents the order of magnitude of the
+   unit following the number.  The number may contain thousands
+   separators and a decimal point, but it may not contain leading blanks.
+   Negative numbers get negative orders; zero numbers have a zero order.  */
 
-  if (nonzero)
+static int _GL_ATTRIBUTE_PURE
+find_unit_order (char const *number)
+{
+  bool minus_sign = (*number == '-');
+  char const *p = number + minus_sign;
+  unsigned char max_digit = traverse_raw_number (&p);
+  if ('0' < max_digit)
 {
+  unsigned char ch = *p;
   int order = unit_order[ch];
   return (minus_sign ? -order : order);
 }
@@ -2293,23 +2309,14 @@ debug_key (struct line const *line, struct keyfield 
const *key)
 ignore_value (strtold (beg, &tighter_lim));
   else if (key->numeric || key->human_numeric)
 {
-  char *p = beg + (beg < lim && *beg == '-');
-  bool found_digit = false;
-  unsigned char ch;
-
-  do
+  char const *p = beg + (beg < lim && *beg == '-');
+  unsigned char max_digit = traverse_raw_number (&p);
+  if ('0' <= max_digit)
 {
-  while (ISDIGIT (ch = *p++))
-found_digit = true;
+  unsigned char ch = *p;
+  tighter_lim = (char *) p
++ (key->human_numeric && unit_order[ch]);
 }
-  while (ch == thousands_sep);
-
-  if (ch == decimal_point)
-while (ISDIGIT (ch = *p++))
-  found_digit = true;
-
-  if (found_digit)
-tighter_lim = p - ! (key->human_numeric && unit_order[ch]);
 }
   else
 tighter_lim = lim;
-- 
2.5.5

bug#24015: [PATCH v2 2/3] sort: make -h work with -k and blank used as thousands separator

[Kamil Dudka]

* src/sort.c (traverse_raw_number): Allow to skip only one occurrence
of thousands_sep to avoid finding the unit in the next column in case
thousands_sep matches as blank and is used as column delimiter.
* tests/misc/sort-h-thousands-sep.sh: Add regression test for this bug.
* tests/local.mk: Reference the test.
* NEWS: Mention the bug fix.
Reported at https://bugzilla.redhat.com/1355780
---
 NEWS   |  2 ++
 src/sort.c | 14 
 tests/local.mk |  1 +
 tests/misc/sort-h-thousands-sep.sh | 47 ++
 4 files changed, 59 insertions(+), 5 deletions(-)
 create mode 100755 tests/misc/sort-h-thousands-sep.sh

diff --git a/NEWS b/NEWS
index 4d8fb45..736b95e 100644
--- a/NEWS
+++ b/NEWS
@@ -21,6 +21,8 @@ GNU coreutils NEWS-*- 
outline -*-
   nl now resets numbering for each page section rather than just for each page.
   [This bug was present in "the beginning".]
 
+  sort -h -k now works even in locales that use blank as thousands separator.
+
   stty --help no longer outputs extraneous gettext header lines
   for translated languages. [bug introduced in coreutils-8.24]
 
diff --git a/src/sort.c b/src/sort.c
index 58c1167..038f6ae 100644
--- a/src/sort.c
+++ b/src/sort.c
@@ -1902,13 +1902,17 @@ traverse_raw_number (char const **number)
  to be lacking in units.
  FIXME: add support for multibyte thousands_sep and decimal_point.  */
 
-  do
+  while (ISDIGIT (ch = *p++))
 {
-  while (ISDIGIT (ch = *p++))
-if (max_digit < ch)
-  max_digit = ch;
+  if (max_digit < ch)
+max_digit = ch;
+
+  /* Allow to skip only one occurrence of thousands_sep to avoid finding
+ the unit in the next column in case thousands_sep matches as blank
+ and is used as column delimiter.  */
+  if (*p == thousands_sep)
+++p;
 }
-  while (ch == thousands_sep);
 
   if (ch == decimal_point)
 while (ISDIGIT (ch = *p++))
diff --git a/tests/local.mk b/tests/local.mk
index 27cbf6e..889142a 100644
--- a/tests/local.mk
+++ b/tests/local.mk
@@ -348,6 +348,7 @@ all_tests = \
   tests/misc/sort-discrim.sh   \
   tests/misc/sort-files0-from.pl   \
   tests/misc/sort-float.sh \
+  tests/misc/sort-h-thousands-sep.sh   \
   tests/misc/sort-merge.pl \
   tests/misc/sort-merge-fdlimit.sh \
   tests/misc/sort-month.sh \
diff --git a/tests/misc/sort-h-thousands-sep.sh 
b/tests/misc/sort-h-thousands-sep.sh
new file mode 100755
index 000..17f1b6c
--- /dev/null
+++ b/tests/misc/sort-h-thousands-sep.sh
@@ -0,0 +1,47 @@
+#!/bin/sh
+# exercise 'sort -h' in locales where thousands separator is blank
+
+# Copyright (C) 2016 Free Software Foundation, Inc.
+
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see .
+
+. "${srcdir=.}/tests/init.sh"; path_prepend_ ./src
+print_ver_ sort
+test "$(LC_ALL=sv_SE locale thousands_sep)" = ' ' \
+  || skip_ 'The Swedish locale with blank thousands separator is unavailable.'
+
+tee exp1 > in << _EOF_
+1   1k  4 003   1M
+2k  2M  4 002   2
+3M  3   4 001   3k
+_EOF_
+
+cat > exp2 << _EOF_
+3M  3   4 001   3k
+1   1k  4 003   1M
+2k  2M  4 002   2
+_EOF_
+
+cat > exp3 << _EOF_
+3M  3   4 001   3k
+2k  2M  4 002   2
+1   1k  4 003   1M
+_EOF_
+
+for i in 1 2 3; do
+  LC_ALL="sv_SE.utf8" sort -h -k $i "in" > "out${i}" || fail=1
+  compare "exp${i}" "out${i}" || fail=1
+done
+
+Exit $fail
-- 
2.5.5

bug#24015: [PATCH] sort: make -h work with -k and blank used as thousands separator

[Kamil Dudka]

* src/sort.c (find_unit_order): Allow to skip only one occurrence
of thousands_sep to avoid finding the unit in the next column in case
thousands_sep matches as blank and is used as column delimiter.
* tests/misc/sort-h-thousands-sep.sh: Add regression test for this bug.
* tests/local.mk: Reference the test.
Reported at https://bugzilla.redhat.com/1355780
---
 src/sort.c | 12 ++
 tests/local.mk |  1 +
 tests/misc/sort-h-thousands-sep.sh | 45 ++
 3 files changed, 54 insertions(+), 4 deletions(-)
 create mode 100755 tests/misc/sort-h-thousands-sep.sh

diff --git a/src/sort.c b/src/sort.c
index f717604..a2cadda 100644
--- a/src/sort.c
+++ b/src/sort.c
@@ -1904,12 +1904,16 @@ find_unit_order (char const *number)
  to be lacking in units.
  FIXME: add support for multibyte thousands_sep and decimal_point.  */
 
-  do
+  while (ISDIGIT (ch = *p++))
 {
-  while (ISDIGIT (ch = *p++))
-nonzero |= ch - '0';
+  nonzero |= ch - '0';
+
+  /* Allow to skip only one occurrence of thousands_sep to avoid finding
+ the unit in the next column in case thousands_sep matches as blank
+ and is used as column delimiter.  */
+  if (*p == thousands_sep)
+++p;
 }
-  while (ch == thousands_sep);
 
   if (ch == decimal_point)
 while (ISDIGIT (ch = *p++))
diff --git a/tests/local.mk b/tests/local.mk
index 27cbf6e..889142a 100644
--- a/tests/local.mk
+++ b/tests/local.mk
@@ -348,6 +348,7 @@ all_tests = \
   tests/misc/sort-discrim.sh   \
   tests/misc/sort-files0-from.pl   \
   tests/misc/sort-float.sh \
+  tests/misc/sort-h-thousands-sep.sh   \
   tests/misc/sort-merge.pl \
   tests/misc/sort-merge-fdlimit.sh \
   tests/misc/sort-month.sh \
diff --git a/tests/misc/sort-h-thousands-sep.sh 
b/tests/misc/sort-h-thousands-sep.sh
new file mode 100755
index 000..a1e02de
--- /dev/null
+++ b/tests/misc/sort-h-thousands-sep.sh
@@ -0,0 +1,45 @@
+#!/bin/sh
+# exercise 'sort -h' in locales where thousands separator is blank
+
+# Copyright (C) 2016 Free Software Foundation, Inc.
+
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see .
+
+. "${srcdir=.}/tests/init.sh"; path_prepend_ ./src
+print_ver_ sort
+
+tee in > exp1 << _EOF_
+1   1k  4 003   1M
+2k  2M  4 002   2
+3M  3   4 001   3k
+_EOF_
+
+cat > exp2 << _EOF_
+3M  3   4 001   3k
+1   1k  4 003   1M
+2k  2M  4 002   2
+_EOF_
+
+cat > exp3 << _EOF_
+3M  3   4 001   3k
+2k  2M  4 002   2
+1   1k  4 003   1M
+_EOF_
+
+for i in 1 2 3; do
+  LC_ALL="sv_SE.utf8" sort -h -k $i "in" > "out${i}" || fail=1
+  compare "exp${i}" "out${i}" || fail=1
+done
+
+Exit $fail
-- 
2.5.5

bug#23868: [PATCH v2] install: with -Z, set default SELinux context also for directories

[Kamil Dudka]

* doc/coreutils.texi (install invocation): Update -Z documentation.
* src/install.c (make_ancestor): Set default security context before
calling mkdir() if the -Z option is given.
(process_dir): Call restorecon() on the destination directory if the
-Z option is given.
(usage): Update -Z documentation.
* tests/install/install-Z-selinux.sh: A new test for 'install -Z -D'
and 'install -Z -d' based on tests/mkdir/restorecon.sh.
* tests/local.mk: Reference the test.

Reported at https://bugzilla.redhat.com/1339135
---
 doc/coreutils.texi |  2 +-
 src/install.c  | 33 ++
 tests/install/install-Z-selinux.sh | 58 ++
 tests/local.mk |  1 +
 4 files changed, 88 insertions(+), 6 deletions(-)
 create mode 100755 tests/install/install-Z-selinux.sh

diff --git a/doc/coreutils.texi b/doc/coreutils.texi
index 47c63db..914aec7 100644
--- a/doc/coreutils.texi
+++ b/doc/coreutils.texi
@@ -9217,7 +9217,7 @@ Print the name of each file before moving it.
 @cindex security context
 This option functions similarly to the @command{restorecon} command,
 by adjusting the SELinux security context according
-to the system default type for destination files.
+to the system default type for destination files and each created directory.
 
 @end table
 
diff --git a/src/install.c b/src/install.c
index 2ff279c..1e1fed5 100644
--- a/src/install.c
+++ b/src/install.c
@@ -39,6 +39,7 @@
 #include "prog-fprintf.h"
 #include "quote.h"
 #include "savewd.h"
+#include "selinux.h"
 #include "stat-time.h"
 #include "utimens.h"
 #include "xstrtol.h"
@@ -423,6 +424,12 @@ announce_mkdir (char const *dir, void *options)
 static int
 make_ancestor (char const *dir, char const *component, void *options)
 {
+  struct cp_options const *x = options;
+  if (x->set_security_context && defaultcon (dir, S_IFDIR) < 0
+  && ! ignorable_ctx_err (errno))
+error (0, errno, _("failed to set default creation context for %s"),
+   quoteaf (dir));
+
   int r = mkdir (component, DEFAULT_MODE);
   if (r == 0)
 announce_mkdir (dir, options);
@@ -433,12 +440,28 @@ make_ancestor (char const *dir, char const *component, 
void *options)
 static int
 process_dir (char *dir, struct savewd *wd, void *options)
 {
-  return (make_dir_parents (dir, wd,
-make_ancestor, options,
-dir_mode, announce_mkdir,
-dir_mode_bits, owner_id, group_id, false)
+  struct cp_options const *x = options;
+
+  int ret = (make_dir_parents (dir, wd, make_ancestor, options,
+   dir_mode, announce_mkdir,
+   dir_mode_bits, owner_id, group_id, false)
   ? EXIT_SUCCESS
   : EXIT_FAILURE);
+
+  /* FIXME: Due to the current structure of make_dir_parents()
+ we don't have the facility to call defaultcon() before the
+ final component of DIR is created.  So for now, create the
+ final component with the context from previous component
+ and here we set the context for the final component. */
+  if (ret == EXIT_SUCCESS && x->set_security_context)
+{
+  if (! restorecon (last_component (dir), false, false)
+  && ! ignorable_ctx_err (errno))
+error (0, errno, _("failed to restore context for %s"),
+   quoteaf (dir));
+}
+
+  return ret;
 }
 
 /* Copy file FROM onto file TO, creating TO if necessary.
@@ -651,7 +674,7 @@ In the 4th form, create all components of the given 
DIRECTORY(ies).\n\
   fputs (_("\
   --preserve-context  preserve SELinux security context\n\
   -Z  set SELinux security context of destination\n\
-file to default type\n\
+file and each created directory to default type\n\
   --context[=CTX] like -Z, or if CTX is specified then set the\n\
 SELinux or SMACK security context to CTX\n\
 "), stdout);
diff --git a/tests/install/install-Z-selinux.sh 
b/tests/install/install-Z-selinux.sh
new file mode 100755
index 000..9c3b642
--- /dev/null
+++ b/tests/install/install-Z-selinux.sh
@@ -0,0 +1,58 @@
+#!/bin/sh
+# test 'install -Z -D' and 'install -Z -d'
+# based on tests/mkdir/restorecon.sh
+
+# Copyright (C) 2013-2016 Free Software Foundation, Inc.
+
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, s

bug#23868: [PATCH] install: with -Z, set default SELinux context also for directories

[Kamil Dudka]

* doc/coreutils.texi (install invocation): Update -Z documentation.
* src/install.c (make_ancestor): Set default security context before
calling mkdir() if the -Z option was given.
(process_dir): Call restorecon() on the destination directory if the -Z
option was given.
(usage): Update -Z documentation.

Reported at https://bugzilla.redhat.com/1339135
---
 doc/coreutils.texi |  2 +-
 src/install.c  | 33 -
 2 files changed, 29 insertions(+), 6 deletions(-)

diff --git a/doc/coreutils.texi b/doc/coreutils.texi
index 47c63db..36cad87 100644
--- a/doc/coreutils.texi
+++ b/doc/coreutils.texi
@@ -9217,7 +9217,7 @@ Print the name of each file before moving it.
 @cindex security context
 This option functions similarly to the @command{restorecon} command,
 by adjusting the SELinux security context according
-to the system default type for destination files.
+to the system default type for destination files (and each created directory).
 
 @end table
 
diff --git a/src/install.c b/src/install.c
index 2ff279c..25159c2 100644
--- a/src/install.c
+++ b/src/install.c
@@ -39,6 +39,7 @@
 #include "prog-fprintf.h"
 #include "quote.h"
 #include "savewd.h"
+#include "selinux.h"
 #include "stat-time.h"
 #include "utimens.h"
 #include "xstrtol.h"
@@ -423,6 +424,12 @@ announce_mkdir (char const *dir, void *options)
 static int
 make_ancestor (char const *dir, char const *component, void *options)
 {
+  struct cp_options const *x = options;
+  if (x->set_security_context && defaultcon (dir, S_IFDIR) < 0
+  && ! ignorable_ctx_err (errno))
+error (0, errno, _("failed to set default creation context for %s"),
+   quoteaf (dir));
+
   int r = mkdir (component, DEFAULT_MODE);
   if (r == 0)
 announce_mkdir (dir, options);
@@ -433,12 +440,28 @@ make_ancestor (char const *dir, char const *component, 
void *options)
 static int
 process_dir (char *dir, struct savewd *wd, void *options)
 {
-  return (make_dir_parents (dir, wd,
-make_ancestor, options,
-dir_mode, announce_mkdir,
-dir_mode_bits, owner_id, group_id, false)
+  struct cp_options const *x = options;
+
+  int ret = (make_dir_parents (dir, wd, make_ancestor, options,
+   dir_mode, announce_mkdir,
+   dir_mode_bits, owner_id, group_id, false)
   ? EXIT_SUCCESS
   : EXIT_FAILURE);
+
+  /* FIXME: Due to the current structure of make_dir_parents()
+ we don't have the facility to call defaultcon() before the
+ final component of DIR is created.  So for now, create the
+ final component with the context from previous component
+ and here we set the context for the final component. */
+  if (ret == EXIT_SUCCESS && x->set_security_context)
+{
+  if (! restorecon (last_component (dir), false, false)
+  && ! ignorable_ctx_err (errno))
+error (0, errno, _("failed to restore context for %s"),
+   quoteaf (dir));
+}
+
+  return ret;
 }
 
 /* Copy file FROM onto file TO, creating TO if necessary.
@@ -651,7 +674,7 @@ In the 4th form, create all components of the given 
DIRECTORY(ies).\n\
   fputs (_("\
   --preserve-context  preserve SELinux security context\n\
   -Z  set SELinux security context of destination\n\
-file to default type\n\
+file (and each created directory) to default 
type\n\
   --context[=CTX] like -Z, or if CTX is specified then set the\n\
 SELinux or SMACK security context to CTX\n\
 "), stdout);
-- 
2.5.5

bug#12020: ls should show when extended system attributes are set

[Kamil Dudka]

On Sunday, July 22, 2012 18:16:39 Jim Meyering wrote:
> Kamil Dudka wrote:
> > On Sunday, July 22, 2012 14:40:46 Jim Meyering wrote:
> >> When already using --color, we do get each test result for free
> > 
> > Not really.  The check for file capabilities is optional even with
> > --color.
> > The 'ca' indicator in $LS_COLORS needs to be set to a color to enable
> > this.
> 
> Hi Kamil,
> 
> While true that you can disable it,
> with the default color settings, that indicator is set,
> 
>$ dircolors --pr |grep CAP
>CAPABILITY 30;41 # file with capability
> 
> so with --color, the check is performed unless you arrange
> to turn it off.

Yes, there is a default color set for file capabilities.  I just wanted to 
highlight that --color does not imply we check for capabilities on its own. 
The way to skip the check even with --color is there intentionally because
of the following bug:

https://bugzilla.redhat.com/467508

There is also a request similar to this one in Red Hat Bugzilla:

https://bugzilla.redhat.com/647786

Kamil

bug#12020: ls should show when extended system attributes are set

[Kamil Dudka]

On Sunday, July 22, 2012 14:40:46 Jim Meyering wrote:
> When already using --color, we do get each test result for free

Not really.  The check for file capabilities is optional even with --color.  
The 'ca' indicator in $LS_COLORS needs to be set to a color to enable this.

Kamil

bug#10282: change in behavior of du with multiple arguments (commit efe53cc)

[Kamil Dudka]

On Tuesday 13 December 2011 18:16:12 Eric Blake wrote:
> I think the proposal is to add a new option that forces du to reset its
> duplicate inode hash table for each command line argument, to make
> behavior more like traditional du, even though it means -s can then
> output a larger usage by summing the first column than what you would
> get by the default behavior, when encountering command line arguments
> that are a duplicate with an inode already traversed earlier in the
> command line.  --count-links isn't quite right, because you still want
> to elide links within a single directory of the command-line argument.
> Or maybe --count-links gains an optional argument, that says how to
> count links:
> 
> --count-links=none -> POSIX behavior (if POSIX requires elision across
> command line arguments
> --count-links=per-directory -> traditional behavior, resetting hash
> between command line arguments
> --count-links == --count-links=all -> count every file on every encounter

What would be the difference between the 'per-directory' variant and invoking 
du multiple times, giving it one argument at a time?

Kamil

bug#10282: change in behavior of du with multiple arguments (commit efe53cc)

[Kamil Dudka]

On Tuesday 13 December 2011 08:09:08 Jim Meyering wrote:
> Paul Eggert wrote:
> > On 12/12/11 14:58, Eric Blake wrote:
> >> "Files with multiple links shall be counted and written for only one
> >> entry. The directory entry that is selected in the report is
> >> unspecified."
> >
> > Yes, that's partly what motivates the current GNU du behavior:
> > the idea is to implement this notion consistently (historical
> > 'du' implementations do not).
> >
> >> But even historically, command line arguments were always listed, even
> >> if they are otherwise multiple links.
> >
> > I suppose we could change GNU 'du' to output "0 X" for a command-line
> > argument X that's already been seen.
> 
> This seems sensible.
> 
> > This wouldn't address the problem
> > perceived by the original poster, though.  And it's a glitch from the
> > point of view of consistency.
> 
> I agree that printing "0 X" for these seems inconsistent with the
> elision mandated for the second and subsequent encounter of a file,
> but I suppose command line arguments are intrinsically different
> enough that handling them specially makes sense.  Maybe even as
> the default.
> 
> > Perhaps 'du' needs a new option to control what to do with
> > files that 'du' has already seen before. something that
> > generalizes --count-links.
> 
> That sounds like a good way to do it.
> Anyone interested?

Thank all of you for looking at the issue.  If I understand it correctly, the 
old behavior was violating POSIX whereas the current default behavior is 
correct.  I tried du --count-links with the original reproducer and it seemed 
to work fine.  So what would be the point in adding a new option?

Kamil

bug#10282: change in behavior of du with multiple arguments (commit efe53cc)

[Kamil Dudka]

Hi,

the following upstream commit introduces a major change in behavior of du
when multiple arguments are specified:

http://git.savannah.gnu.org/cgit/coreutils.git/commit/?id=efe53cc

... and the issue has landed as a bug in our Bugzilla:

https://bugzilla.redhat.com/747075#c3

Was such a change in behavior intended?  I am asking as I was not able to
find it documented anywhere.  The up2date man page states:

Summarize disk usage of each FILE, recursively for directories.

..., where FILE refers to a single argument given to du.  The info 
documentation states:

The FILE argument order affects which links are counted, and changing the
argument order may change the numbers that `du' outputs.

However, changing the numbers is one thing and missing lines in the output
of du is quite another thing.

Could anybody please clarify the current behavior of du?  Thanks in advance!

Kamil

Re: ls command

[Kamil Dudka]

On Wednesday 17 of March 2010 23:14:56 Andreas Schwab wrote:
> That message comes from the shell (csh or tcsh).

Indeed, confirmed off-list:

On Thursday 18 of March 2010 16:53:27 Paul Gerber wrote:
> The problem has been solved:
> https://bugzilla.redhat.com/show_bug.cgi?id=529703

Kamil

Re: ls command

[Kamil Dudka]

On Wed March 17 2010 18:17:38 Paul Gerber wrote:
> What can one do if something that worked for years in various Unix
> version stops working in a new release?
> I assumed there would be continuity in such basic things.

So far you didn't point us to any bug and/or change.  Please specify what 
exactly are you trying, including a simple script which creates those files
so that we can easily try it and compare.  And let me note you are at 
completely wrong mailing list.  You need to discover which shell are you 
actually using and look for the appropriate mailing list.

Kamil

Re: ls command

[Kamil Dudka]

On Wed March 17 2010 15:32:06 Paul Gerber wrote:
> RHEL5: The following two commands give different answers (Written by
> Richard Stallman and David MacKenzie). Is that intended?
> 
> Best regards,  Paul Gerber
> 
> > /bin/ls *.pdb
> > 
> > 1fkn.pdb water_cluster.pdb
> > 
> > /bin/ls  *.pdb *.pdb.Z *.pdb.gz
> > /bin/ls: No match

You are fighting with the shell wildcard expansion.  I suggest to check what 
arguments really go the the ls(1) utility in the first place.

Kamil

Re: [PATCH] sort -V now ignores leading white spaces

[Kamil Dudka]

Hi Pádraig,

On Wed March 3 2010 14:45:50 Pádraig Brady wrote:
> Also if your input is like this can one
> just specify the -b flag globally or for the key?

you're right.  The option -b solves the problem fairly well.  The patch is 
pointless.  I've just taken the code from -n/-h because I hadn't been aware
of the -b option.  Thank you for the hint!

Kamil

[PATCH] sort -V now ignores leading white spaces

[Kamil Dudka]

Hello,

please consider the attached patch, improving sort -V to ignore leading white 
spaces.  The patch makes it possible to sort strings mixed with numbers.  A 
simple test case has been merged directly into tests/misc/sort-version.

Thanks in advance!

Kamil
From 5493d92bca31724b9d26d04f87dd3b9f13d1b123 Mon Sep 17 00:00:00 2001
From: Kamil Dudka 
Date: Wed, 3 Mar 2010 12:36:43 +0100
Subject: [PATCH] sort -V now ignores leading white spaces

* src/sort.c (compare_version): Skip all white spaces before the call of
filevercmp ().
* tests/misc/sort-version: Add a related test case.
* NEWS: Mention the change as a change in behavior.
---
 NEWS|2 ++
 src/sort.c  |   10 +-
 tests/misc/sort-version |4 
 3 files changed, 15 insertions(+), 1 deletions(-)

diff --git a/NEWS b/NEWS
index 2a3ca63..7eb2a0d 100644
--- a/NEWS
+++ b/NEWS
@@ -35,6 +35,8 @@ GNU coreutils NEWS-*- outline -*-
   join -t '' no longer emits an error and instead operates on
   each line as a whole (even if they contain NUL characters).
 
+  sort -V now ignores leading white spaces
+
 
 * Noteworthy changes in release 8.4 (2010-01-13) [stable]
 
diff --git a/src/sort.c b/src/sort.c
index 39cb6d6..c9663eb 100644
--- a/src/sort.c
+++ b/src/sort.c
@@ -1979,6 +1979,8 @@ static int
 compare_version (char *restrict texta, size_t lena,
  char *restrict textb, size_t lenb)
 {
+  char const *a = texta;
+  char const *b = textb;
   int diff;
 
   /* It is necessary to save the character after the end of the field.
@@ -1990,7 +1992,13 @@ compare_version (char *restrict texta, size_t lena,
   texta[lena] = '\0';
   textb[lenb] = '\0';
 
-  diff = filevercmp (texta, textb);
+  /* skip all blanks */
+  while (blanks[to_uchar (*a)])
+a++;
+  while (blanks[to_uchar (*b)])
+b++;
+
+  diff = filevercmp (a, b);
 
   texta[lena] = sv_a;
   textb[lenb] = sv_b;
diff --git a/tests/misc/sort-version b/tests/misc/sort-version
index 741ee8b..60e3e90 100755
--- a/tests/misc/sort-version
+++ b/tests/misc/sort-version
@@ -24,6 +24,8 @@ fi
 . $srcdir/test-lib.sh
 
 cat > in << _EOF_
+11
+ 1
 gcc-c++-10.fc9.tar.gz
 gcc-c++-10.8.12-0.7rc2.fc9.tar.bz2
 glibc-2-0.1.beta1.fc10.rpm
@@ -65,6 +67,8 @@ string start 5.90.0 end of str
 _EOF_
 
 cat > exp << _EOF_
+ 1
+11
 gcc-c++-10.fc9.tar.gz
 gcc-c++-10.8.12-0.7rc2.fc9.tar.bz2
 glibc-2-0.1.beta1.fc10.rpm
-- 
1.6.2.5

Re: (no subject)

[Kamil Dudka]

On Friday 29 of January 2010 18:11:53 Henry Hung wrote:
> Hi
> I don't know whether this is a design intent or a bug.
> When I tried to list recursively a specified file, ls only searched the
> current directory for the file, not recursively. For example, ls -R myFile*

http://www.gnu.org/software/coreutils/faq/coreutils-faq.html#Why-doesn_0027t-rm-_002dr-_002a_002epattern-recurse-like-it-should_003f

Kamil

Re: [PATCH v2] who --mesg now checks the group of TTY devices

[Kamil Dudka]

On Saturday 23 of January 2010 02:43:39 Kamil Dudka wrote:
> On Saturday 23 of January 2010 00:54:44 Kamil Dudka wrote:
> > # ls -l /dev/tty?
> > crw--w. 1 root root 4, 0 2010-01-22 18:48 /dev/tty0
> > crw--w. 1 root root 4, 1 2010-01-22 18:48 /dev/tty1
> > crw--w. 1 root tty  4, 2 2010-01-22 18:50 /dev/tty2
> > crw---. 1 root root 4, 3 2010-01-22 18:48 /dev/tty3
> > crw---. 1 root root 4, 4 2010-01-22 18:48 /dev/tty4
> > crw---. 1 root root 4, 5 2010-01-22 18:48 /dev/tty5
> > crw---. 1 root root 4, 6 2010-01-22 18:48 /dev/tty6
> > crw--w. 1 root tty  4, 7 2010-01-22 18:48 /dev/tty7
> > crw--w. 1 root tty  4, 8 2010-01-22 18:48 /dev/tty8
> > crw--w. 1 root tty  4, 9 2010-01-22 18:48 /dev/tty9
>
> The listing above is in fact a bit misleading since the group is changed
> to "tty" within login.c from util-linux-ng.

Note that util-linux-ng is not the only package providing login.

Other systems may use e.g. shadow.  Then the TTY group can be set 
in /etc/login.defs.  Look at the TTYGROUP option here:

http://vmlinux.org/cgi-bin/dwww?type=runman&location=login.defs/5

> Kamil

Re: [PATCH v2] who --mesg now checks the group of TTY devices

[Kamil Dudka]

On Saturday 23 of January 2010 00:54:44 Kamil Dudka wrote:
> # ls -l /dev/tty?
> crw--w. 1 root root 4, 0 2010-01-22 18:48 /dev/tty0
> crw--w. 1 root root 4, 1 2010-01-22 18:48 /dev/tty1
> crw--w. 1 root tty  4, 2 2010-01-22 18:50 /dev/tty2
> crw---. 1 root root 4, 3 2010-01-22 18:48 /dev/tty3
> crw---. 1 root root 4, 4 2010-01-22 18:48 /dev/tty4
> crw---. 1 root root 4, 5 2010-01-22 18:48 /dev/tty5
> crw---. 1 root root 4, 6 2010-01-22 18:48 /dev/tty6
> crw--w. 1 root tty  4, 7 2010-01-22 18:48 /dev/tty7
> crw--w. 1 root tty  4, 8 2010-01-22 18:48 /dev/tty8
> crw--w. 1 root tty  4, 9 2010-01-22 18:48 /dev/tty9

The listing above is in fact a bit misleading since the group is changed
to "tty" within login.c from util-linux-ng.

Kamil

Re: [PATCH v2] who --mesg now checks the group of TTY devices

[Kamil Dudka]

On Friday 22 of January 2010 17:03:51 Jim Meyering wrote:
> Can you reproduce the situation in which this new configure-time
> option is required on e.g., Fedora 12?  So far, I haven't been able to,
> since all tty devices properly get the "tty" group, and in that
> case, the simple perm-comparison test is adequate.

What I am getting on a sane F-12 installation follows:

# ls -l /dev/tty?
crw--w. 1 root root 4, 0 2010-01-22 18:48 /dev/tty0
crw--w. 1 root root 4, 1 2010-01-22 18:48 /dev/tty1
crw--w. 1 root tty  4, 2 2010-01-22 18:50 /dev/tty2
crw---. 1 root root 4, 3 2010-01-22 18:48 /dev/tty3
crw---. 1 root root 4, 4 2010-01-22 18:48 /dev/tty4
crw---. 1 root root 4, 5 2010-01-22 18:48 /dev/tty5
crw---. 1 root root 4, 6 2010-01-22 18:48 /dev/tty6
crw--w. 1 root tty  4, 7 2010-01-22 18:48 /dev/tty7
crw--w. 1 root tty  4, 8 2010-01-22 18:48 /dev/tty8
crw--w. 1 root tty  4, 9 2010-01-22 18:48 /dev/tty9

> I realized that I didn't really remember the details,

Nor do I as wasn't anyhow involved in GNU coretuils when the issue
was discussed ;-)

> and NEWS didn't tell me enough, so started rewriting it:
>
>   who: the "+/-" --mesg (-T) indicator of whether a user/tty is accepting
>   messages could be incorrectly listed as "+", when in fact, the user was
>   not accepting messages (mesg no).  Before, who would examine only the
>   permission bits, and not consider the group of the TTY device file.
>   Thus, when a tty file's group would change somehow e.g., to "root",
>   that would make it unwritable (via write(1)) by normal users, in spite
>   of whatever the permission bits might imply.  Now, when configured
>   using the --with-tty-group[=NAME] option, who also compares the group
>   of the TTY device with NAME (or "tty" if no group name is specified).

> How can the group name change (or be set initially) to other than "tty"?
> I suspect this is an issue only on older systems.  If so, I need to
> say that in NEWS, rather than "somehow".

It looks like sort of default state to me.  To be frank, not sure if
intended and/or documented actually :-)

> Other than that, everything looks fine (I moved the "const").

I knew I had forgotten something.  Sorry about that.  It wasn't intentionally.

Kamil

[PATCH v2] who --mesg now checks the group of TTY devices

[Kamil Dudka]

Hi Jim,

attached is the second version of the patch.

On Thursday 21 of January 2010 13:04:27 Jim Meyering wrote:
> >> It would be helpful to say how to determine the appropriate group name.
> >> Something like "ls -lg /dev/tty" or
> >> $ stat --format %G /dev/tty
> >> tty
> >
> > Do you mean directly in the help string or somewhere else?
>
> I was thinking of a comment in the .m4 file,
> but now that you mention the help string, perhaps that's better.
> You choose.

Well, let it be a shell comment then.

> You're right that we go to extremes to avoid in-functions #ifdefs.
> In-function #ifdefs are evil, but so is code duplication.
> It's a trade-off.  Since this function is so small,
> and the fraction of duplicated code would have been so high,
> the in-function #ifdef is clearly the lesser evil.

It makes sense to me.

Kamil
From 55d223fee4900620d7147f134ebebe39fdca90a9 Mon Sep 17 00:00:00 2001
From: Kamil Dudka 
Date: Fri, 22 Jan 2010 15:17:19 +0100
Subject: [PATCH] who --mesg now checks the group of TTY devices

... if coreutils is compiled with --with-tty-group.  Based on a patch
written by Piotr Gackiewicz.  Details at
https://bugzilla.redhat.com/454261

* src/who.c (is_tty_writable): A new function returning true if a TTY
device is writable by the group.  Additionally it checks the group to be
the same as TTY_GROUP_NAME when compiled with --with-tty-group.
* m4/jm-macros.m4: Introduce a new configure option --with-tty-group.
* NEWS: Mention the change.
---
 NEWS|6 ++
 THANKS  |1 +
 m4/jm-macros.m4 |   19 +++
 src/who.c   |   22 +-
 4 files changed, 47 insertions(+), 1 deletions(-)

diff --git a/NEWS b/NEWS
index 530ff95..0af7b9a 100644
--- a/NEWS
+++ b/NEWS
@@ -2,6 +2,12 @@ GNU coreutils NEWS-*- outline -*-
 
 * Noteworthy changes in release ?.? (-??-??) [?]
 
+** New features
+
+  who --mesg used to ignore the group of a TTY device when checking if it is
+  possible to send messages there.  Now, if coreutils is compiled with
+  --with-tty-group[=NAME] configure option, it also compares the group of the
+  TTY device with NAME (or "tty" if no group is specified).
 
 * Noteworthy changes in release 8.4 (2010-01-13) [stable]
 
diff --git a/THANKS b/THANKS
index 1207368..d8cdf82 100644
--- a/THANKS
+++ b/THANKS
@@ -495,6 +495,7 @@ Philippe Schnoebelenphilippe.schnoebe...@imag.fr
 Phillip Jones   mo...@datastacks.com
 Piergiorgio Sartor  sar...@sony.de
 Pieter Bowman   bow...@math.utah.edu
+Piotr Gackiewiczga...@intertele.pl
 Piotr Kwapulinski   k...@univ.gda.pl
 Prashant TR t...@eth.net
 Priit Jõerüüt   jemm4j...@yahoo.com
diff --git a/m4/jm-macros.m4 b/m4/jm-macros.m4
index 2713827..e0efd29 100644
--- a/m4/jm-macros.m4
+++ b/m4/jm-macros.m4
@@ -144,6 +144,25 @@ AC_DEFUN([coreutils_MACROS],
 ])
 
   AC_REQUIRE([AM_LANGINFO_CODESET])
+
+  # configure options --with-tty-group/--without-tty-group
+  # usually you can determine the group of TTYs by 'stat --format %G /dev/tty'
+  # omitting this option is equal to --without-tty-group
+  AC_ARG_WITH([tty-group],
+AS_HELP_STRING([--with-tty-group[[[=NAME,
+  [group used by system for TTYs, "tty" when not specified]
+  [ (default: do not rely on any group used for TTYs)]),
+[tty_group_name=$withval],
+[tty_group_name=no])
+
+  if test "x$tty_group_name" != xno; then
+if test "x$tty_group_name" = xyes; then
+  tty_group_name=tty
+fi
+AC_MSG_NOTICE([TTY group used by system set to "$tty_group_name"])
+AC_DEFINE_UNQUOTED([TTY_GROUP_NAME], ["$tty_group_name"],
+  [group used by system for TTYs])
+  fi
 ])
 
 AC_DEFUN([gl_CHECK_ALL_HEADERS],
diff --git a/src/who.c b/src/who.c
index f71db3b..4110d37 100644
--- a/src/who.c
+++ b/src/who.c
@@ -37,6 +37,10 @@
 #include "hard-locale.h"
 #include "quote.h"
 
+#ifdef TTY_GROUP_NAME
+# include 
+#endif
+
 /* The official name of this program (e.g., no `g' prefix).  */
 #define PROGRAM_NAME "who"
 
@@ -308,6 +312,22 @@ print_line (int userlen, const char *user, const char state,
   free (x_exitstr);
 }
 
+/* Return true if a terminal device given as PSTAT allows other users
+   to send messages to; false otherwise */
+static bool
+is_tty_writable (const struct stat *pstat)
+{
+#ifdef TTY_GROUP_NAME
+  /* Ensure the group of the TTY device matches TTY_GROUP_NAME, more info at
+ https://bugzilla.redhat.com/454261 */
+  struct group *ttygr = getgrnam (TTY_GROUP_NAME);
+  if (!ttygr || (pstat->st_gid != ttygr->gr_gid))
+return false;
+#endif
+
+  return pstat->st_mode & S_IWGRP;
+}

Re: How to overwrite the destination directory by 'cp'?

[Kamil Dudka]

On Thursday 21 of January 2010 23:35:13 Peng Yu wrote:
> Suppose I have directory a and b, the following command will copy the
> content of a to b/a, rather than overwrite the directory 'b' by the
> directory 'a'. I'm wondering if there is an option to overwrite 'b'?
>
> cp -r a b

I don't think something like that is possible with cp itself.  It looks a bit 
dangerous to me anyway.  Instead of that, there is a way to avoid the copy 
operation in such case:

$ cp -r --no-target-directory a b

If you want to destroy content of 'b' and replace with content of 'a', then 
you want to do this:

$ rm -rf b; cp -r --no-target-directory a b

Kamil

Re: [PATCH] who: --mesg now respects also group of a TTY

[Kamil Dudka]

Hi Jim,

thanks for review!

On Thursday 21 of January 2010 12:28:03 Jim Meyering wrote:
> Actually, more comments (in the code and log) would be welcome, too ;-)

Sure.  I also forgot the credit Piotr Gackiewicz as the original author
in the first place...

> Please add a little explanation before the new function, is_tty_writable

What about "Return true if a terminal device given as PSTAT allows other users 
to send messages to; false otherwise" ?

> and list one or both of those URLs for context in the commit log.

I think the rhbz link should be sufficient.

> > +** New features
> > +
> > +  who --mesg now respects also group of a TTY when compiled with
> > +  --with-tty-group
>
> Saying it "respects the group..." doesn't really communicate what is
> changed.  How about saying that it works now in some cases (details?)
> where it used to fail?

What about this?

"who --mesg used to ignore group of a TTY device when checking if it is 
possible to send messages there.  Now if coreutils is compiled 
with --with-tty-group[=TTY_GROUP_NAME] configure option, it also compares 
group of the TTY device with TTY_GROUP_NAME (or "tty" if no group is 
specified)."

> It would be helpful to say how to determine the appropriate group name.
> Something like "ls -lg /dev/tty" or
> $ stat --format %G /dev/tty
> tty

Do you mean directly in the help string or somewhere else?

>
> > diff --git a/src/who.c b/src/who.c
>
> ...
>
> There is enough duplication below that
> I'd prefer to move the #ifdef/#endif into the function.

I am all for that.  I thought it was prohibited in GNU coreutils :-)

> Also, please humor me and put the "const" after the type name:

np

Kamil

[PATCH] who: --mesg now respects also group of a TTY

[Kamil Dudka]

Hello,

enclosed is a patch making the output of who --mesg more reliable when 
compiled with --with-tty-group.  The issue was discussed before at

https://bugzilla.redhat.com/454261

and later at

http://thread.gmane.org/gmane.comp.gnu.coreutils.bugs/14022

Any comments are welcome!

Kamil
From df522e97bd36b498cf75f4a7edd978f83388cdfd Mon Sep 17 00:00:00 2001
From: Kamil Dudka 
Date: Wed, 20 Jan 2010 16:15:37 +0100
Subject: [PATCH] who: --mesg now respects also group of a TTY

... when compiled with --with-tty-group
* src/who.c (is_tty_writable): A new function returning true if a TTY
device is writable by group.  Additionally it checks the group to be
the same as TTY_GROUP_NAME when compiled with --with-tty-group.
* m4/jm-macros.m4: Introduce a new configure options --with-tty-group.
* NEWS: Mention the change.
---
 NEWS|5 +
 m4/jm-macros.m4 |   17 +
 src/who.c   |   24 +++-
 3 files changed, 45 insertions(+), 1 deletions(-)

diff --git a/NEWS b/NEWS
index 530ff95..de310bd 100644
--- a/NEWS
+++ b/NEWS
@@ -2,6 +2,11 @@ GNU coreutils NEWS-*- outline -*-
 
 * Noteworthy changes in release ?.? (-??-??) [?]
 
+** New features
+
+  who --mesg now respects also group of a TTY when compiled with
+  --with-tty-group
+
 
 * Noteworthy changes in release 8.4 (2010-01-13) [stable]
 
diff --git a/m4/jm-macros.m4 b/m4/jm-macros.m4
index 2713827..1e55db1 100644
--- a/m4/jm-macros.m4
+++ b/m4/jm-macros.m4
@@ -144,6 +144,23 @@ AC_DEFUN([coreutils_MACROS],
 ])
 
   AC_REQUIRE([AM_LANGINFO_CODESET])
+
+  # configure options --with-tty-group/--without-tty-group
+  AC_ARG_WITH([tty-group],
+AS_HELP_STRING([--with-tty-group[[[=NAME,
+  [group used by system for TTYs, "tty" when not specified]
+  [ (default: do not rely on any group used for TTYs)]),
+[tty_group_name=$withval],
+[tty_group_name=no])
+
+  if test "x$tty_group_name" != xno; then
+if test "x$tty_group_name" = xyes; then
+  tty_group_name=tty
+fi
+AC_MSG_NOTICE([TTY group used by system set to "$tty_group_name"])
+AC_DEFINE_UNQUOTED([TTY_GROUP_NAME], ["$tty_group_name"],
+  [group used by system for TTYs])
+  fi
 ])
 
 AC_DEFUN([gl_CHECK_ALL_HEADERS],
diff --git a/src/who.c b/src/who.c
index f71db3b..62bd948 100644
--- a/src/who.c
+++ b/src/who.c
@@ -37,6 +37,10 @@
 #include "hard-locale.h"
 #include "quote.h"
 
+#ifdef TTY_GROUP_NAME
+# include 
+#endif
+
 /* The official name of this program (e.g., no `g' prefix).  */
 #define PROGRAM_NAME "who"
 
@@ -308,6 +312,24 @@ print_line (int userlen, const char *user, const char state,
   free (x_exitstr);
 }
 
+#ifdef TTY_GROUP_NAME
+static bool
+is_tty_writable (const struct stat *pstat)
+{
+  struct group *ttygr = getgrnam (TTY_GROUP_NAME);
+  if (!ttygr || (pstat->st_gid != ttygr->gr_gid))
+return false;
+
+  return pstat->st_mode & S_IWGRP;
+}
+#else
+static bool
+is_tty_writable (const struct stat *pstat)
+{
+  return pstat->st_mode & S_IWGRP;
+}
+#endif
+
 /* Send properly parsed USER_PROCESS info to print_line.  The most
recent boot time is BOOTTIME. */
 static void
@@ -346,7 +368,7 @@ print_user (const STRUCT_UTMP *utmp_ent, time_t boottime)
 
   if (stat (line, &stats) == 0)
 {
-  mesg = (stats.st_mode & S_IWGRP) ? '+' : '-';
+  mesg = is_tty_writable (&stats) ? '+' : '-';
   last_change = stats.st_atime;
 }
   else
-- 
1.6.2.5

Re: [PATCH] ls.c: reorder includes to work around broken

[Kamil Dudka]

On Saturday 09 of January 2010 22:40:08 Jim Meyering wrote:
> I've added details (and "* src/ls.c: ...) to the log.

Thanks for the amendment.

Kamil