On 05/02/2023 16:56, Glenn Golden wrote:
Glenn Golden <g...@zplane.com> [2023-02-02 15:55:51 -0700]:
Pádraig Brady <p...@draigbrady.com> [2023-02-01 20:37:47 +0000]:
That was informative thanks.
What I think is happening is that to support --follow=name tail(1) operates
in non blocking mode so that it doesn't block when reading a file, and has
the opportunity to recheck.
Now it determines when a read() shouldn't block by doing stat() and if size
or mtime haven't changed, then it doesn't perform the read. In fact for non
regular files it only goes on the mtime, which I guess is not changing for
your device.
Yes, that appears to be the case: The mtime is evidently updated only when
the USB host writes to the device; data arriving from the device to the host
does not update mtime.
Note if you were just doing --follow=descriptor on a single file tail(1)
would operate in a simpler blocking manner and would work fine with your
device I expect (not through the changing symlink of course).
I think following by descriptor can't really work reliably for this case,
because the tail process, even though it operates in blocking mode as you say,
nevertheless kind of heisenbergs the situation because it holds the original
fd open, which in turn causes the USB driver to assign a new minor device
number to the device upon its reboot. And that effectively dissociates the
original fd from the device. So tail thereafter just sees EOF forever.
Example: Suppose the original file descriptor that dev/ttyPSLOG points to
is /dev/ttyACM0, and tail has been successfully following that via
--follow=descriptor in blocking mode. Then the device reboots. Upon
detecting the reincarnated device, the driver sees that /dev/ttyACM0 is
still open (because tail hasn't explicitly close()d it, despite the fact
that there's no longer a device behind it and he keeps getting EOF on
every read()). So because /dev/ttyACM0 is still marked as "open", the
driver winds up assigning the reincarnated device a new interface name based
on the next available minor device number, e.g. /dev/ttyACM1. As a result,
the original fd is left pointing to /dev/ttyACM0, which no longer exists
in the filesystem, and read() returns EOF forever. (Why doesn't it instead
return in error upon disappearance of the original device node? I don't know.
But "EOF forever" is what is observed with strace.)
In contrast, if tail -- or any other process for that matter -- did not have
the original fd open when the reboot occurred, only then would the driver
(probably) wind up reassigning /dev/ttyACM0 to the device.
So, perversely, it is the tail process itself, by holding the fd open,
that prevents the behavior we'd like with --follow=descriptor.
I wonder could we try the read() anyway when operating on a
a single non regular file that has successfully been set in non blocking mode?
In that case we shouldn't block and read() would return 0 if no data.
I haven't thought about that much, but the diff below should do it.
At least it shows the part of the code involved,
which compares the various stat() members to determine to read or not.
Perhaps you could add debugging there (or strace -v -e fstat,newfstatat ...).
For example to see if st_size changes for you
(I don't think there is any other stat member we could key on).
Ok, thanks. Let me fiddle around with your updated patch (the one named
"tail-F-dev.patch") and I'll let you know what I find.
Your two patches (the one mentioned above and the earlier one avoiding the
xlseek() on a non-regular file) do almost fix the problem, but not quite.
I added one additional mod, and with that, it now seems to address the
issue fully. (And doesn't seem to break any "make check" tests.)
Oh right I see. Sorry for missing that.
After an fd change we need to go around the loop again
to reinit the local fd and also guard against fd==-1 etc.
Previously we always went around the loop again after a recheck()
so any previous use of fd was fine.
Attaching the final two patches I hope to apply for this.
They should apply to coreutils-9.1
cheers,
Pádraig
From ae5a27b6fd01ccd0816abe0728563531a3f1422c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= <p...@draigbrady.com>
Date: Mon, 30 Jan 2023 21:44:10 +0000
Subject: [PATCH 1/2] tail: fix support for -F with non seekable files
This was seen to be an issue when following a
symlink that was being updated to point to
different underlying devices.
* src/tail.c (recheck): Guard the lseek() call to only
be performed for regular files.
* NEWS: Mention the bug fix.
---
NEWS | 4 ++++
src/tail.c | 3 ++-
2 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/NEWS b/NEWS
index b3cde4a01..6cd9a6198 100644
--- a/NEWS
+++ b/NEWS
@@ -52,6 +52,10 @@ GNU coreutils NEWS -*- outline -*-
Previously it may have output 1 character too wide for certain widths.
[bug introduced in coreutils-5.3]
+ tail --follow=name works again with non seekable files. Previously it
+ exited with an "Illegal seek" error when such a file was replaced.
+ [bug introduced in fileutils-4.1.6]
+
`wc -c` will again efficiently determine the size of large files
on all systems. It no longer redundantly reads data from certain
sized files larger than SIZE_MAX.
diff --git a/src/tail.c b/src/tail.c
index 2244509dd..03061e8bf 100644
--- a/src/tail.c
+++ b/src/tail.c
@@ -1122,7 +1122,8 @@ recheck (struct File_spec *f, bool blocking)
{
/* Start at the beginning of the file. */
record_open_fd (f, fd, 0, &new_stats, (is_stdin ? -1 : blocking));
- xlseek (fd, 0, SEEK_SET, pretty_name (f));
+ if (S_ISREG (new_stats.st_mode))
+ xlseek (fd, 0, SEEK_SET, pretty_name (f));
}
}
--
2.26.2
From eeb933be12752eeac6fa31e4a096f967e4e49794 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= <p...@draigbrady.com>
Date: Wed, 1 Feb 2023 20:41:31 +0000
Subject: [PATCH 2/2] tail: improve --follow=name with single non regular files
* src/tail (tail_forever): Attempt to read() from non blocking
single non regular file, which shouldn't block, but also
read data even when the mtime doesn't change.
* NEWS: Mention the improvement.
* THANKS.in: Thanks for detailed testing.
---
NEWS | 4 ++++
THANKS.in | 1 +
src/tail.c | 14 ++++++++++++--
3 files changed, 17 insertions(+), 2 deletions(-)
diff --git a/NEWS b/NEWS
index 6cd9a6198..89413e94e 100644
--- a/NEWS
+++ b/NEWS
@@ -128,6 +128,10 @@ GNU coreutils NEWS -*- outline -*-
when removing directories. For example EIO will be faithfully
diagnosed, rather than being conflated with ENOTEMPTY.
+ tail --follow=name now works with single non regular files whose
+ mtime doesn't reflect new data being available.
+ Previously tail would not show any new data in this case.
+
* Noteworthy changes in release 9.1 (2022-04-15) [stable]
diff --git a/THANKS.in b/THANKS.in
index a3d179a55..8d903268e 100644
--- a/THANKS.in
+++ b/THANKS.in
@@ -230,6 +230,7 @@ Gerald Pfeifer ger...@pfeifer.com
Gerhard Poul gp...@gnu.org
Germano Leichsenring germ...@jedi.cs.kobe-u.ac.jp
Glen Lenker glen.len...@gmail.com
+Glenn Golden g...@zplane.com
GOTO Masanori go...@debian.or.jp
Greg Louis glo...@dynamicro.on.ca
Greg McGary g...@gnu.org
diff --git a/src/tail.c b/src/tail.c
index 03061e8bf..e2e7f4690 100644
--- a/src/tail.c
+++ b/src/tail.c
@@ -1222,6 +1222,7 @@ tail_forever (struct File_spec *f, size_t n_files, double sleep_interval)
f[i].blocking = blocking;
}
+ bool read_unchanged = false;
if (!f[i].blocking)
{
if (fstat (fd, &stats) != 0)
@@ -1244,9 +1245,14 @@ tail_forever (struct File_spec *f, size_t n_files, double sleep_interval)
recheck (&f[i], f[i].blocking);
f[i].n_unchanged_stats = 0;
}
- continue;
+ if (fd != f[i].fd || S_ISREG (stats.st_mode) || 1 < n_files)
+ continue;
+ else
+ read_unchanged = true;
}
+ assert (fd == f[i].fd);
+
/* This file has changed. Print out what we can, and
then keep looping. */
@@ -1254,7 +1260,8 @@ tail_forever (struct File_spec *f, size_t n_files, double sleep_interval)
f[i].mode = stats.st_mode;
/* reset counter */
- f[i].n_unchanged_stats = 0;
+ if (! read_unchanged)
+ f[i].n_unchanged_stats = 0;
/* XXX: This is only a heuristic, as the file may have also
been truncated and written to if st_size >= size
@@ -1289,6 +1296,9 @@ tail_forever (struct File_spec *f, size_t n_files, double sleep_interval)
bytes_read = dump_remainder (false, name, fd, bytes_to_read);
+ if (read_unchanged && bytes_read)
+ f[i].n_unchanged_stats = 0;
+
any_input |= (bytes_read != 0);
f[i].size += bytes_read;
}
--
2.26.2
