Add a file exists check to adaptived #24
Conversation
Signed-off-by: George Kennedy <[email protected]>
Signed-off-by: George Kennedy <[email protected]>
Unlike the effects, where the file has to exist during init time, for the causes the file has to exist when the cause is checked in *_main(). Signed-off-by: George Kennedy <[email protected]>
oracle-contributor-agreement
bot
added
the
OCA Verified
| char *subp; | ||
|
|
||
| strcpy(check_path, path); | ||
| subp = strstr(check_path, "*"); |
There was a problem hiding this comment.
I'm not a fan of replacing * with \0. While unusual, * is valid in filenames and could occur anywhere in the file. Also, * is used by adaptived for wildcarding of paths.
I don't think we want this check.
| @@ -80,6 +80,10 @@ int cgroup_setting_psi_init(struct adaptived_effect * const eff, struct json_obj | |||
| if (ret) | |||
| goto error; | |||
|
|
|||
| ret = adaptived_file_exists(cgroup_path_str); | |||
There was a problem hiding this comment.
The cgroup_setting_by_psi effect can operate on a hierarchy, thus * is valid in the path (and it may not map to a valid path.)
I think a better option here would be to do something like:
path_len = strlen(cgroup_path_str);
if (cgroup_path_str[path_len - 1] == '*') {
/*
* Skip file existence check because a wildcard path was provided
*/
} else {
ret = adaptived_file_exists(cgroup_path_str);
if (ret)
goto error;
}
There was a problem hiding this comment.
Since there could be some copy-pasta here, you could incorporate this into adaptived_file_exists() with a parameter like skip_wildcard or something like that
| @@ -64,6 +64,10 @@ int kill_cgroup_init(struct adaptived_effect * const eff, struct json_object *ar | |||
| if (ret) | |||
| goto error; | |||
|
|
|||
| ret = adaptived_file_exists(cgroup_path_str); | |||
There was a problem hiding this comment.
kill_cgroup also allows wildcards. See comment above in the PSI effect.
| @@ -66,6 +66,10 @@ int kill_cgroup_psi_init(struct adaptived_effect * const eff, struct json_object | |||
| if (ret) | |||
| goto error; | |||
|
|
|||
| ret = adaptived_file_exists(cgroup_path_str); | |||
There was a problem hiding this comment.
kill_cgroup_by_psi also allows wildcards. See comment above in the PSI effect.
| @@ -74,6 +74,10 @@ static int _cgroup_setting_init(struct adaptived_effect * const eff, struct json | |||
| if (ret) | |||
| goto error; | |||
|
|
|||
| ret = adaptived_file_exists(setting_str); | |||
There was a problem hiding this comment.
Also if we're going to check if the file exists, I think we need a way for the user to skip this check.
What if they had a config like this:
causes:
always
effects:
make_a_cgroup
operate_on_the_cgroup_just_made
A config like the above wouldn't work with the checks as written because the cgroup doesn't exist until the make_a_cgroup's main runs.
There was a problem hiding this comment.
We could mimic the -ENOENT logic for other optional flags
|
Maybe we don't need this pull request anymore as you re-wrote the tests that create cgroups to work v1 or v2. Abandon this pull request? |
Check for file exists in causes and effects.