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

[Cyrill Gorcunov]

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

...
> > +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)
> > +			break;
> 
> if (ret < 0)
> 	pr_perror(...);

It's already here, if errno != EAGAIN.

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