[CRIU] [PATCH v3 04/16] autofs: dump stage introduced

[Stanislav Kinsburskiy]

14.12.2015 12:45, Pavel Emelyanov пишет:
> On 12/14/2015 02:12 PM, Stanislav Kinsburskiy wrote:
>
>>>> +
>>>> +	return ret;
>>>> +}
>>>> +
>>>> +static int autofs_fixup_options(struct mount_info *pm, int pgrp, bool catatonic,
>>>> +				int kernel_fd, bool kernel_fd_alive,
>>>> +				int read_fd)
>>>> +{
>>>> +	char **options;
>>>> +	int nr_opts, i, err = -1;
>>>> +	char *new_opt;
>>>> +	struct mount_info *t;
>>>> +
>>>> +	new_opt = construct_string("pgrp=%d,fd=%d", pgrp, kernel_fd);
>>>> +	if (!new_opt)
>>>> +		return -1;
>>>> +
>>>> +	if (!catatonic && !kernel_fd_alive) {
>>>> +		/* Write end is closed or invalid
>>>> +		 * Let's introduce new options only for us to carry pipe
>>>> +		 * information. */
>>> So, do I get it right, that the only reason for doing the complex write-end
>>> discovery is to decide whether or not to keep read_fd=%d in the image?
>> I don't understand. Write end is known and just checked for presence and
>> validity.
>> Discovery is done for read end. And we do it for two purposes:
>> 1) Check, that it's empty
>> 2) Dump, if write end is closed.
> I'm talking about the kernel_fd_alive variable. It's set to true after calling
> the autofs_kernel_pipe_alive() and is only used here, to check whether or not
> to keep the read_fd=%d option in the image or not. Also, the only thing for
> which the autofs_kernel_pipe_alive() is used is to set the kernel_fd_alive bool.
>
> So my question is -- why not keep the read_fd=%d option always and remove the
> whole autofs_kernel_pipe_alive() routine?

Because "read_fd" option is used on restore stage to device whether or 
not temporary write end must be created.
In case of this option is absent, only artificial file is created to be 
used to fix up mount point.
Otherwise there must be added write end of the pipe to file descriptors, 
which must be closed after mount is fixed.

>>>> +		new_opt = add_to_string(new_opt, ",read_fd=%d", read_fd);
>>> If you need to keep some fs-specific data in the image that is not going to
>>> be passed as-is into the mount() call, just introduce the necessary optional
>>> fields in the mnt proto file.
>> This mnt proto file is generic.
>> Do you want me to add some autofs-specific fild to it (which can be
>> unset, btw, thus I need another boolean flag)?
> Well, this can be done in two ways. The first is to add optional field into the
> mnt entry. The second is to introduce one more image file (like for tmpfs).
>
> Keeping this in options string is also an option, but if this particular
> option is never passed into mount() system call and you parse this string
> anyway, I propose to save the cpu cycles and keep the desired data out of
> this string.

It will make code more complex, and, frankly, I don't see any 
significant reason for this.
But I can do it, if you insist.

>>>> +	}
>>>> +	if (!new_opt)
>>>> +		return -1;
>>>> +
>>>> +	split(pm->options, ',', &options, &nr_opts);
>>>> +	if (!options)
>>>> +		return -1;
>>>> +
>>>> +	for (i = 0; i < nr_opts; i++) {
>>>> +		char *opt = options[i];
>>>> +
>>>> +		if (!strncmp(opt, "pgrp=", strlen("pgrp=")) ||
>>>> +		    !strncmp(opt, "fd=", strlen("fd=")) ||
>>>> +		    !strncmp(opt, "pipe_ino=", strlen("pipe_ino=")))
>>>> +			continue;
>>>> +
>>>> +		new_opt = add_to_string(new_opt, ",%s", opt);
>>>> +		if (!new_opt)
>>>> +			goto out;
>>>> +	}
>>>> +
>>>> +	xfree(pm->options);
>>>> +	pm->options = new_opt;
>>>> +
>>>> +	/* Change options for all bind-mounts */
>>> What for?
>> To make sure, that mount will be called with the options I fixed, and
>> not with original ones (like it can be, because actual mount point to
>> create is selected randomly).
> But the bind mounts will be called like this
>
>        mount(foo, bar, NULL, MS_BIND, NULL);
>
> without any knowledge about the options.
>

Yes. BUt we don't know, which mount point will be selected ad the one to 
restore.
Frankly, it's possibly determined somehow. But in my experiments mount 
with fixed options was always one of the bind-mount and not the the one 
to create.