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

[Pavel Emelyanov]

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.

>  }
>  
>  /*
> 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).

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

> +
> +	/* 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.

> +
> +	ret = is_empty_dir(fd);
> +	if (ret < 0) {
> +		pr_perror("problem testing %s for emptiness", spath);
> +		return -1;
> +	}
> +
> +	if (!ret) {
> +		pr_err("dumping bridges with attached slaves not supported currently\n");
> +		return -1;
> +	}
> +
> +	return write_netdev_img(nde, imgset);
> +}
> +
>  static int dump_one_ethernet(struct ifinfomsg *ifi, char *kind,
>  		struct rtattr **tb, struct cr_imgset *fds)
>  {

-- Pavel