Do not suppress Chmod on non-existent file#260
Conversation
To include fsnotify/fsnotify#260 (fix for fsnotify/fsnotify#194). Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
|
@nathany @markbates @ppknap PTAL |
To include fsnotify/fsnotify#260 (fix for fsnotify/fsnotify#194). Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
486df7f to
cd73d9e
Compare
|
Rebased; the PR description and the commit message slightly updated for clarity. |
cd73d9e to
46c5bfc
Compare
|
It's a pity that no one took a look at this :( the issue is still valid |
vladlosev
left a comment
There was a problem hiding this comment.
Some cleanup note, otherwise LGTM.
211a9dd to
5e53cc7
Compare
|
Rebased; addressed @vladlosev review comment. @nathany PTAL |
|
CI failures look legit but unrelated |
|
@nathany PTAL |
|
@kolyshkin could you please rebase with master, above CI failure should be fixed with this 7f4cf4d |
|
I confirm that small change is working. I'm having this problem using lumberjack, because it keeps the file opened. After this change the following code starts to work: package main
import (
"os"
"log"
"github.com/fsnotify/fsnotify"
"gopkg.in/natefinch/lumberjack.v2"
)
func main() {
myFile := "/var/log/x.log"
file := &lumberjack.Logger{
Filename: myFile,
}
file.Write([]byte{})
file.Close()
watcher, err := fsnotify.NewWatcher()
if err != nil {
log.Println(err)
}
defer watcher.Close()
done := make(chan bool)
go func() {
for {
select {
case event, ok := <-watcher.Events:
log.Println("ok:", ok)
if !ok {
return
}
log.Println("event:", event)
if event.Op&fsnotify.Chmod == fsnotify.Chmod {
file.Close()
} else if event.Op&fsnotify.Rename == fsnotify.Rename || event.Op&fsnotify.Remove == fsnotify.Remove {
file.Write([]byte{})
log.Println("modified file:", event.Name)
if err := watcher.Add(event.Name); err != nil {
log.Println("Unable to watch file: ", event.Name, " ", err)
os.Exit(1)
}
log.Println("started watching file: ", event.Name)
}
case err, ok := <-watcher.Errors:
log.Println("err ok:", ok)
if !ok {
return
}
log.Println("error:", err)
}
}
}()
err = watcher.Add(myFile)
if err != nil {
log.Println(err)
}
<-done
}Before this change, after doing a |
5e53cc7 to
d3f1d2d
Compare
|
Rebased |
d3f1d2d to
e17772f
Compare
|
I validated this PR for https://github.com/nxadm/tail (a tail Go library, using fsnotify). It looks good to me and it fixes bugs related to reopening deleted files. Thanks for your work, @kolyshkin! It would be great if this PR could be reviewed and merged. |
Currently fsnotify suppresses a Chmod event if the file does not exist when the event is received. This makes it impossible to use fsnotify to detect when an opened file is removed. In such case the Linux kernel sends IN_ATTRIB event, as described in inotify(7) man page: > IN_ATTRIB (*) > Metadata changed—for example, permissions (e.g., chmod(2)), > timestamps (e.g., utimensat(2)), extended attributes (setx‐ > attr(2)), link count (since Linux 2.6.25; e.g., for the tar‐ > get of link(2) and for unlink(2)), and user/group ID (e.g., > chown(2)). (to clarify, in this very case it's link count that changes). To fix: * Modify the code to only suppress MODIFY and CREATE events. * Add a test case to verify Chmod event is delivered. While at it, fix the comment in ignoreLinux() to use the up-to-date terminology (event ops). A test case is added. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
5bc399e to
53832bc
Compare
arp242
left a comment
There was a problem hiding this comment.
Rebased on main, and seems good.
One possible concern is that CHMOD events may arrive after the REMOVE, which is what this check addresses in the first place. In this case, the cure seems worse than the disease, but should probably make a note of this in the changelog for the next version.
Currently fsnorify suppresses a Chmod event if the file does not exist
when the event is received.
This makes it impossible to use fsnotify to detect when an opened
file is removed. In such case the Linux kernel sends IN_ATTRIB event,
as described in inotify(7) man page:
(in this very case it's link count that changes).
To fix:
While at it, fix the comment in ignoreLinux() to use the up-to-date
terminology (event ops).
This is heavily based on #205; kudos to @vladlosev for his work.
Fixes #194.
Update
Yes I did