<div dir="ltr"><div>Hi Richard, <br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Mar 7, 2022 at 2:43 AM Richard Palethorpe <<a href="mailto:rpalethorpe@suse.de">rpalethorpe@suse.de</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hello Luke,<br>
<br>
Really great work for the patch series in general! However see comments<br>
below.<br>
<br>
Luke Nowakowski-Krijger <<a href="mailto:luke.nowakowskikrijger@canonical.com" target="_blank">luke.nowakowskikrijger@canonical.com</a>> writes:<br>
<br>
> Implement tst_cg_load_config which consumes the state given by<br>
> tst_cg_print_config to update the internal test state to reflect<br>
> the given config.<br>
><br>
> This allows for programs using the cgroup C API to load and reload<br>
> state, allowing functionality such as calling tst_cg_require and<br>
> tst_cg_cleanup to function properly between programs or between<br>
> invocations of a binary using the C API.<br>
><br>
> Signed-off-by: Luke Nowakowski-Krijger <<a href="mailto:luke.nowakowskikrijger@canonical.com" target="_blank">luke.nowakowskikrijger@canonical.com</a>><br>
> ---<br>
> v2: Add root null check in parse_root_config.<br>
> Remove checking for ltp_drain_dir key from config as it was<br>
> redundant.<br>
> Remove unsued variable in parse_ctrl_config.<br>
> Cleanup some compiler warnings.<br>
> v3: Rewrite to consume each line of the config with a scanf to make<br>
> the parsing much simpler while using new config variables.<br>
><br>
> include/tst_cgroup.h | 7 +++++<br>
> lib/tst_cgroup.c | 62 ++++++++++++++++++++++++++++++++++++++++++++<br>
> 2 files changed, 69 insertions(+)<br>
><br>
> diff --git a/include/tst_cgroup.h b/include/tst_cgroup.h<br>
> index 87e55f4df..9bad0d366 100644<br>
> --- a/include/tst_cgroup.h<br>
> +++ b/include/tst_cgroup.h<br>
> @@ -121,6 +121,13 @@ void tst_cg_scan(void);<br>
> */<br>
> void tst_cg_print_config(void);<br>
> <br>
> +/* Load the config printed out by tst_cg_print_config and configure the internal<br>
> + * libary state to match the config. Used to allow tst_cg_cleanup to properly<br>
> + * cleanup mounts and directories created by tst_cg_require between program<br>
> + * invocations.<br>
> + */<br>
> +void tst_cg_load_config(const char *const config);<br>
> +<br>
> /* Ensure the specified controller is available in the test's default<br>
> * CGroup, mounting/enabling it if necessary. Usually this is not<br>
> * necesary use tst_test.needs_cgroup_controllers instead.<br>
> diff --git a/lib/tst_cgroup.c b/lib/tst_cgroup.c<br>
> index 8f95064b3..90d91d94e 100644<br>
> --- a/lib/tst_cgroup.c<br>
> +++ b/lib/tst_cgroup.c<br>
> @@ -366,6 +366,68 @@ static struct cgroup_root *cgroup_find_root(const char *const mnt_path)<br>
> return NULL;<br>
> }<br>
> <br>
> +static void parse_config(const char *const config_entry)<br>
<br>
cgroup_parse_config_line perhaps? or cgroup_parse_ctrl?<br>
<br>
> +{<br>
> + const char ctrl_name[32], mnt_path[PATH_MAX],<br>
> test_dir_name[NAME_MAX + 1];<br>
<br>
These buffers are not const.<br>
<br>
I have lsp and clangd setup in emacs which warns about this. It's easy<br>
to miss the warning in the compiler output.<br>
<br>
> + int ver, we_require_it, we_mounted_it, ltp_dir_we_created_it;<br>
> + struct cgroup_root *root;<br>
> + struct cgroup_ctrl *ctrl;<br>
> +<br>
> + sscanf(config_entry, CONFIG_FORMAT,<br>
> + ctrl_name, &ver, &we_require_it, mnt_path, &we_mounted_it,<br>
> + <p_dir_we_created_it, test_dir_name);<br>
<br>
You need to run 'make check-tst_cgroup' in ltp/lib. The sscanf return<br>
value is not checked which could result in segfaults and other confusing<br>
errors if parsing fails.<br>
<br>
> +<br>
> + if (!(ctrl = cgroup_find_ctrl(ctrl_name)))<br>
<br>
<a href="http://check_patch.pl" rel="noreferrer" target="_blank">check_patch.pl</a> forbids assignments in if statements (it makes static<br>
analysis difficult). Again this is caught with 'make check'.<br>
<br>
> + tst_brk(TBROK, "Could not find ctrl from config. Ctrls changing between calls?");<br>
> +<br>
> + ctrl->we_require_it = we_require_it;<br>
> +<br>
> + if (!(root = cgroup_find_root(mnt_path)))<br>
> + tst_brk(TBROK, "Could not find root from config. Config possibly malformed?");<br>
> +<br>
> + if (we_mounted_it)<br>
> + root->we_mounted_it = 1;<br>
> +<br>
> + if (!root->ltp_dir.dir_name) {<br>
> + cgroup_dir_mk(&root->mnt_dir, cgroup_ltp_dir, &root->ltp_dir);<br>
> + cgroup_dir_mk(&root->ltp_dir, cgroup_ltp_drain_dir, &root->drain_dir);<br>
> + if (ltp_dir_we_created_it) {<br>
> + root->ltp_dir.we_created_it = 1;<br>
> + root->drain_dir.we_created_it = 1;<br>
> + }<br>
> + }<br>
> +<br>
> + if (!root->test_dir.dir_name && strcmp(test_dir_name, "NULL")) {<br>
> + strncpy(cgroup_test_dir, test_dir_name, NAME_MAX);<br>
<br>
I think this could result in an unterminated string. strncpy does not<br>
add a null char after the string that was copied. Note also that the<br>
format string passed to sscanf does not limit the field width so it<br>
could overwrite root and ctrl on the stack if the input is long enough.<br>
<br></blockquote><div><br></div><div>Thank you for pointing all these things out. I forgot about all the shenanigans that can happen when you don't sanity check string stuff :)</div><div><br></div><div>A few solutions to creating the scanff format are:</div><div>1) pre-processor trick to concatenate defines into a string, something like this</div><div>#define _tostr(str) #str<br>#define tostr(str) _tostr(str)<br>#define CONFIG_FORMAT "%" tostr(CTRL_NAME_MAX) "s\t%d\t%d\t%" tostr(PATH_MAX) "s\t%d\t%d\t%" tostr(NAME_MAX) "s"</div><div><br></div><div>2) just hardcode the lengths to avoid all of this nonsense above</div><div><br></div><div>3) just create the format at runtime</div><div><br></div><div>I'm more or less asking what you think is best to avoid sending out all of the patches and then discussing this / if there is a better way. <br></div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> + cgroup_dir_mk(&root->ltp_dir, cgroup_test_dir, &root->test_dir);<br>
> + root->test_dir.we_created_it = 1;<br>
> + }<br>
> +}<br>
> +<br>
> +/* Load the test state config provided by tst_cg_print_config<br>
> + *<br>
> + * This will reload some internal tst_cgroup state given by the config<br>
> + * that might otherwise have been lost between calls or between different<br>
> + * processes. In particular this is used by testcases/lib/tst_cgctl to<br>
> + * provide access to this C api to shell scripts.<br>
> + *<br>
> + * The config keeps track of the minimal state needed for tst_cg_cleanup<br>
> + * to cleanup mounts and directories created by tst_cg_require.<br>
> + */<br>
> +void tst_cg_load_config(const char *const config)<br>
> +{<br>
> + char temp_config[BUFSIZ];<br>
> + char *line;<br>
> +<br>
> + if (strlen(config) >= BUFSIZ)<br>
> + tst_brk(TBROK, "Config has exceeded buffer size?");<br>
> + strncpy(temp_config, config, BUFSIZ);<br>
<br>
Again, this won't copy the null byte from config if strlen(config) ==<br>
BUFSIZ. You could use memcpy here anyway if you know the string length.<br>
<br>
> +<br>
> + line = strtok(temp_config, "\n");<br>
> + /* Make sure to consume the header. */<br>
> + for (line = strtok(NULL, "\n"); line; line = strtok(NULL, "\n"))<br>
> + parse_config(line);<br>
> +}<br>
> <br>
> /* Determine if a mounted cgroup hierarchy is unique and record it if so.<br>
> *<br>
<br></blockquote><div><br></div><div>I will take all of the other things you mentioned and clean them up. Thanks for the review!<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
-- <br>
Thank you,<br>
Richard.<br></blockquote><div><br></div><div>Thanks, <br></div><div>- Luke <br></div></div></div>