[CRIU] [PATCH 2/2] introduce --skip-mnt cli option

[Pavel Emelyanov]

On 04/01/2015 04:07 PM, Oleg Nesterov wrote:
> On 04/01, Pavel Emelyanov wrote:
>>
>> On 03/31/2015 07:24 PM, Oleg Nesterov wrote:
>>
>>> @@ -1004,6 +1035,13 @@ struct mount_info *parse_mountinfo(pid_t pid, struct ns_id *nsid, bool for_dump)
>>>  			goto end;
>>>  		}
>>>
>>> +		if (for_dump && should_skip_mount(new->mountpoint + 1)) {
>>
>> This check is not enough. If we have a file opened on skipped mountpoint, then
>> dump will succeed, but we will fail on restore.
> 
> I thought about this case. dump_one_reg_file()->lookup_nsid_by_mnt_id()
> should fail correctly.

:) Indeed

> Let me test... yes,
> 
> 	$ perl -e 'close STDOUT; close STDERR; sleep' < /boot/System.map
> 
> and "dump --skip-mnt /boot" fails with "Unable to look up the %d mount".
> 
> Hmm. but I think this error message deserves another cosmetic cleanup.

Yes. Log messages in CRIU are not self-descriptive.

> Perhaps it should print link->name? Otherwise it is absolutely not clear
> what this message actually means.

Well yes, adding file name would help but it will still be unclear what
to do in this case.

I have a general ... food for a thought -- if something fails on dump, what
should we tell into logs to help user understand what's going on? I admit
there's no universal answer to this question.

> But,
> 
>> Such behavior is actually not
>> good, restore shouldn't fail due to badly created images :)
> 
> Sure ;) And probably this option can cause such kind of problem. But again,
> the user should know what he does or do not use this option. This is like
> --enable-fs, use at your own risk.

Actually if --skip-mnt succeeds the dump and fails the restore, then we
will have to fix the dump (to fail).

>> Other than this I have no objections against the feature.
> 
> So what do you think I should change in this patch?

After explaining why opened files dump fails only two things, one is
the "usage" string (you mention it) and the comment in mount.c why
just dropping the mount is OK.

> I forgot about the "Usage:" string, it should include the new option...
> 
> Anything else? Apart from RPC which I didn't look at yet, will try to
> do later.

Thanks :)

-- Pavel