[CRIU] [PATCH 2/3] net: allow c/r of empty bridges in the container

[Tycho Andersen]

On Wed, Nov 11, 2015 at 03:49:32PM +0300, Pavel Emelyanov wrote:
> Please, find some comments inline.
> 
> > --- a/mount.c
> > +++ b/mount.c
> > @@ -1357,31 +1357,19 @@ out:
> >  static int dump_empty_fs(struct mount_info *pm)
> >  {
> >  	int fd, ret = -1;
> > -	struct dirent *de;
> > -	DIR *fdir = NULL;
> >  	fd = open_mountpoint(pm);
> >  
> >  	if (fd < 0)
> >  		return -1;
> >  
> > -	fdir = fdopendir(fd);
> > -	if (fdir == NULL) {
> > -		close(fd);
> > +	ret = is_empty_dir(fd);
> > +	close(fd);
> > +	if (ret < 0) {
> > +		pr_err("%s isn't empty\n", pm->fstype->name);
> >  		return -1;
> >  	}
> >  
> > -	while ((de = readdir(fdir))) {
> > -		if (dir_dots(de))
> > -			continue;
> > -
> > -		pr_err("%s isn't empty: %s\n", pm->fstype->name, de->d_name);
> > -		goto out;
> > -	}
> > -
> > -	ret = 0;
> > -out:
> > -	closedir(fdir);
> > -	return ret;
> > +	return ret ? 0 : -1;
> 
> You return "OK, let's dump it" when dir emptiness check fails, while
> before the patch error resulted in error.

if is_empty_dir() returns 0 (i.e. the dir is not empty), this returns
-1 doesn't it?

Perhaps there are too many negations in there, I can rearrange it to
be clearer somehow in the next version? I don't have any good ideas,
though.

> >  }
> >  
> >  /*
> > diff --git a/net.c b/net.c
> > index f68a2d8..22f7133 100644
> > --- a/net.c
> > +++ b/net.c
> > @@ -225,6 +225,39 @@ static int dump_unknown_device(struct ifinfomsg *ifi, char *kind,
> >  	return -1;
> >  }
> >  
> > +static int dump_bridge(NetDeviceEntry *nde, struct cr_imgset *imgset) {
> 
> Brace should be on the new line (coding style).

Whoops :)

> > +	char spath[64];
> > +	int ret, fd;
> > +
> > +	ret = sprintf(spath, "class/net/%s/brif", nde->name);
> 
> Shouldn't it be snprintf for such a short spath?
> 
> > +	if (ret < 0 || ret >= sizeof(spath))
> > +		return -1;
> 
> Or even better -- the name of a netdevice cannot exceed IFNAMSIZ constant,
> so you can always upper estimate the size of this buffer.

Yep, sounds good.

> > +
> > +	/* Let's only allow dumping empty bridges for now. To do a full bridge
> > +	 * restore, we need to make sure the bridge and slaves are restored in
> > +	 * the right order and attached correctly. It looks like the veth code
> > +	 * supports this, but we need some way to do ordering.
> > +	 */
> > +	fd = openat(ns_sysfs_fd, spath, O_DIRECTORY, 0);
> > +	if (fd < 0) {
> > +		pr_perror("opening %s failed", spath);
> > +		return -1;
> > +	}
> 
> The fd is leaked even on success path.

:( thanks.

Tycho