[CRIU] Re: [PATCH 6/8] inotify: Add checkpoint/restore

[Pavel Emelyanov]

Almost perfect.

> +struct inotify_wd_entry {
> +	u32	id;
> +	u64	i_ino;
> +	u32	mask;
> +	u32	s_dev;
> +	u32	r_dev;

Unused.

> +	u32	wd;
> +	fh_t	f_handle;
> +} __packed;
> +

> +		if (ret) {
> +			switch (errno) {
> +			case ENODATA:
> +				goto next;
> +			case ENOENT:
> +				goto finish;
> +			case ENOMEM:
> +			default:
> +				pr_perror("Fetching inotify failed %d", -errno);
> +				goto err;
> +			}

			if (errno == ENODATA)
				continue;
			if (errno == ENOENT)
				break;

			pr_perror();
			goto err;

> +		}

> +	mntpath = get_mnt_root(iwe->s_dev, mntinfo, nr_mntinfo);
> +	mntfd = open(mntpath, O_RDONLY);
> +	if (mntfd < 0) {
> +		pr_err("Mount root for %x not found\n", iwe->s_dev);
> +		return -1;
> +	}

I'd make get_mnt_root() return an fd.

> +	nr_mntinfo = parse_mountinfo(getpid(), mntinfo, nr_mntinfo);
> +	if (nr_mntinfo < 1) {
> +		pr_err("Parsing mountinfo %d failed\n", getpid());
> +		return -1;
> +	}
> +
> +	image_fd = open_image_ro(CR_FD_INOTIFY);
> +	if (image_fd < 0)
> +		return -1;
> +

This has nothing do to with inotifies only. Plz, move the whole mntinfo init into
mount.c and call it from cr-restore.c.

> +	info = xmalloc(sizeof(*info) * nr_infos);

These are not shared, why not allocate infos one-by-one adding them to list?

> +static int cmp(const void *a, const void *b)
> +{
> +	const struct inotify_file_info *x = a;
> +	const struct inotify_file_info *y = b;
> +
> +	return (y->ife.id - x->ife.id);
> +}

What for? To speedup the subsequent search? Oh, the amount of cpu cycles it saved is
negligible in comparison to the amount of brain cycles used to understand what is
going on.

Allocate info-s one by one and store them in a sort-on-insert manner. This will be the
same from O(.) POV but will be MUCH more readable.

> +		idx = bsi(mark->iwe.id, info, i + 1);

Ugly name of a function, but it will go away.

> +	for (j = 0; j < i; j++) {
> +		pr_info("Collected inotify: id %8x flags %8x\n", info[j].ife.id, info[j].ife.flags);
> +		file_desc_add(&info[j].d, FDINFO_INOTIFY, info[j].ife.id, &desc_ops);
> +	}

Add them once you allocate the info in the reworked patch.

Thanks,
Pavel