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

[Dmitry Safonov]

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

>
>> 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