[CRIU] [PATCH 2/4] Add support for configuration files

Dmitry Safonov 0x7f454c46 at gmail.com
Wed May 24 02:50:47 PDT 2017


2017-05-24 12:41 GMT+03:00 Dmitry Safonov <0x7f454c46 at gmail.com>:
> 2017-05-23 20:58 GMT+03:00 Veronika Kabatova <vkabatov at redhat.com>:
>>
>>
>> ----- Original Message -----
>>> From: "Dmitry Safonov" <0x7f454c46 at gmail.com>
>>> To: "Veronika Kabatova" <vkabatov at redhat.com>
>>> Cc: "crml" <criu at openvz.org>
>>> Sent: Monday, May 22, 2017 8:23:08 PM
>>> Subject: Re: [CRIU] [PATCH 2/4] Add support for configuration files
>>>
>>> Hi Veronika,
>>>
>>> I like this stuff, but this looks like it wants some cleanups.
>>> Please, check comments below.
>>>
>>
>> Hi, thanks for the thorough review! I agree with most of what you said.
>> Comments below as well. If you won't have additional advice about these
>> things I can submit the fixed patchset.
>>
>>> 2017-05-22 18:00 GMT+03:00  <vkabatov at redhat.com>:
>>> > From: Veronika Kabatova <vkabatov at redhat.com>
>>> >
>>> > Implementation changes for usage of simple configuration files. Before
>>> > parsing the command line options, either default configuration files
>>> > (/etc/criu.d/default.conf, $HOME/.criu.d/default.conf; in this order) are
>>> > parsed, or a specific config file passed by the user. Two new options are
>>> > introduced: "--config FILEPATH" option allows users to specify a single
>>> > configuration file they want to use; and "--no-default-config" option to
>>> > forbid the parsing of default configuration files. Both options are to be
>>> > passed only via the command line.
>>> >
>>> > Usage of configuration files is not mandatory to keep backwards
>>> > compatibility. The implementation of this feature tries to be compatible
>>> > with command line usage -- the user should get the same results whether
>>> > he passes the options (in the right order of parsing) on command line or
>>> > writes them in config files. This allows the user to:
>>> >
>>> > 1) Override boolean options if needed
>>> > 2) Specify partial configuration for options that are possible to pass
>>> >    several times (e.g. "--external"), and pass the rest of the options
>>> >    based on process runtime by command line
>>> >
>>> > Configuration file syntax allows comments marked with '#' sign, the rest
>>> > of the line after '#' is ignored. The user can use one option per line
>>> > (with argument supplied on the same line if needed, divided with whitespace
>>> > characters), the options are the same as long options (without the "--"
>>> > prefix used on command line).
>>> >
>>> > Configuration file example (syntax purposes only, doesn't make sense):
>>> >
>>> > $ cat ~/.criu.d/default.conf
>>> > tcp-established
>>> > work-dir /home/<USERNAME>/criu/work_directory
>>> > extra # inline comment
>>> > no-restore-sibling
>>> > tree    111111
>>> >
>>> > Signed-off-by: Veronika Kabatova <vkabatov at redhat.com>
> [..]
>>>
>>> > +       int elements_read = 0, len = 0;
>>> > +       bool was_newline = true;
>>> > +       char **configuration = xmalloc(size * sizeof(char *));
>>>
>>> Variable declarations should be at the function beginning,
>>> that's C style, gcc by default may bark on you with warnings
>>> for such stuff.
>>>
>>
>> GCC has never been angry at me for not doing that, but I changed
>> it to match your style.
>
> Well, it's -Wdeclaration-after-statement GCC's option.
> It's used by default in Linux kernel and we try to follow kernel's
> code style in CRIU.
> You can view it if you want here:
> http://elixir.free-electrons.com/linux/latest/source/Documentation/process/coding-style.rst

Ok, actually, it's not described there (it's been some time since I last looked
there, so was sure it's written), but kernel just follows it with
enabled gcc option.

>
>>
>>> While at it - `elements_read' is too gross for loop counter,
>>> could it be just `i'?
>>> While in contrast `size' may be more descriptive..
>>>
>>> > +       if (configuration == NULL) {
>>> > +               fclose(configfile);
>>> > +               return NULL;
>>> > +       }
>>> > +       /* Initialize first element, getopt ignores it */
>>> > +       configuration[0] = xmalloc((strlen("criu") + 1) * sizeof(char));
>>>
>>> sizeof(char) = 1, I assure you!
>>>
>>> > +       if (configuration[0] == NULL) {
>>> > +               fclose(configfile);
>>> > +               return NULL;
>>> > +       }
>>> > +       memcpy(configuration[0], "criu\0", strlen("criu") + 1);
>>>
>>> Why is this not just
>>> configuration[0] = "criu";
>>> huh?
>>>
>>
>> Getting a nice core dump on my machine if I use this. I suspect it's because
>> of freeing the configuration arrays, and valgrind kinda confirms it saying
>> the address is in r-x mapped file segment, mentioned on free() call. I guess
>> I can change the free_array function, but because of potential reuse of it on
>> any other char **array I prefer not to.
>
> Oh, I see - then change free_array *or* add a comment here, please.
>
>>> > @@ -333,9 +494,30 @@ int main(int argc, char *argv[], char *envp[])
>>> >
>>> >         while (1) {
>>> >                 idx = -1;
>>> > -               opt = getopt_long(argc, argv, short_opts, long_opts, &idx);
>>> > -               if (opt == -1)
>>> > +
>>> > +               switch (state) {
>>> > +               case PARSING_FIRST:
>>> > +                       opt = getopt_long(first_count, first_conf,
>>> > short_opts, long_opts, &idx);
>>> > +                       break;
>>> > +               case PARSING_SECOND:
>>> > +                       opt = getopt_long(second_count, second_conf,
>>> > short_opts, long_opts, &idx);
>>> > +                       break;
>>> > +               case PARSING_ARGV:
>>> > +                       opt = getopt_long(argc, argv, short_opts,
>>> > long_opts, &idx);
>>> > +                       break;
>>> > +               default:
>>> >                         break;
>>> > +               }
>>> > +
>>> > +               if (opt == -1) {
>>> > +                       if (state < PARSING_ARGV) {
>>>
>>> So, as you have default switch break, you
>>> can just omit this comparisson.
>>>
>>
>> The default branch in switch breaks only from the switch. This one is
>> for ending the while loop. I guess I can remove the default branch if
>> it's confusing since no other values are expected, I'm just used to
>> have it there.
>
> Yeah, please, remove the default as it's not used and makes
> no difference with/without it anyway.
>
> --
>              Dmitry



-- 
             Dmitry


More information about the CRIU mailing list