On 02/07/2011 05:54 PM, Dhaval Giani wrote:
> As per the discussion on the mailing list, the daemon should have
> the ability to load both configuration files and configuration directories.
>
> Provide this ability.
>
> TODO:
> 1. Move to a fts based walk as opposed to a readdir.
I am not sure it adds anything useful. readdir() looks fine to me. Or
are you interested in walking subdirectories?
> 2. Decide whether a configuration file is default or a configuration
> directory.
> 3. Handle minor coding style issues.
>
> Signed-off-by: Dhaval Giani<dhaval.gi...@gmail.com>
>
> ---
> src/cgrules.c | 90
> ++++++++++++++++++++++++++++++++++++++++++++++-
> src/daemon/cgrulesengd.c | 28 +++++++++++++-
> src/libcgroup-internal.h | 1
> 3 files changed, 116 insertions(+), 3 deletions(-)
>
> Index: libcg/src/daemon/cgrulesengd.c
> ===================================================================
> --- libcg.orig/src/daemon/cgrulesengd.c
> +++ libcg/src/daemon/cgrulesengd.c
> @@ -960,7 +960,7 @@ int main(int argc, char *argv[])
> struct group *gr;
>
> /* Command line arguments */
> - const char *short_options = "hvqf:s::ndQu:g:c:";
> + const char *short_options = "hvqf:s::ndQu:g:c:D:";
> struct option long_options[] = {
> {"help", no_argument, NULL, 'h'},
> {"verbose", no_argument, NULL, 'v'},
> @@ -973,10 +973,12 @@ int main(int argc, char *argv[])
> {"socket-user", required_argument, NULL, 'u'},
> {"socket-group", required_argument, NULL, 'g'},
> {"config-file", required_argument, NULL, 'c'},
> + {"config-dir", required_argument, NULL, 'D'},
> {NULL, 0, NULL, 0}
> };
>
> cgrules_config_file = NULL;
> + cgrules_config_dir = NULL;
>
> /* Make sure the user is root. */
> if (getuid() != 0) {
> @@ -1069,6 +1071,13 @@ int main(int argc, char *argv[])
> goto finished;
> }
> break;
> + case 'D': /* --config-dir */
> + cgrules_config_dir = strdup(optarg);
> + if (!cgrules_config_dir) {
> + ret = 4;
> + goto finished;
> + }
> + break;
> default:
> usage(stderr, "");
> ret = 2;
> @@ -1076,7 +1085,22 @@ int main(int argc, char *argv[])
> }
> }
>
> - if (!cgrules_config_file) {
> + /*
> + * Cannot have both config dir and config file set.
> + * This will confuse the implementation. So just mark such a
> + * configuration as invalid and fail.
> + */
> + if (cgrules_config_file&& cgrules_config_dir) {
> + fprintf(stderr, "Error: Both configuration directory and " \
> + "configuration file are mentioned\n");
> + goto finished;
> + }
> +
> + /*
> + * XX: TODO: Decide if config dir is the default or the config file is
> + * Current implementation just keeps the config file as default.
> + */
> + if (!cgrules_config_file&& !cgrules_config_dir) {
> cgrules_config_file = strdup(CGRULES_DEFAULT_CONFIG);
> if (!cgrules_config_file) {
This decision should be made inside the library. Other apps, which use
rules, would benefit from that and behave consistently.
> fprintf(stderr, "Failed to set the correct"\
> Index: libcg/src/libcgroup-internal.h
> ===================================================================
> --- libcg.orig/src/libcgroup-internal.h
> +++ libcg/src/libcgroup-internal.h
> @@ -68,6 +68,7 @@ __BEGIN_DECLS
> #define min(x,y) ((y)>(x)?(x):(y))
>
> char *cgrules_config_file;
> +char *cgrules_config_dir;
>
> /* Check if cgroup_init has been called or not. */
> int cgroup_initialized;
> Index: libcg/src/cgrules.c
> ===================================================================
> --- libcg.orig/src/cgrules.c
> +++ libcg/src/cgrules.c
> @@ -17,6 +17,8 @@
> *
> */
>
> +#define _GNU_SOURCE
> +
> #include<dirent.h>
> #include<errno.h>
> #include<libcgroup.h>
> @@ -38,9 +40,11 @@
> #include<assert.h>
> #include<linux/un.h>
> #include<grp.h>
> +#include<stddef.h>
>
> /* Check if the rules cache has been loaded or not. */
> static bool cgroup_rules_loaded;
> +static bool cgrules_dir_provided;
>
> /* List of configuration rules */
> static struct cgroup_rule_list rl;
> @@ -474,8 +478,92 @@ unlock:
> static int cgroup_parse_rules(bool cache, uid_t muid,
> gid_t mgid, const char *mprocname)
> {
> - return cgroup_parse_rules_file(cache, muid, mgid, mprocname,
> cgrules_config_file);
> + DIR *cgrules_dir;
> + struct dirent *curr_ent = NULL;
> + int len;
> + struct dirent *dir_buff;
> + int read_ret;
> + int parse_fail_count = 0;
> + char *parse_fail_list = NULL;
> +
> + if (cgrules_config_file)
> + return cgroup_parse_rules_file(cache, muid, mgid, mprocname,
> + cgrules_config_file);
> +
> + /*
> + * This means we have a directory defined, and we parse using that
> + * TODO: Implement using fts if folks think that is a better option
> + */
> +
> + cgrules_dir = opendir(cgrules_config_dir);
> + if (!cgrules_dir) {
> + last_errno = errno;
> + return ECGOTHER;
> + }
> +
> + len = offsetof(struct dirent, d_name) +
> + pathconf(cgrules_config_dir, _PC_NAME_MAX) + 1;
> +
> + dir_buff = malloc(len);
> + if (!dir_buff) {
> + last_errno = errno;
> + return ECGOTHER;
> + }
> +
> + read_ret = readdir_r(cgrules_dir, curr_ent,&dir_buff);
I don't think this is valid usage of readdir_r, curr_ent is
uninitialized and the call will probably crash. See
http://womble.decadent.org.uk/readdir_r-advisory.html for an example how
to use and why it is not so good idea... Simple readdir is safe here,
it's return values can't mix with app's readdir in other threads.
> +
> + while(read_ret == 0&& curr_ent) {
while(read_ret == 0&& dir_buff) ?
> + char *cgrules_file;
> + int parse_ret;
> +
> + /*
> + * We need a better way to handle DT_UNKNOWN, but for now we
> + * just pray to $DEITY that we can handle it:-)
> + */
> +
> + if (curr_ent->d_type != DT_REG || curr_ent->d_type !=
> DT_UNKNOWN) {
> + read_ret = readdir_r(cgrules_dir, curr_ent,&dir_buff);
> + continue;
> + }
> +
> + cgrules_file = strdup(curr_ent->d_name);
Why strdup? Is it needed after cgroup_parse_rules_file?
> +
> + parse_ret = cgroup_parse_rules_file(cache, muid, mgid,
> mprocname,
> + cgrules_file);
> +
> + if (parse_ret) {
> + char *tmp_list;
> + parse_fail_count++;
> + if (parse_fail_list) {
> + tmp_list = strdup(parse_fail_list);
> + if (!tmp_list) {
> + last_errno = errno;
> + return ECGOTHER;
> + }
> +
> + free(parse_fail_list);
> + asprintf(&parse_fail_list, "%s, %s", tmp_list,
> curr_ent->d_name);
> + } else {
> + parse_fail_list = strdup(curr_ent->d_name);
> + }
> +
> + }
> + read_ret = readdir_r(cgrules_dir, curr_ent,&dir_buff);
> +
> + }
> + fprintf(stderr, "Failed to parse %d files\n", parse_fail_count);
> + fprintf(stderr, "Failed to parse %s\n", parse_fail_list);
I don't like fprintf in the library... use cgroup_dbg please. Or use
different method how to tell user what's wrong, e.g. by returning a
pointer or better logging. cgrulesengd logs to syslog for example.
> +
> + /*
> + * TODO: Right return value
> + */
> + if (parse_fail_count)
> + return 0;
> +
Probably some free() and closedir() are missing, please be careful.
> + return 0;
> +
> }
> +
> /** cg_prepare_cgroup
> * Process the selected rule. Prepare the cgroup structure which can be
> * used to add the task to destination cgroup.
>
>
------------------------------------------------------------------------------
The ultimate all-in-one performance toolkit: Intel(R) Parallel Studio XE:
Pinpoint memory and threading errors before they happen.
Find and fix more than 250 security defects in the development cycle.
Locate bottlenecks in serial and parallel code that limit performance.
http://p.sf.net/sfu/intel-dev2devfeb
_______________________________________________
Libcg-devel mailing list
Libcg-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/libcg-devel
