[CRIU] [PATCH 2/6] mnt: add --ext-mount-map auto option

[Pavel Emelyanov]

> @@ -641,7 +645,92 @@ static int validate_mounts(struct mount_info *info, bool for_dump)
>  	return 0;
>  }
>  
> -static int collect_shared(struct mount_info *info)
> +static int resolve_external_mounts(struct mount_info *info)
> +{
> +	struct mount_info *m;
> +
> +	for (m = info; m; m = m->next) {
> +		struct mount_info *pm;
> +		bool found = false;
> +		struct ns_id *ns = NULL, *iter;
> +		int ret;
> +
> +		if (m->parent == NULL || m->is_ns_root)
> +			continue;

Before the patch we only resolved external for !fsroot_mounted() mi-s. Shouldn't
the same check be here too?

> +		ret = try_resolve_ext_mount(m);
> +		if (ret < 0 && ret != -ENOTSUP) {
> +			return -1;
> +		} else if (ret == -ENOTSUP && !opts.autodetect_ext_mounts) {
> +			continue;
> +		} else if (ret == 0) {
> +			continue;
> +		}
> +
> +		for (iter = ns_ids; iter->next; iter = iter->next) {
> +			if (iter->pid == getpid() && iter->nd == &mnt_ns_desc) {
> +				ns = iter;
> +				break;
> +			}
> +		}
> +
> +		if (!ns) {
> +			pr_err("Failed to find criu pid's mount ns!");
> +			return -1;
> +		}
> +
> +		for (pm = ns->mnt.mntinfo_list; pm; pm = pm->next) {
> +			int size, ret;
> +			char *p;
> +			bool match = false;
> +			struct ext_mount *em;
> +
> +			if (mounts_equal(pm, m, true))
> +				match = true;
> +
> +			if (!match)
> +				continue;

The bool match seems to be excessive. The

  if (!mounts_equal())
    continue;

would be enough :)

> +
> +			size = strlen(pm->mountpoint + 1) + strlen(m->root) + 1;
> +			p = xmalloc(sizeof(char) * size);
> +			if (!p)
> +				return -1;
> +
> +			ret = snprintf(p, size+1, "%s%s", pm->mountpoint + 1, m->root);
> +			if (ret < 0 || ret >= size) {
> +				free(p);
> +				return -1;
> +			}
> +
> +			em = xmalloc(sizeof(struct ext_mount));
> +			if (!em) {
> +				free(p);
> +				return -1;
> +			}
> +
> +			em->val = "CRIU:AUTOGENERATED";
> +			em->key = p;
> +			m->external = em;
> +
> +			pr_info("autodetected external mount %s for %s\n", p, m->mountpoint);
> +			xfree(m->source);
> +			m->source = p;
> +
> +			found = true;
> +			break;
> +		}
> +
> +		if (!found) {
> +			pr_info("couldn't find external bind mount for %d %s "
> +				"(master_id: %d shared_id: %d)\n",
> +				m->mnt_id, m->mountpoint, m->master_id, m->shared_id);
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int collect_shared(struct mount_info *info, bool for_dump)
>  {
>  	struct mount_info *m, *t;
>  
> @@ -675,10 +764,12 @@ static int collect_shared(struct mount_info *info)
>  			}
>  		}
>  
> -		if (need_master && m->parent) {
> -			pr_err("Mount %d (master_id: %d shared_id: %d) "
> -			       "has unreachable sharing\n", m->mnt_id,
> -				m->master_id, m->shared_id);
> +		// If we haven't already determined this mount is external,
> +		// then we don't know where it came from.
> +		if (need_master && m->parent && !m->external) {
> +			pr_err("Mount %d %s (master_id: %d shared_id: %d) "
> +			       "has unreachable sharing. Try --enable-external-masters.\n", m->mnt_id,
> +				m->mountpoint, m->master_id, m->shared_id);
>  			return -1;
>  		}
>  
> @@ -1594,6 +1685,9 @@ static bool can_mount_now(struct mount_info *mi)
>  	if (mi->is_ns_root)
>  		return true;
>  
> +	if (mi->external)
> +		return true;
> +
>  	if (mi->master_id && mi->bind == NULL)
>  		return false;
>  
> @@ -1880,6 +1974,11 @@ static int collect_mnt_from_image(struct mount_info **pms, struct ns_id *nsid)
>  		pm->need_plugin		= me->with_plugin;
>  		pm->is_ns_root		= is_root(me->mountpoint);
>  
> +		pr_debug("\t\tGetting source for %d\n", pm->mnt_id);
> +		pm->source = xstrdup(me->source);
> +		if (!pm->source)
> +			goto err;
> +
>  		/* FIXME: abort unsupported early */
>  		pm->fstype		= decode_fstype(me->fstype);
>  
> @@ -1893,8 +1992,29 @@ static int collect_mnt_from_image(struct mount_info **pms, struct ns_id *nsid)
>  
>  			em = ext_mount_lookup(me->root);
>  			if (!em) {
> -				pr_err("No mapping for %s mountpoint\n", me->mountpoint);
> -				goto err;\
> +				if (!opts.autodetect_ext_mounts) {
> +					pr_err("No mapping for %s mountpoint\n", me->mountpoint);
> +					goto err;
> +				}
> +
> +				/*
> +				 * Make up an external mount entry for this
> +				 * mount point, since we couldn't find a user
> +				 * supplied one.
> +				 */
> +				em = xmalloc(sizeof(struct ext_mount));
> +				if (!em)
> +					goto err;
> +
> +				em->val = pm->source;
> +
> +				/*
> +				 * Put a : in here since those are invalid on
> +				 * the cli, so we know it's autogenerated in
> +				 * debugging.
> +				 */
> +				em->key = "CRIU:AUTOGENERATED";
> +				list_add_tail(&em->l, &opts.ext_mounts);
>  			}
>  
>  			pm->external = em;
> @@ -1926,11 +2046,6 @@ static int collect_mnt_from_image(struct mount_info **pms, struct ns_id *nsid)
>  
>  		pr_debug("\t\tGetting mpt for %d %s\n", pm->mnt_id, pm->mountpoint);
>  
> -		pr_debug("\t\tGetting source for %d\n", pm->mnt_id);
> -		pm->source = xstrdup(me->source);
> -		if (!pm->source)
> -			goto err;
> -
>  		pr_debug("\t\tGetting opts for %d\n", pm->mnt_id);
>  		pm->options = xstrdup(me->options);
>  		if (!pm->options)
> @@ -2108,7 +2223,7 @@ static int populate_mnt_ns(struct mount_info *mis)
>  	if (!pms)
>  		return -1;
>  
> -	if (collect_shared(mis))
> +	if (collect_shared(mis, false))
>  		return -1;
>  
>  	for (nsid = ns_ids; nsid; nsid = nsid->next) {
> @@ -2346,6 +2461,7 @@ int mntns_get_root_by_mnt_id(int mnt_id)
>  struct collect_mntns_arg {
>  	bool need_to_validate;
>  	bool for_dump;
> +	bool really_collect_self_mounts;
>  };
>  
>  static int collect_mntns(struct ns_id *ns, void *__arg)
> @@ -2360,7 +2476,8 @@ static int collect_mntns(struct ns_id *ns, void *__arg)
>  	if (arg->for_dump && ns->pid != getpid())
>  		arg->need_to_validate = true;
>  
> -	mntinfo_add_list(pms);
> +	if (!opts.autodetect_ext_mounts || ns->pid != getpid() || arg->really_collect_self_mounts)
> +		mntinfo_add_list(pms);

This if and the if for really_collect_self_mounts seems strange to me.

There are two "types" of pms we can get here -- criu's one and non-criu's
one. The former one should only be put into the global list if it's the only
one at all, i.e. -- we're having the non-ns case.

So the single if (ns->pid != getpid() || !(root_ns_mask & CLONE_NEWNS)) here
should give us what we want.

>  	return 0;
>  }
>  
> @@ -2371,15 +2488,34 @@ int collect_mnt_namespaces(bool for_dump)
>  
>  	arg.for_dump = for_dump;
>  	arg.need_to_validate = false;
> +	arg.really_collect_self_mounts = false;
>  
> -	ret = walk_namespaces(&mnt_ns_desc, false, collect_mntns, &need_to_validate);
> +	/*
> +	 * XXX: this is a little bit of spaghetti. If we are autodetecting
> +	 * external mounts, we want to collect the criu pid's mountinfo (and
> +	 * thus the whole mountns) so we can corrolate the external masters.
> +	 * So we clear the mountns bit for this call to walk_namespaces.
> +	 *
> +	 * However, we don't want to add criu's mountns to the dump target
> +	 * list if it isn't really going to be dumped, so we set the
> +	 * really_collect_self_mounts flag if we really do want to collect
> +	 * them above.
> +	 */
> +	if (opts.autodetect_ext_mounts && !(root_ns_mask & CLONE_NEWNS)) {
> +		arg.really_collect_self_mounts = true;
> +	}

If I'm correct with the above only the above if can be removed.

> +
> +	ret = walk_namespaces(&mnt_ns_desc, opts.autodetect_ext_mounts, collect_mntns, &arg);
>  	if (ret)
>  		goto err;
>  
> +	if (resolve_external_mounts(mntinfo))
> +		goto err;
> +
>  	if (arg.need_to_validate) {
>  		ret = -1;
>  
> -		if (collect_shared(mntinfo))
> +		if (collect_shared(mntinfo, true))
>  			goto err;
>  		if (validate_mounts(mntinfo, true))
>  			goto err;
>