[CRIU] [PATCH] Extend the parser to accept negative options

Kir Kolyshkin kir at openvz.org
Wed Mar 29 17:17:25 PDT 2017


On 03/29/2017 05:10 PM, Dmitry Safonov wrote:
> 2017-03-30 2:51 GMT+03:00 Kir Kolyshkin <kir at openvz.org>:
>> On 03/28/2017 12:24 PM, Veronika Kabatova wrote:
>>> Introducing negative options for true / false values. The original
>>> getopt_long parser is kept since it is able to set flag-like values.
>>> The type of the options needed to be changed to integers for getopt_long
>>> to accept flag-like value settings (as per getopt_long documentation,
>>> the address of integer variable has to be passed).
>>>
>>> Corresponding negative options are not added for deprecated options.
>>>
>>> This patch is a preparation for the addition of configuration files
>>> (GitHub issue #278). General idea of this feature is to have global
>>> configuration files in /etc/criu.d/ directory and user-specific
>>> configuration files in $HOME/.criu.d/ directory, with the possibility
>>> of specifying a chosen file to be used (default files will be used if
>>> none is specified, or none in case the default ones are not present,
>>> to not break compatibility). The options in configuration files should
>>> be possible to be overriden by the options specified on command line,
>>> hence the negative options addition.
>>>
>>> The whole feature of configuration files will remove the need of
>>> specifying all the options on command line, with the possibility of
>>> reusing a file for different use case with only overriding some of the
>>> values specified there.
>>>
>>> In case both types of option (negative and positive) are passed, the
>>> later one will be applied -- this works with the philosophy of
>>> overriding the "earlier" options from configuration files.
>>>
>>> Signed-off-by: Veronika Kabatova <vkabatov at redhat.com>
>>> ---
>>>    Documentation/criu.txt    | 110 +++++++++++-----------
>>>    criu/crtools.c            | 231
>>> +++++++++++++++++++++-------------------------
>>>    criu/include/cr_options.h |  42 ++++-----
>>>    criu/include/file-lock.h  |   3 +-
>>>    criu/include/files.h      |   1 +
>>>    criu/include/sk-inet.h    |   6 +-
>>>    criu/include/tty.h        |   3 +-
>>>    7 files changed, 191 insertions(+), 205 deletions(-)
>>>
>>> diff --git a/Documentation/criu.txt b/Documentation/criu.txt
>>> index 1d64f7f..17bd1fa 100644
>>> --- a/Documentation/criu.txt
>>> +++ b/Documentation/criu.txt
>>> @@ -54,17 +54,18 @@ The following levels are available:
>>>    *-o*, *--log-file* 'file'::
>>>        Write logging messages to 'file'.
>>>    -*--log-pid*::
>>> -    Write separate logging files per each pid.
>>> +*--[no-]log-pid*::
>>> +    [Don't] write separate logging files per each pid.
>>>    -*--display-stats*::
>>> +*--[no-]display-stats*::
>>>        During dump as well as during restore *criu* collects information
>>>        like the time required to dump or restore the process or the
>>>        number of pages dumped or restored. This information is always
>>>        written to the files 'stats-dump' and 'stats-restore' and can
>>>        be easily displayed using *crit*. The option *--display-stats*
>>>        additionally prints out this information on the console at the end
>>> -    of a dump or a restore.
>>> +    of a dump or a restore, using *--no-display-stats* disables this in
>>
>> s/,/;/
>>
>>> +    case it was previously enabled.
>>>      *-D*, *--images-dir* 'path'::
>>>        Use 'path' as a base directory where to look for sets of image
>>> files.
>>> @@ -131,9 +132,9 @@ memory changes since the previous *pre-dump*. Note
>>> that during this
>>>    procedure. *pre-dump* requires at least *-t* option (see *dump* below).
>>>    In addition, *page-server* options may be specified.
>>>    -*--track-mem*::
>>> -    Turn on memory changes tracker in the kernel. If the option is
>>> -    not passed the memory tracker get turned on implicitly.
>>> +*--[no-]track-mem*::
>>> +    Turn on [off if enabled] memory changes tracker in the kernel. If no
>>
>> Just say "Turn on/off".
>>
>>
>>> +    option is passed the memory tracker get turned on implicitly.
>>>      *dump*
>>>    ~~~~~~
>>> @@ -250,37 +251,40 @@ For example, the command line for the above example
>>> should look like this:
>>>        a predefined controller property with the new one shipped. If the
>>> option
>>>        is not used, the predefined properties are merged with the provided
>>> ones.
>>>    -*--tcp-established*::
>>> -    Checkpoint established TCP connections.
>>> +*--[no-]tcp-established*::
>>> +    [Don't] checkpoint established TCP connections.
>>>    -*--skip-in-flight*::
>>> +*--[no-]skip-in-flight*::
>>>        This option skips in-flight TCP connections. If any TCP connections
>>>        that are not yet completely established are found, *criu* ignores
>>>        these connections, rather than errors out.
>>>        The TCP stack on the client side is expected to handle the
>>>        re-connect gracefully.
>>> +    The *--no-skip-in-flight* option disables the skipping in case it
>>> +    was previously enabled.
>>>    -*--evasive-devices*::
>>> -    Use any path to a device file if the original one is inaccessible.
>>> +*--[no-]evasive-devices*::
>>> +    [Don't] use any path to a device file if the original one is
>>> inaccessible.
>>>    -*--page-server*::
>>> -    Send pages to a page server (see the *page-server* command).
>>> +*--[no-]page-server*::
>>> +    [Don't] end pages to a page server (see the *page-server* command).
>>
>> s/end/send/
>>
>>
>>>    -*--force-irmap*::
>>> -    Force resolving names for inotify and fsnotify watches.
>>> +*--[no-]force-irmap*::
>>> +    [Don't] force resolving names for inotify and fsnotify watches.
>>>    -*--auto-dedup*::
>>> -    Deduplicate "old" data in pages images of previous *dump*. This
>>> option
>>> -    implies incremental *dump* mode (see the *pre-dump* command).
>>> +*--[no-]auto-dedup*::
>>> +    [Don't] deduplicate "old" data in pages images of previous *dump*.
>>> This
>>> +    option implies incremental *dump* mode (see the *pre-dump* command).
>>>    -*-l*, *--file-locks*::
>>> -    Dump file locks. It is necessary to make sure that all file lock
>>> users
>>> -    are taken into dump, so it is only safe to use this for enclosed
>>> containers
>>> -    where locks are not held by any processes outside of dumped process
>>> tree.
>>> +*-l*, *--[no-]file-locks*::
>>> +    [Don't] dump file locks. It is necessary to make sure that all file
>>> lock
>>> +    users are taken into dump, so it is only safe to use this for
>>> enclosed
>>> +    containers where locks are not held by any processes outside of
>>> dumped
>>> +    process tree.
>>>    -*--link-remap*::
>>> -    Allows to link unlinked files back, if possible (modifies filesystem
>>> -    during *restore*).
>>> +*--[no-]link-remap*::
>>> +    Allows [disallows] to link unlinked files back, if possible (modifies
>>> +    filesystem during *restore*).
>>>      *--ghost-limit* 'size'::
>>>        Set the maximum size of deleted file to be carried inside image.
>>> @@ -289,9 +293,9 @@ For example, the command line for the above example
>>> should look like this:
>>>        'size' may be postfixed with a *K*, *M* or *G*, which stands for
>>> kilo-,
>>>        mega, and gigabytes, accordingly.
>>>    -*-j*, *--shell-job*::
>>> -    Allow one to dump shell jobs. This implies the restored task will
>>> -    inherit session and process group ID from the *criu* itself.
>>> +*-j*, *--[no-]shell-job*::
>>> +    Allow [disallows] one to dump shell jobs. This implies the restored
>>> task
>>> +    will inherit session and process group ID from the *criu* itself.
>>>        This option also allows to migrate a single external tty connection,
>>>        to migrate applications like *top*. If used with *dump* command,
>>>        it must be specified with *restore* as well.
>>> @@ -340,15 +344,15 @@ The 'resource' argument can be one of the following:
>>>    Note that square brackets used in this option arguments are literals and
>>>    usually need to be escaped from shell.
>>>    -*-d*, *--restore-detached*::
>>> -    Detach *criu* itself once restore is complete.
>>> +*-d*, *--[no-]restore-detached*::
>>> +    [Don't] detach *criu* itself once restore is complete.
>>>      *-s*, *--leave-stopped*::
>>>        Leave tasks in stopped state after restore (rather than resuming
>>>        their execution).
>>>    -*-S*, *--restore-sibling*::
>>> -    Restore root task as a sibling (makes sense only with
>>> +*-S*, *--[no-]restore-sibling*::
>>> +    [Don't] restore root task as a sibling (makes sense only with
>>>        *--restore-detached*).
>>>      *-r*, *--root* 'path'::
>>> @@ -412,23 +416,24 @@ The 'mode' may be one of the following:
>>>        Change the root cgroup the controller will be installed into. No
>>> controller
>>>        means that root is the default for all controllers not specified.
>>>    -*--tcp-established*::
>>> -    Restore previously dumped established TCP connections. This implies
>>> that
>>> -    the network has been locked between *dump* and *restore* phases so
>>> other
>>> -    side of a connection simply notice a kind of lag.
>>> +*--[no-]tcp-established*::
>>> +    [Don't] restore previously dumped established TCP connections. This
>>> implies
>>> +    that the network has been locked between *dump* and *restore* phases
>>> so
>>> +    other side of a connection simply notice a kind of lag.
>>>      *--veth-pair* 'IN'*=*'OUT'::
>>>        Correspondence between outside and inside names of veth devices.
>>>    -*-l*, *--file-locks*::
>>> -    Restore file locks from the image.
>>> +*-l*, *--[no-]file-locks*::
>>> +    [Don't] restore file locks from the image.
>>>    -*--auto-dedup*::
>>> -    As soon as a page is restored it get punched out from image.
>>> +*--[no-]auto-dedup*::
>>> +    As soon as a page is restored it gets punched out from image. Use
>>> +    *--no-auto-dedup* to disable this in case it was enabled previously.
>>>    -*-j*, *--shell-job*::
>>> -    Restore shell jobs, in other words inherit session and process group
>>> -    ID from the criu itself.
>>> +*-j*, *--[no-]shell-job*::
>>> +    [Don't] restore shell jobs, in other words inherit session and
>>> process
>>> +    group ID from the criu itself.
>>>      *--cpu-cap* ['cap'[,'cap'...]]::
>>>        Specify CPU capabilities to be present on the CPU the process is
>>> @@ -474,10 +479,11 @@ during *dump*, and images are migrated to a less
>>> capable CPU and are
>>>    to be restored. By default, *criu* shows an error that CPU capabilities
>>>    are not adequate, but this can be suppressed by using *--cpu-cap=none*.
>>>    -*--weak-sysctls*::
>>> +*--[no-]weak-sysctls*::
>>>       Silently skip restoring sysctls that are not available. This allows
>>>       to restore on an older kernel, or a kernel configured without some
>>> -   options.
>>> +   options. Using *--no-weak-sysctls* disables silent skipping in case it
>>> +   was previously enabled.
>>>      *check*
>>>    ~~~~~~~
>>> @@ -513,11 +519,11 @@ Missing Category 2 and 3 features cause *criu* to
>>> print "Looks good but
>>>    Without any options, *criu check* checks Category 1 features. This
>>>    behavior can be changed by using the following options:
>>>    -*--extra*::
>>> -    Check kernel support for Category 2 features.
>>> +*--[no-]extra*::
>>> +    [Don't] check kernel support for Category 2 features.
>>>    -*--experimental*::
>>> -    Check kernel support for Category 3 features.
>>> +*--[no-]experimental*::
>>> +    [Don't] check kernel support for Category 3 features.
>>>      *--all*::
>>>        Check kernel support for Category 1, 2, and 3 features.
>>> @@ -530,8 +536,8 @@ behavior can be changed by using the following
>>> options:
>>>    ~~~~~~~~~~~~~
>>>    Launches *criu* in page server mode.
>>>    -*--daemon*::
>>> -    Runs page server as a daemon (background process).
>>> +*--[no-]daemon*::
>>> +    [Don't] run page server as a daemon (background process).
>>>      *--status_fd*::
>>>        Write \\0 to the FD and close it once page-server is ready to handle
>>
>> I have an idea! Instead of adding and describing "no" to every option,
>> Why don't you say (somewhere at the beginning of OPTIONS)
>> that most of the long binary options can be prefixed with *--no-*
>> to negate the option meaning (example: --track-mem and --no-track-mem).
>>
>>> diff --git a/criu/crtools.c b/criu/crtools.c
>>> index 5a039a8..4af29fa 100644
>>> --- a/criu/crtools.c
>>> +++ b/criu/crtools.c
>>> @@ -226,9 +226,12 @@ int main(int argc, char *argv[], char *envp[])
>>>                  { "pid",                        required_argument,      0,
>>> 'p'  },
>>>                  { "leave-stopped",              no_argument,            0,
>>> 's'  },
>>>                  { "leave-running",              no_argument,            0,
>>> 'R'  },
>>> -               { "restore-detached",           no_argument,            0,
>>> 'd'  },
>>> -               { "restore-sibling",            no_argument,            0,
>>> 'S'  },
>>> -               { "daemon",                     no_argument,            0,
>>> 'd'  },
>>> +               { "restore-detached",           no_argument,
>>> &opts.restore_detach, true      },
>>> +               { "no-restore-detached",        no_argument,
>>> &opts.restore_detach, false     },
>>
>> A simple macro can be written to generate these two lines for
>> every binary option, as the only things different between the first
>> and the second lines are "no-" prefix and true vs false value.
>>
>> Ditto for all the other params modified in a similar way.
>>
>>
>>> +               { "restore-sibling",            no_argument,
>>> &opts.restore_sibling, true     },
>>> +               { "no-restore-sibling",         no_argument,
>>> &opts.restore_sibling, false    },
>>> +               { "daemon",                     no_argument,
>>> &opts.restore_detach, true      },
>>> +               { "no-daemon",                  no_argument,
>>> &opts.restore_detach, false     },
>>>                  { "contents",                   no_argument,            0,
>>> 'c'  },
>>>                  { "file",                       required_argument,      0,
>>> 'f'  },
>>>                  { "fields",                     required_argument,      0,
>>> 'F'  },
>>> @@ -239,27 +242,37 @@ int main(int argc, char *argv[], char *envp[])
>>>                  { "root",                       required_argument,      0,
>>> 'r'  },
>>>                  { USK_EXT_PARAM,                optional_argument,      0,
>>> 'x'  },
>>>                  { "help",                       no_argument,            0,
>>> 'h'  },
>>> -               { SK_EST_PARAM,                 no_argument,            0,
>>> 1042 },
>>> +               { SK_EST_PARAM,                 no_argument,
>>> &opts.tcp_established_ok, true  },
>>> +               { SK_NO_EST_PARAM,              no_argument,
>>> &opts.tcp_established_ok, false },
>>>                  { "close",                      required_argument,      0,
>>> 1043 },
>>> -               { "log-pid",                    no_argument,            0,
>>> 1044 },
>>> +               { "log-pid",                    no_argument,
>>> &opts.log_file_per_pid, true    },
>>> +               { "no-log-pid",                 no_argument,
>>> &opts.log_file_per_pid, false   },
>>>                  { "version",                    no_argument,            0,
>>> 'V'  },
>>> -               { "evasive-devices",            no_argument,            0,
>>> 1045 },
>>> +               { "evasive-devices",            no_argument,
>>> &opts.evasive_devices, true     },
>>> +               { "no-evasive-devices",         no_argument,
>>> &opts.evasive_devices, false    },
>>>                  { "pidfile",                    required_argument,      0,
>>> 1046 },
>>>                  { "veth-pair",                  required_argument,      0,
>>> 1047 },
>>>                  { "action-script",              required_argument,      0,
>>> 1049 },
>>> -               { LREMAP_PARAM,                 no_argument,            0,
>>> 1041 },
>>> -               { OPT_SHELL_JOB,                no_argument,            0,
>>> 'j'  },
>>> -               { OPT_FILE_LOCKS,               no_argument,            0,
>>> 'l'  },
>>> -               { "page-server",                no_argument,            0,
>>> 1050 },
>>> +               { LREMAP_PARAM,                 no_argument,
>>> &opts.link_remap_ok, true       },
>>> +               { NO_LREMAP_PARAM,              no_argument,
>>> &opts.link_remap_ok, false      },
>>
>> The above suggestion will also save you from introducing such additional
>> defines.
>>
>>
>>> +               { OPT_SHELL_JOB,                no_argument,
>>> &opts.shell_job, true           },
>>> +               { OPT_NO_SHELL_JOB,             no_argument,
>>> &opts.shell_job, false          },
>>> +               { OPT_FILE_LOCKS,               no_argument,
>>> &opts.handle_file_locks, true   },
>>> +               { OPT_NO_FILE_LOCKS,            no_argument,
>>> &opts.handle_file_locks, false  },
>>> +               { "page-server",                no_argument,
>>> &opts.use_page_server, true     },
>>> +               { "no-page-server",             no_argument,
>>> &opts.use_page_server, false    },
>>>                  { "address",                    required_argument,      0,
>>> 1051 },
>>>                  { "port",                       required_argument,      0,
>>> 1052 },
>>>                  { "prev-images-dir",            required_argument,      0,
>>> 1053 },
>>>                  { "ms",                         no_argument,            0,
>>> 1054 },
>>> -               { "track-mem",                  no_argument,            0,
>>> 1055 },
>>> -               { "auto-dedup",                 no_argument,            0,
>>> 1056 },
>>> +               { "track-mem",                  no_argument,
>>> &opts.track_mem, true           },
>>> +               { "no-track-mem",               no_argument,
>>> &opts.track_mem, false          },
>>> +               { "auto-dedup",                 no_argument,
>>> &opts.auto_dedup, true          },
>>> +               { "no-auto-dedup",              no_argument,
>>> &opts.auto_dedup, false         },
>>>                  { "libdir",                     required_argument,      0,
>>> 'L'  },
>>>                  { "cpu-cap",                    optional_argument,      0,
>>> 1057 },
>>> -               { "force-irmap",                no_argument,            0,
>>> 1058 },
>>> +               { "force-irmap",                no_argument,
>>> &opts.force_irmap, true         },
>>> +               { "no-force-irmap",             no_argument,
>>> &opts.force_irmap, false        },
>>>                  { "ext-mount-map",              required_argument,      0,
>>> 'M'  },
>>>                  { "exec-cmd",                   no_argument,            0,
>>> 1059 },
>>>                  { "manage-cgroups",             optional_argument,      0,
>>> 1060 },
>>> @@ -268,8 +281,8 @@ int main(int argc, char *argv[], char *envp[])
>>>                  { "feature",                    required_argument,      0,
>>> 1063 },
>>>                  { "skip-mnt",                   required_argument,      0,
>>> 1064 },
>>>                  { "enable-fs",                  required_argument,      0,
>>> 1065 },
>>> -               { "enable-external-sharing",    no_argument,            0,
>>> 1066 },
>>> -               { "enable-external-masters",    no_argument,            0,
>>> 1067 },
>>> +               { "enable-external-sharing",    no_argument,
>>> &opts.enable_external_sharing, true     },
>>> +               { "enable-external-masters",    no_argument,
>>> &opts.enable_external_masters, true     },
>>>                  { "freeze-cgroup",              required_argument,      0,
>>> 1068 },
>>>                  { "ghost-limit",                required_argument,      0,
>>> 1069 },
>>>                  { "irmap-scan-path",            required_argument,      0,
>>> 1070 },
>>> @@ -278,19 +291,26 @@ int main(int argc, char *argv[], char *envp[])
>>>                  { "external",                   required_argument,      0,
>>> 1073 },
>>>                  { "empty-ns",                   required_argument,      0,
>>> 1074 },
>>>                  { "lazy-pages",                 no_argument,            0,
>>> 1076 },
>>> -               { "extra",                      no_argument,            0,
>>> 1077 },
>>> -               { "experimental",               no_argument,            0,
>>> 1078 },
>>> +               { "extra",                      no_argument,
>>> &opts.check_extra_features, true        },
>>> +               { "no-extra",                   no_argument,
>>> &opts.check_extra_features, false       },
>>> +               { "experimental",               no_argument,
>>> &opts.check_experimental_features, true },
>>> +               { "no-experimental",            no_argument,
>>> &opts.check_experimental_features, false},
>>>                  { "all",                        no_argument,            0,
>>> 1079 },
>>>                  { "cgroup-props",               required_argument,      0,
>>> 1080 },
>>>                  { "cgroup-props-file",          required_argument,      0,
>>> 1081 },
>>>                  { "cgroup-dump-controller",     required_argument,      0,
>>> 1082 },
>>> -               { SK_INFLIGHT_PARAM,            no_argument,            0,
>>> 1083 },
>>> -               { "deprecated",                 no_argument,            0,
>>> 1084 },
>>> +               { SK_INFLIGHT_PARAM,            no_argument,
>>> &opts.tcp_skip_in_flight, true  },
>>> +               { SK_NO_INFLIGHT_PARAM,         no_argument,
>>> &opts.tcp_skip_in_flight, false },
>>> +               { "deprecated",                 no_argument,
>>> &opts.deprecated_ok, true       },
>>> +               { "no-deprecated",              no_argument,
>>> &opts.deprecated_ok, false      },
>>>                  { "check-only",                 no_argument,            0,
>>> 1085 },
>>> -               { "display-stats",              no_argument,            0,
>>> 1086 },
>>> -               { "weak-sysctls",               no_argument,            0,
>>> 1087 },
>>> +               { "display-stats",              no_argument,
>>> &opts.display_stats, true       },
>>> +               { "no-display-stats",           no_argument,
>>> &opts.display_stats, false      },
>>> +               { "weak-sysctls",               no_argument,
>>> &opts.weak_sysctls, true        },
>>> +               { "no-weak-sysctls",            no_argument,
>>> &opts.weak_sysctls, false       },
>>>                  { "status-fd",                  required_argument,      0,
>>> 1088 },
>>> -               { "remote",                     no_argument,            0,
>>> 1089 },
>>> +               { "remote",                     no_argument,
>>> &opts.remote, true              },
>>> +               { "no-remote",                  no_argument,
>>> &opts.remote, false             },
>>>                  { },
>>>          };
>>>    @@ -330,6 +350,8 @@ int main(int argc, char *argv[], char *envp[])
>>>                  opt = getopt_long(argc, argv, short_opts, long_opts,
>>> &idx);
>>>                  if (opt == -1)
>>>                          break;
>>> +               if (!opt)
>>> +                       continue;
>>>                  switch (opt) {
>>>                  case 's':
>>> @@ -394,14 +416,6 @@ int main(int argc, char *argv[], char *envp[])
>>>                          } else
>>>                                  log_level++;
>>>                          break;
>>> -               case 1041:
>>> -                       pr_info("Will allow link remaps on FS\n");
>>> -                       opts.link_remap_ok = true;
>>> -                       break;
>>> -               case 1042:
>>> -                       pr_info("Will dump TCP connections\n");
>>> -                       opts.tcp_established_ok = true;
>>> -                       break;
>>>                  case 1043: {
>>>                          int fd;
>>>    @@ -410,12 +424,6 @@ int main(int argc, char *argv[], char *envp[])
>>>                          close(fd);
>>>                          break;
>>>                  }
>>> -               case 1044:
>>> -                       opts.log_file_per_pid = 1;
>>> -                       break;
>>> -               case 1045:
>>> -                       opts.evasive_devices = true;
>>> -                       break;
>>>                  case 1046:
>>>                          opts.pidfile = optarg;
>>>                          break;
>>> @@ -437,9 +445,6 @@ int main(int argc, char *argv[], char *envp[])
>>>                                  return 1;
>>>                          break;
>>> -               case 1050:
>>> -                       opts.use_page_server = true;
>>> -                       break;
>>>                  case 1051:
>>>                          opts.addr = optarg;
>>>                          break;
>>> @@ -457,12 +462,6 @@ int main(int argc, char *argv[], char *envp[])
>>>                  case 1053:
>>>                          opts.img_parent = optarg;
>>>                          break;
>>> -               case 1055:
>>> -                       opts.track_mem = true;
>>> -                       break;
>>> -               case 1056:
>>> -                       opts.auto_dedup = true;
>>> -                       break;
>>>                  case 1057:
>>>                          if (parse_cpu_cap(&opts, optarg))
>>>                                  goto usage;
>>> @@ -520,12 +519,6 @@ int main(int argc, char *argv[], char *envp[])
>>>                          if (!add_fsname_auto(optarg))
>>>                                  return 1;
>>>                          break;
>>> -               case 1066:
>>> -                       opts.enable_external_sharing = true;
>>> -                       break;
>>> -               case 1067:
>>> -                       opts.enable_external_masters = true;
>>> -                       break;
>>>                  case 1068:
>>>                          opts.freeze_cgroup = optarg;
>>>                          break;
>>> @@ -577,12 +570,6 @@ int main(int argc, char *argv[], char *envp[])
>>>                                  return 1;
>>>                          }
>>>                          break;
>>> -               case 1077:
>>> -                       opts.check_extra_features = true;
>>> -                       break;
>>> -               case 1078:
>>> -                       opts.check_experimental_features = true;
>>> -                       break;
>>>                  case 1079:
>>>                          opts.check_extra_features = true;
>>>                          opts.check_experimental_features = true;
>>> @@ -597,35 +584,12 @@ int main(int argc, char *argv[], char *envp[])
>>>                          if (!cgp_add_dump_controller(optarg))
>>>                                  return 1;
>>>                          break;
>>> -               case 1083:
>>> -                       pr_msg("Will skip in-flight TCP connections\n");
>>> -                       opts.tcp_skip_in_flight = true;
>>> -                       break;
>>> -               case 1084:
>>> -                       pr_msg("Turn deprecated stuff ON\n");
>>> -                       opts.deprecated_ok = true;
>>> -                       break;
>>> -               case 1085:
>>> -                       pr_msg("Only checking if requested operation will
>>> succeed\n");
>>> -                       opts.check_only = true;
>>> -                       opts.final_state = TASK_ALIVE;
>>
>> I don't see where are you recreating this removed functionality.
> Oh, that's actually, quite nice part of the patch: instead of setting
> bools inside of opts by the hands, it passes a pointer as option::flag
> to getopt_long(). Though, that could has been described in the
> changelog..

I like this part a lot, too, but I'm talking about something quite 
different here:
setting opts.final_state to TASK_ALIVE in case opts.check_only is set.

>
>>
>>> -                       break;
>>> -               case 1086:
>>> -                       opts.display_stats = true;
>>> -                       break;
>>> -               case 1087:
>>> -                       pr_msg("Will skip non-existant sysctls on
>>> restore\n");
>>> -                       opts.weak_sysctls = true;
>>> -                       break;
>>>                  case 1088:
>>>                          if (sscanf(optarg, "%d", &opts.status_fd) != 1) {
>>>                                  pr_err("Unable to parse a value of
>>> --status-fd\n");
>>>                                  return 1;
>>>                          }
>>>                          break;
>>> -               case 1089:
>>> -                       opts.remote = true;
>>> -                       break;
>>>                  case 'V':
>>>                          pr_msg("Version: %s\n", CRIU_VERSION);
>>>                          if (strcmp(CRIU_GITID, "0"))
>>> @@ -639,6 +603,17 @@ int main(int argc, char *argv[], char *envp[])
>>>                  }
>>>          }
>>>    +     if (opts.deprecated_ok)
>>> +               pr_msg("Turn deprecated stuff ON\n");
>>> +       if (opts.tcp_skip_in_flight)
>>> +               pr_msg("Will skip in-flight TCP connections\n");
>>> +       if (opts.tcp_established_ok)
>>> +               pr_info("Will dump TCP connections\n");
>>> +       if (opts.link_remap_ok)
>>> +               pr_info("Will allow link remaps on FS\n");
>>> +       if (opts.weak_sysctls)
>>> +               pr_msg("Will skip non-existant sysctls on restore\n");
>>> +
>>>          if (getenv("CRIU_DEPRECATED")) {
>>>                  pr_msg("Turn deprecated stuff ON via env\n");
>>>                  opts.deprecated_ok = true;
>>> @@ -851,29 +826,29 @@ usage:
>>>    "Dump/Restore options:\n"
>>>    "\n"
>>>    "* Generic:\n"
>>> -"  -t|--tree PID         checkpoint a process tree identified by PID\n"
>>> -"  -d|--restore-detached detach after restore\n"
>>> -"  -S|--restore-sibling  restore root task as sibling\n"
>>> -"  -s|--leave-stopped    leave tasks in stopped state after checkpoint\n"
>>> -"  -R|--leave-running    leave tasks in running state after checkpoint\n"
>>> -"  -D|--images-dir DIR   directory for image files\n"
>>> -"     --pidfile FILE     write root task, service or page-server pid to
>>> FILE\n"
>>> -"  -W|--work-dir DIR     directory to cd and write logs/pidfiles/stats
>>> to\n"
>>> -"                        (if not specified, value of --images-dir is
>>> used)\n"
>>> -"     --cpu-cap [CAP]    CPU capabilities to write/check. CAP is
>>> comma-separated\n"
>>> -"                        list of: cpu, fpu, all, ins, none. To disable\n"
>>> -"                        a capability, use ^CAP. Empty argument implies
>>> all\n"
>>> -"     --exec-cmd         execute the command specified after '--' on
>>> successful\n"
>>> -"                        restore making it the parent of the restored
>>> process\n"
>>> -"  --freeze-cgroup       use cgroup freezer to collect processes\n"
>>> -"  --weak-sysctls        skip restoring sysctls that are not available\n"
>>> -"  --lazy-pages          restore pages on demand\n"
>>> -"                        this requires running a second instance of
>>> criu\n"
>>> -"                        in lazy-pages mode: 'criu lazy-pages -D DIR'\n"
>>> -"                        --lazy-pages and lazy-pages mode require
>>> userfaultfd\n"
>>> -"  --check-only          check if checkpointing/restoring will actually
>>> work\n"
>>> -"                        the process will keep on running and memory
>>> pages\n"
>>> -"                        will not be dumped\n"
>>> +"  -t|--tree PID              checkpoint a process tree identified by
>>> PID\n"
>>> +"  -d|--[no-]restore-detached [don't] detach after restore\n"
>>> +"  -S|--[no-]restore-sibling  [don't] restore root task as sibling\n"
>>> +"  -s|--leave-stopped         leave tasks in stopped state after
>>> checkpoint\n"
>>> +"  -R|--leave-running         leave tasks in running state after
>>> checkpoint\n"
>>> +"  -D|--images-dir DIR        directory for image files\n"
>>> +"     --pidfile FILE          write root task, service or page-server pid
>>> to FILE\n"
>>> +"  -W|--work-dir DIR          directory to cd and write
>>> logs/pidfiles/stats to\n"
>>> +"                             (if not specified, value of --images-dir is
>>> used)\n"
>>> +"     --cpu-cap [CAP]         CPU capabilities to write/check. CAP is
>>> comma-separated\n"
>>> +"                             list of: cpu, fpu, all, ins, none. To
>>> disable\n"
>>> +"                             a capability, use ^CAP. Empty argument
>>> implies all\n"
>>> +"     --exec-cmd              execute the command specified after '--' on
>>> successful\n"
>>> +"                             restore making it the parent of the
>>> restored process\n"
>>> +"  --freeze-cgroup            use cgroup freezer to collect processes\n"
>>> +"  --[no-]weak-sysctls        [don't] skip restoring sysctls that are not
>>> available\n"
>>> +"  --lazy-pages               restore pages on demand\n"
>>> +"                             this requires running a second instance of
>>> criu\n"
>>> +"                             in lazy-pages mode: 'criu lazy-pages -D
>>> DIR'\n"
>>> +"                             --lazy-pages and lazy-pages mode require
>>> userfaultfd\n"
>>> +"  --check-only               check if checkpointing/restoring will
>>> actually work\n"
>>> +"                             the process will keep on running and memory
>>> pages\n"
>>> +"                             will not be dumped\n"
>>>    "\n"
>>>    "* External resources support:\n"
>>>    "  --external RES        dump objects from this list as external
>>> resources:\n"
>>> @@ -890,21 +865,21 @@ usage:
>>>    "                            macvlan[IFNAME]:OUTNAME\n"
>>>    "                            mnt[COOKIE]:ROOT\n"
>>>    "\n"
>>> -"  --remote              dump/restore images directly to/from remote node
>>> using\n"
>>> -"                        image-proxy/image-cache\n"
>>> +"  --[no-]remote             [don't] dump/restore images directly to/from
>>> remote node using\n"
>>> +"                            image-proxy/image-cache\n"
>>>    "* Special resources support:\n"
>>> -"     --" SK_EST_PARAM "  checkpoint/restore established TCP
>>> connections\n"
>>> -"     --" SK_INFLIGHT_PARAM "   skip (ignore) in-flight TCP
>>> connections\n"
>>> -"  -r|--root PATH        change the root filesystem (when run in mount
>>> namespace)\n"
>>> -"  --evasive-devices     use any path to a device file if the original
>>> one\n"
>>> -"                        is inaccessible\n"
>>> -"  --link-remap          allow one to link unlinked files back when
>>> possible\n"
>>> -"  --ghost-limit size    limit max size of deleted file contents inside
>>> image\n"
>>> -"  --action-script FILE  add an external action script\n"
>>> -"  -j|--" OPT_SHELL_JOB "        allow one to dump and restore shell
>>> jobs\n"
>>> -"  -l|--" OPT_FILE_LOCKS "       handle file locks, for safety, only used
>>> for container\n"
>>> +"     --[no-]" SK_EST_PARAM "  [don't] checkpoint/restore established TCP
>>> connections\n"
>>> +"     --[no-]" SK_INFLIGHT_PARAM "   [don't] skip (ignore) in-flight TCP
>>> connections\n"
>>> +"  -r|--root PATH            change the root filesystem (when run in
>>> mount namespace)\n"
>>> +"  --[no-]evasive-devices    [don't] use any path to a device file if the
>>> original one\n"
>>> +"                            is inaccessible\n"
>>> +"  --[no-]link-remap         [don't] allow one to link unlinked files
>>> back when possible\n"
>>> +"  --ghost-limit size        limit max size of deleted file contents
>>> inside image\n"
>>> +"  --action-script FILE      add an external action script\n"
>>> +"  -j|--[no-]" OPT_SHELL_JOB "     [don't] allow one to dump and restore
>>> shell jobs\n"
>>> +"  -l|--[no-]" OPT_FILE_LOCKS "    [don't] handle file locks, for safety,
>>> only used for container\n"
>>>    "  -L|--libdir           path to a plugin directory (by default "
>>> CR_PLUGIN_DEFAULT ")\n"
>>> -"  --force-irmap         force resolving names for inotify/fsnotify
>>> watches\n"
>>> +"  --[no-]force-irmap        [don't] force resolving names for
>>> inotify/fsnotify watches\n"
>>>    "  --irmap-scan-path FILE\n"
>>>    "                        add a path the irmap hints to scan\n"
>>>    "  --manage-cgroups [m]  dump/restore process' cgroups; argument can be
>>> one of\n"
>>> @@ -949,8 +924,8 @@ usage:
>>>    "Check options:\n"
>>>    "  Without options, \"criu check\" checks availability of absolutely
>>> required\n"
>>>    "  kernel features, critical for performing dump and restore.\n"
>>> -"  --extra               add check for extra kernel features\n"
>>> -"  --experimental        add check for experimental kernel features\n"
>>> +"  --[no-]extra          add [remove] check for extra kernel features\n"
>>> +"  --[no-]experimental   add [remove] check for experimental kernel
>>> features\n"
>>>    "  --all                 same as --extra --experimental\n"
>>>    "  --feature FEAT        only check a particular feature, one of:"
>>>          );
>>> @@ -959,28 +934,28 @@ usage:
>>>    "\n"
>>>    "* Logging:\n"
>>>    "  -o|--log-file FILE    log file name\n"
>>> -"     --log-pid          enable per-process logging to separate FILE.pid
>>> files\n"
>>> +"     --[no-]log-pid     [don't] enable per-process logging to separate
>>> FILE.pid files\n"
>>>    "  -v[v...]            increase verbosity (can use multiple v)\n"
>>>    "  -vNUM               set verbosity to NUM (higher level means more
>>> output):\n"
>>>    "                          -v1 - only errors and messages\n"
>>>    "                          -v2 - also warnings (default level)\n"
>>>    "                          -v3 - also information messages and
>>> timestamps\n"
>>>    "                          -v4 - lots of debug\n"
>>> -"  --display-stats       print out dump/restore stats\n"
>>> +"  --[no-]display-stats  [don't] print out dump/restore stats\n"
>>>    "\n"
>>>    "* Memory dumping options:\n"
>>> -"  --track-mem           turn on memory changes tracker in kernel\n"
>>> -"  --prev-images-dir DIR path to images from previous dump (relative to
>>> -D)\n"
>>> -"  --page-server         send pages to page server (see options below as
>>> well)\n"
>>> -"  --auto-dedup          when used on dump it will deduplicate \"old\"
>>> data in\n"
>>> -"                        pages images of previous dump\n"
>>> -"                        when used on restore, as soon as page is
>>> restored, it\n"
>>> -"                        will be punched from the image\n"
>>> +"  --[no-]track-mem           turn on [off] memory changes tracker in
>>> kernel\n"
>>> +"  --prev-images-dir DIR      path to images from previous dump (relative
>>> to -D)\n"
>>> +"  --[no-]page-server         [don't] send pages to page server (see
>>> options below as well)\n"
>>> +"  --[no-]auto-dedup          when used on dump it will [won't]
>>> deduplicate \"old\" data in\n"
>>> +"                                  pages images of previous dump\n"
>>> +"                             when used on restore, as soon as page is
>>> restored, it\n"
>>> +"                                  will [won't] be punched from the
>>> image\n"
>>>    "\n"
>>>    "Page/Service server options:\n"
>>>    "  --address ADDR        address of server or service\n"
>>>    "  --port PORT           port of page serve or service\n"
>>> -"  -d|--daemon           run in the background after creating socket\n"
>>> +"  -d|--[no-]daemon      [don't] run in the background after creating
>>> socket\n"
>>>    "  --status-fd FD        write \\0 to the FD and close it once process
>>> is ready\n"
>>>    "                        to handle requests\n"
>>>    "\n"
>>
>> Please make sure criu --help output fits into 80 columns, otherwise it looks
>> ugly on the default 80x24 terminal. Perhaps you can also describe the no-
>> prefix separately (same suggestion as for the man page).
>>
>>
>>> diff --git a/criu/include/cr_options.h b/criu/include/cr_options.h
>>> index 518d2a3..00e7f9a 100644
>>> --- a/criu/include/cr_options.h
>>> +++ b/criu/include/cr_options.h
>>> @@ -51,21 +51,21 @@ struct cr_options {
>>>          int                     final_state;
>>>          char                    *show_dump_file;
>>>          char                    *show_fmt;
>>> -       bool                    check_extra_features;
>>> -       bool                    check_experimental_features;
>>> +       int                     check_extra_features;
>>> +       int                     check_experimental_features;
>>>          bool                    show_pages_content;
>>>          union {
>>> -               bool            restore_detach;
>>> +               int             restore_detach;
>>>                  bool            daemon_mode;
>>>          };
>>> -       bool                    restore_sibling;
>>> +       int                     restore_sibling;
>>>          bool                    ext_unix_sk;
>>> -       bool                    shell_job;
>>> -       bool                    handle_file_locks;
>>> -       bool                    tcp_established_ok;
>>> -       bool                    evasive_devices;
>>> -       bool                    link_remap_ok;
>>> -       bool                    log_file_per_pid;
>>> +       int                     shell_job;
>>> +       int                     handle_file_locks;
>>> +       int                     tcp_established_ok;
>>> +       int                     evasive_devices;
>>> +       int                     link_remap_ok;
>>> +       int                     log_file_per_pid;
>>>          bool                    swrk_restore;
>>>          char                    *output;
>>>          char                    *root;
>>> @@ -76,15 +76,15 @@ struct cr_options {
>>>          struct list_head        external;
>>>          struct list_head        join_ns;
>>>          char                    *libdir;
>>> -       bool                    use_page_server;
>>> +       int                     use_page_server;
>>>          unsigned short          port;
>>>          char                    *addr;
>>>          int                     ps_socket;
>>> -       bool                    track_mem;
>>> +       int                     track_mem;
>>>          char                    *img_parent;
>>> -       bool                    auto_dedup;
>>> +       int                     auto_dedup;
>>>          unsigned int            cpu_cap;
>>> -       bool                    force_irmap;
>>> +       int                     force_irmap;
>>>          char                    **exec_cmd;
>>>          unsigned int            manage_cgroups;
>>>          char                    *new_global_cg_root;
>>> @@ -92,8 +92,8 @@ struct cr_options {
>>>          char                    *cgroup_props_file;
>>>          struct list_head        new_cgroup_roots;
>>>          bool                    autodetect_ext_mounts;
>>> -       bool                    enable_external_sharing;
>>> -       bool                    enable_external_masters;
>>> +       int                     enable_external_sharing;
>>> +       int                     enable_external_masters;
>>>          bool                    aufs;           /* auto-deteced, not via
>>> cli */
>>>          bool                    overlayfs;
>>>    #ifdef CONFIG_BINFMT_MISC_VIRTUALIZED
>>> @@ -106,7 +106,7 @@ struct cr_options {
>>>          unsigned int            timeout;
>>>          unsigned int            empty_ns;
>>>          bool                    lazy_pages;
>>> -       bool                    tcp_skip_in_flight;
>>> +       int                     tcp_skip_in_flight;
>>>          char                    *work_dir;
>>>          /*
>>> @@ -115,13 +115,13 @@ struct cr_options {
>>>           * the deprecated stuff is not working, but it's still possible
>>>           * to turn one ON while the code is in.
>>>           */
>>> -       bool                    deprecated_ok;
>>> -       bool                    display_stats;
>>> -       bool                    weak_sysctls;
>>> +       int                     deprecated_ok;
>>> +       int                     display_stats;
>>> +       int                     weak_sysctls;
>>>          int                     status_fd;
>>>          bool                    orphan_pts_master;
>>>          bool                    check_only;
>>> -       bool                    remote;
>>> +       int                     remote;
>>>    };
>>>      extern struct cr_options opts;
>>> diff --git a/criu/include/file-lock.h b/criu/include/file-lock.h
>>> index c3f2dab..f9936da 100644
>>> --- a/criu/include/file-lock.h
>>> +++ b/criu/include/file-lock.h
>>> @@ -67,6 +67,7 @@ struct fd_parms;
>>>    extern int note_file_lock(struct pid *, int fd, int lfd, struct fd_parms
>>> *);
>>>    extern int dump_file_locks(void);
>>>    -#define OPT_FILE_LOCKS       "file-locks"
>>> +#define OPT_FILE_LOCKS         "file-locks"
>>> +#define OPT_NO_FILE_LOCKS      "no-file-locks"
>>>      #endif /* __FILE_LOCK_H__ */
>>> diff --git a/criu/include/files.h b/criu/include/files.h
>>> index 5aa6139..1789408 100644
>>> --- a/criu/include/files.h
>>> +++ b/criu/include/files.h
>>> @@ -168,6 +168,7 @@ extern int close_old_fds(void);
>>>    #endif
>>>      #define LREMAP_PARAM        "link-remap"
>>> +#define NO_LREMAP_PARAM "no-link-remap"
>>>      extern int shared_fdt_prepare(struct pstree_item *item);
>>>    diff --git a/criu/include/sk-inet.h b/criu/include/sk-inet.h
>>> index bf6fb1d..cf0fff6 100644
>>> --- a/criu/include/sk-inet.h
>>> +++ b/criu/include/sk-inet.h
>>> @@ -73,8 +73,10 @@ extern void cpt_unlock_tcp_connections(void);
>>>    extern int dump_one_tcp(int sk, struct inet_sk_desc *sd);
>>>    extern int restore_one_tcp(int sk, struct inet_sk_info *si);
>>>    -#define SK_EST_PARAM "tcp-established"
>>> -#define SK_INFLIGHT_PARAM "skip-in-flight"
>>> +#define SK_EST_PARAM           "tcp-established"
>>> +#define SK_NO_EST_PARAM                "no-tcp-established"
>>> +#define SK_INFLIGHT_PARAM      "skip-in-flight"
>>> +#define SK_NO_INFLIGHT_PARAM   "no-skip-in-flight"
>>>      struct task_restore_args;
>>>    int prepare_tcp_socks(struct task_restore_args *);
>>> diff --git a/criu/include/tty.h b/criu/include/tty.h
>>> index 5ad8b09..0a7015c 100644
>>> --- a/criu/include/tty.h
>>> +++ b/criu/include/tty.h
>>> @@ -39,6 +39,7 @@ extern int tty_restore_ctl_terminal(struct file_desc *d,
>>> int fd);
>>>      extern int devpts_check_bindmount(struct mount_info *m);
>>>    -#define OPT_SHELL_JOB        "shell-job"
>>> +#define OPT_SHELL_JOB          "shell-job"
>>> +#define OPT_NO_SHELL_JOB       "no-shell-job"
>>>      #endif /* __CR_TTY_H__ */
>>
>> _______________________________________________
>> CRIU mailing list
>> CRIU at openvz.org
>> https://lists.openvz.org/mailman/listinfo/criu
>
>



More information about the CRIU mailing list