Skip to content

Do not suppress Chmod on non-existent file#260

Merged
arp242 merged 1 commit intofsnotify:mainfrom
kolyshkin:delete-opened-file
Jul 21, 2022
Merged

Do not suppress Chmod on non-existent file#260
arp242 merged 1 commit intofsnotify:mainfrom
kolyshkin:delete-opened-file

Conversation

@kolyshkin
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

kolyshkin commented Aug 31, 2018

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

@kolyshkin
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown

@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.

@kolyshkin kolyshkin dismissed stale reviews from vladlosev and nathany via 5e53cc7 March 2, 2020 23:15
@kolyshkin kolyshkin force-pushed the delete-opened-file branch from 211a9dd to 5e53cc7 Compare March 2, 2020 23:15
@kolyshkin
Copy link
Copy Markdown
Contributor Author

Rebased; addressed @vladlosev review comment.

@nathany PTAL

@kolyshkin
Copy link
Copy Markdown
Contributor Author

CI failures look legit but unrelated

@kolyshkin
Copy link
Copy Markdown
Contributor Author

@nathany PTAL

@amitsingh21
Copy link
Copy Markdown

@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
Copy Markdown

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
Copy Markdown
Contributor Author

Rebased

@nxadm
Copy link
Copy Markdown

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
Copy Markdown

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>
@arp242 arp242 force-pushed the delete-opened-file branch from 5bc399e to 53832bc Compare July 21, 2022 10:24
Copy link
Copy Markdown
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