[CRIU] [PATCH] Add inherit fd support

[Saied Kazemi]

On Tue, Dec 9, 2014 at 2:50 AM, Andrew Vagin <avagin at gmail.com> wrote:
> On Mon, Dec 08, 2014 at 08:02:40PM -0800, Saied Kazemi wrote:
>> There are cases where a process's file descriptor cannot be restored
>> from the checkpoint images.  For example, a pipe file descriptor with
>> one end in the checkpointed process and the other end in a separate
>> process (that was not part of the checkpointed process tree) cannot be
>> restored because after checkpoint the pipe will be broken.
>>
>> There are also cases where the user wants to use a new file during
>> restore instead of the original file at checkpoint time.  For example,
>> the user wants to change the log file of a process from /path/to/oldlog
>> to /path/to/newlog.
>>
>> In these cases, criu's caller should set up a new file descriptor to be
>> inherited by the restored process and specify the file descriptor with the
>> --inherit-fd command line option.  The argument of --inherit-fd has the
>> format fd[%d]:%s, where %d tells criu which of its own file descriptors
>> to use for restoring the file identified by %s.
>>
>> As a debugging aid, if the argument has the format debug[%d]:%s, it tells
>> criu to write out the string after colon to the file descriptor %d.  This
>> can be used, for example, as an easy way to leave a "restore marker"
>> in the output stream of the process.
>>
>> It's important to note that inherit fd support breaks applications
>> that depend on the state of the file descriptor being inherited.  So,
>> consider inherit fd only for specific use cases that you know for sure
>> won't break the application.
>>
>> For examples please visit http://criu.org/Category:HOWTO.
>
> I don't have objection to commit this patch. Thank you for the work.
>
> Acked-by: Saied Kazemi <saied at google.com>
>
> Pls, look at an inline comment
>
>>
>> Signed-off-by: Saied Kazemi <saied at google.com>
>> ---
>>  cr-restore.c         |   6 ++
>>  crtools.c            |  15 ++++
>>  files-reg.c          |  14 +++
>>  files.c              | 241 ++++++++++++++++++++++++++++++++++++++++++++++++++-
>>  include/cr_options.h |   1 +
>>  include/files.h      |   7 ++
>>  pipes.c              |  33 +++++++
>>  util.c               |   5 ++
>>  8 files changed, 321 insertions(+), 1 deletion(-)
>>
> ...
>> +
>> +/*
>> + * Close all inherit fds.
>> + */
>> +int inherit_fd_fini()
>> +{
>> +     int ov;
>> +     struct inherit_fd *inh;
>> +
>> +     list_for_each_entry(inh, &opts.inherit_fds, inh_list) {
>> +             if (inh->inh_fd < 0) {
>> +                     pr_err("File %s in inherit fd list has invalid fd %d\n",
>> +                             inh->inh_id, inh->inh_fd);
>> +                     return -1;
>> +             }
>> +
>> +             ov = inherit_fd_overwritten(inh);
>
> I don't understand this moment. Could you show an example when a file
> descriptor can be overwritten?

Some parts of the file restore engine close an fd (via close() or
dup()) and reuse it.  To avoid adding a ton of code in these parts to
check for clashes and moving inherit fds, we just make sure that when
we're wrapping up we won't accidentally close an inherit fd that has
been reused.

One place that I see this in my experiments is send_fd_to_self().
Adding a check for clashes there will prevent the fd from being
overwritten.  But to be conservative and have more protection against
different use cases, we should make sure that we won't close inherit
fds that have been reused.

I can send my small changes either as a separate patch on top of this
one or I can amend the existing patch and send again.  I think it's
better if I amend, but what you prefer?


>> +             if (ov < 0)
>> +                     return -1;
>> +
>> +             if (!ov) {
>> +                     pr_debug("Closing inherit fd %d -> %s\n", inh->inh_fd,
>> +                             inh->inh_id);
>> +                     if (close_safe(&inh->inh_fd) < 0)
>> +                             return -1;
>> +             }
>> +     }
>> +     return 0;
>> +}
> ...