On Fri 06-05-22 03:00:58, Amir Goldstein wrote: > > > > HOWEVER! look at the way we implemented reporting of FAN_RENAME > > > > (i.e. match_mask). We report_new location only if watching sb or watching > > > > new dir. We did that for a reason because watcher may not have permissions > > > > to read new dir. We could revisit this decision for a privileged group, but will > > > > need to go back reading all the discussions we had about this point to see > > > > if there were other reasons(?). > > > > > > Yeah, this is a good point. We are able to safely report the new parent > > > only if the watching process is able to prove it is able to watch it. > > > Adding even more special cases there would be ugly and error prone I'm > > > afraid. We could certainly make this available only to priviledged > > > notification groups but still it is one more odd corner case and the > > > usecase does not seem to be that big. > > > > Sorry, I'm confused about the conclusion we've drawn here. Are we hard > > up against not extending FAN_RENAME for the sole reason that the > > implementation might be ugly and error prone? > > > > Can we not expose this case exclusively to privileged notification > > groups/watchers? This case seems far simpler than what has already > > been implemented in the FAN_RENAME series, that is as you mentioned, > > trying to safely report the new parent only if the watching process is > > able to prove it is able to watch it. If anything, I would've expected > > the privileged case to be implemented prior to attempting to cover > > whether the super block or target directory is being watched. > > To be fair, that is what the "added complexity" for the privileged use > case looks like: > > /* Report both old and new parent+name if sb watching */ > report_old = report_new = > + !FAN_GROUP_FLAG(group, FANOTIFY_UNPRIV) || > match_mask & (1U << FSNOTIFY_ITER_TYPE_SB); > report_old |= > match_mask & (1U << FSNOTIFY_ITER_TYPE_INODE); > > There is a bit more complexity to replace FSNOTIFY_ITER_TYPE_INODE2 > with FSNOTIFY_ITER_TYPE_DIR1 and FSNOTIFY_ITER_TYPE_DIR1. > > But I understand why Jan is hesitant about increasing the cases for > already highly > specialized code. > > My only argument in favor of this case is that had we though about it before > merging FAN_RENAME we would have probably included it.(?) So I've slept on it and agree that allowing FAN_RENAME on a file with the semantics Matthew wants is consistent with the current design and probably the only sensible meaning we can give to it. I also agree that updating permission checks for reporting parent dirs isn't that big of a headache and maintenance burden going further. I'm still somewhat concerned about how the propagation of two parent directories and then formatting into the event is going to work out (i.e., how many special cases that's going to need) but I'm willing to have a look at the patch. Maybe it won't be as bad as I was afraid :). > > Ah, I really wanted to stay away from watching the super block for all > > FAN_RENAME events. I feel like userspace wearing the pain for such > > cases is suboptimal, as this is something that can effectively be done > > in-kernel. I agree that kernel can do this more efficiently than userspace but the question is how much in terms of code (and thus maintenance overhead) are we willing to spend for this IMO rather specialized feature. The code to build necessary information to pass with the event, dealing with all different types of watches and backends and then formatting it to the event for userspace is complex as hell. Whenever I have to do or review some non-trivial changes to it, my head hurts ;) And the amount of subtle cornercase bugs we were fixing in that code over the years is just a testimony of this. So that's why I'm reluctant to add even small complications to it for something I find relatively specialized use (think for how many userspace programs this feature is going to be useful, I don't think many). Honza -- Jan Kara <jack@xxxxxxxx> SUSE Labs, CR
