[CRIU] [PATCH 3/3] fsnotify: inotify -- Add flushing of criu generated events

[Andrew Vagin]

On Wed, Sep 03, 2014 at 12:15:17PM +0400, Cyrill Gorcunov wrote:
> On Wed, Sep 03, 2014 at 11:56:12AM +0400, Andrew Vagin wrote:
> > On Tue, Sep 02, 2014 at 11:27:21PM +0400, Cyrill Gorcunov wrote:
> > > When we restore inotify events we generate own events. So flush them before
> > > restored application get control back, otherwise the application may get
> > > confused because these events has nothing to do with original application
> > > logic.
> > > 
> > > Note at the moment we call the inotify_flush_events without testing
> > > if the descriptor is created with O_NONBLOCK, that's because we know
> > > that we're generating events.
> > 
> > but you call read() in a look, so you will block or I don't understand smth
> 
> No, you are correct, we can block, but as a first iteration (you remember that
> we've a few more issues in fsnotify code) this version will work for our test

It will work, because both of our tests create inotify with the IN_NONBLOCK flag.

> cases. As I said in first email, I'll send fixes on top, once the rest of
> issues are resolved. If it cause concerns -- lets drop this patch for a while.

Yes, it cause.

> 
> ...
> > > +static int inotify_flush_events(int inotify_fd, struct fsnotify_file_info *info)
> > > +{
> > > +	struct inotify_event *event;
> > > +	char buf[sizeof(*event) * 8];
> > > +	int ret, off;
> > > +
> > > +	/*
> > > +	 * NOTE: It's up to a caller to make @inotify_fd being
> > > +	 * created with O_NONBLOCK if needed.
> > 
> > I don't understand this comment. I think we need to say why we can't
> > block here.
> 
> The comments says exactly the opposite -- we _might_ block here, thus it's
> up to a caller to the function to make sure we won't block.
> 
> > 
> > > +	 */
> > > +	while (1) {
> > > +		ret = read(inotify_fd, buf, sizeof(buf));
> > 
> > You will block here, if inotify_fd isn't in non-block state.
> > 
> > > +		if (ret < 0) {
> > > +			if (errno != EAGAIN) {
> > > +				pr_perror("Can't read inotify queue for 0x%08x",
> > > +					  info->ife->id);
> > > +				return -1;
> > > +			} else
> > > +				return 0;
> > > +		} else if (ret <= 0)

ret can't be negative here.

		} else if (ret == 0)

> > > +			break;
> > 
> > if (ret < 0)
> > 	pr_perror(...);
> 
> It's already here, if errno != EAGAIN.

I have missed this. Thanks.

> 
> > >  static int restore_one_inotify(int inotify_fd, struct fsnotify_mark_info *info)
> > >  {
> > >  	InotifyWdEntry *iwe = info->iwe;
> > > @@ -539,6 +619,14 @@ static int open_inotify_fd(struct file_desc *d)
> > >  	if (restore_fown(tmp, info->ife->fown))
> > >  		close_safe(&tmp);
> > >  
> > > +	if (tmp >= 0) {
> > 
> > I don't like this game with close_safe(&tmp). Can we add a error label
> > and use goto on error paths?
> 
> Sure.