Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not suppress Chmod on non-existent file #260

Merged
merged 1 commit into from Jul 21, 2022

Conversation

kolyshkin
Copy link
Contributor

@kolyshkin kolyshkin commented Aug 16, 2018

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_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)).

(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).

This is heavily based on #205; kudos to @vladlosev for his work.

Fixes #194.

Update

Please indicate that you have signed the CLA in your pull request.

Yes I did

@kolyshkin
Copy link
Contributor Author

kolyshkin added a commit to kolyshkin/moby that referenced this pull request Aug 16, 2018
To include fsnotify/fsnotify#260
(fix for fsnotify/fsnotify#194).

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
@kolyshkin
Copy link
Contributor Author

@nathany @markbates @ppknap PTAL

kolyshkin added a commit to kolyshkin/moby that referenced this pull request Aug 23, 2018
To include fsnotify/fsnotify#260
(fix for fsnotify/fsnotify#194).

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
@kolyshkin
Copy link
Contributor Author

kolyshkin commented Aug 31, 2018

Rebased; the PR description and the commit message slightly updated for clarity.

@kolyshkin
Copy link
Contributor Author

It's a pity that no one took a look at this :( the issue is still valid

nathany
nathany previously approved these changes Oct 5, 2019
Copy link
Contributor

@nathany nathany left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

Let's just ensure that tests are passing. May need a rebase.

vladlosev
vladlosev previously approved these changes Oct 6, 2019
Copy link

@vladlosev vladlosev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some cleanup note, otherwise LGTM.

inotify_test.go Show resolved Hide resolved
@kolyshkin
Copy link
Contributor Author

Rebased; addressed @vladlosev review comment.

@nathany PTAL

@kolyshkin
Copy link
Contributor Author

CI failures look legit but unrelated

@kolyshkin
Copy link
Contributor Author

@nathany PTAL

@amitsingh21
Copy link

@kolyshkin could you please rebase with master, above CI failure should be fixed with this 7f4cf4d
waiting to get this merged to master, as we need this fix

@alisson276
Copy link

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 rm /var/log/x.log didn't recreate the file.

@kolyshkin
Copy link
Contributor Author

Rebased

@nxadm
Copy link

nxadm commented Feb 6, 2021

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.

@ShansOwn
Copy link

Looks like the CI problem was resolved in #378 and #381.
Can this one be moved forward?

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>
Copy link
Member

@arp242 arp242 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing Remove Event when file handle open before for Linux
8 participants