Skip to content

L
lxc
Commit fa659172 by Christian Brauner Committed by Stéphane Graber
Options

lxc-ls: tweak algorithm for ls_has_all_grps()

- With the -g/--groups argument the user can give a comma-separated list of groups MUST a container must have in order to be displayed. We receive this list as a single string. ls_has_all_grps() is called to check if a container has all the groups of MUST in its current list of groups HAS. I.e. we determine whether MUST ⊆ HAS and only then do we record the container. The original implementation was dumb in that it split the string MUST everytime it needed to check whether MUST ⊆ HAS for a given container. That's pointless work. Instead we split the string MUST only once in main() and pass it to ls_get() which passes it along to ls_has_all_grps(). - Before doing any costly checking make sure that #MUST <= #HAS. If not bail immediately. - The linear search algorithm ls_has_all_grps() currently uses stays for now. Binary search et al. do not seem to make sense since sorting the array HAS for each container is probably too costly. Especially, since it seems unlikely that a users specifies 50+ or so groups on the command line a container must have to be displayed. If however there are a lot of use-cases where users have a lot of containers each with 50-100 groups and regularly use lxc-ls with -g/--groups to only show containers that have 50 specified groups among their 50-100 groups we can revisit this issue and implement e.g. binary search or a ternary search tree. Signed-off-by: 's avatarChristian Brauner <christian.brauner@mailbox.org> Acked-by: 's avatarSerge E. Hallyn <serge.hallyn@ubuntu.com>
parent 4763f6ca
Showing with 37 additions and 33 deletions
src/lxc/arguments.h
...@@ -100,7 +100,7 @@ struct lxc_arguments { ...@@ -100,7 +100,7 @@ struct lxc_arguments {
int all; int all;
int ignore_auto; int ignore_auto;
int list; int list;
char *groups; char *groups; /* also used by lxc-ls */
/* lxc-snapshot and lxc-clone */ /* lxc-snapshot and lxc-clone */
enum task { enum task {
...@@ -122,7 +122,6 @@ struct lxc_arguments { ...@@ -122,7 +122,6 @@ struct lxc_arguments {
/* lxc-ls */ /* lxc-ls */
char *ls_fancy_format; char *ls_fancy_format;
char *ls_groups;
char *ls_filter; char *ls_filter;
unsigned int ls_nesting; /* maximum allowed nesting level */ unsigned int ls_nesting; /* maximum allowed nesting level */
bool ls_active; bool ls_active;
......
src/lxc/lxc_ls.c
...@@ -95,7 +95,8 @@ static void ls_free_arr(char **arr, size_t size); ...@@ -95,7 +95,8 @@ static void ls_free_arr(char **arr, size_t size);
*/ */
static int ls_get(struct ls **m, size_t *size, const struct lxc_arguments *args, static int ls_get(struct ls **m, size_t *size, const struct lxc_arguments *args,
const char *basepath, const char *parent, unsigned int lvl, const char *basepath, const char *parent, unsigned int lvl,
char **lockpath, size_t len_lockpath); char **lockpath, size_t len_lockpath, char **grps_must,
size_t grps_must_len);
static char *ls_get_cgroup_item(struct lxc_container *c, const char *item); static char *ls_get_cgroup_item(struct lxc_container *c, const char *item);
static char *ls_get_config_item(struct lxc_container *c, const char *item, static char *ls_get_config_item(struct lxc_container *c, const char *item,
bool running); bool running);
...@@ -103,6 +104,8 @@ static char *ls_get_groups(struct lxc_container *c, bool running); ...@@ -103,6 +104,8 @@ static char *ls_get_groups(struct lxc_container *c, bool running);
static char *ls_get_ips(struct lxc_container *c, const char *inet); static char *ls_get_ips(struct lxc_container *c, const char *inet);
struct wrapargs { struct wrapargs {
const struct lxc_arguments *args; const struct lxc_arguments *args;
char **grps_must;
size_t grps_must_len;
int pipefd[2]; int pipefd[2];
size_t *size; size_t *size;
const char *parent; const char *parent;
...@@ -122,7 +125,7 @@ static int ls_get_wrapper(void *wrap); ...@@ -122,7 +125,7 @@ static int ls_get_wrapper(void *wrap);
static double ls_get_swap(struct lxc_container *c); static double ls_get_swap(struct lxc_container *c);
static unsigned int ls_get_term_width(void); static unsigned int ls_get_term_width(void);
static char *ls_get_interface(struct lxc_container *c); static char *ls_get_interface(struct lxc_container *c);
static bool ls_has_all_grps(const char *has, const char *must); static bool ls_has_all_grps(const char *has, char **must, size_t must_len);
static struct ls *ls_new(struct ls **ls, size_t *size); static struct ls *ls_new(struct ls **ls, size_t *size);
/* /*
* Print user-specified fancy format. * Print user-specified fancy format.
...@@ -228,12 +231,19 @@ int main(int argc, char *argv[]) ...@@ -228,12 +231,19 @@ int main(int argc, char *argv[])
.autostart_length = 9, /* AUTOSTART */ .autostart_length = 9, /* AUTOSTART */
}; };
char **grps = NULL;
size_t ngrps = 0;
if (my_args.groups) {
grps = lxc_string_split_and_trim(my_args.groups, ',');
ngrps = lxc_array_len((void **)grps);
}
struct ls *ls_arr = NULL; struct ls *ls_arr = NULL;
size_t ls_size = 0; size_t ls_size = 0;
/* &(char *){NULL} is no magic. It's just a compound literal which /* &(char *){NULL} is no magic. It's just a compound literal which
* avoids having a pointless variable in main() that serves no purpose * avoids having a pointless variable in main() that serves no purpose
* here. */ * here. */
int status = ls_get(&ls_arr, &ls_size, &my_args, "", NULL, 0, &(char *){NULL}, 0); int status = ls_get(&ls_arr, &ls_size, &my_args, "", NULL, 0, &(char *){NULL}, 0, grps, ngrps);
if (!ls_arr && status == 0) if (!ls_arr && status == 0)
/* We did not fail. There was just nothing to do. */ /* We did not fail. There was just nothing to do. */
exit(EXIT_SUCCESS); exit(EXIT_SUCCESS);
...@@ -256,6 +266,7 @@ int main(int argc, char *argv[]) ...@@ -256,6 +266,7 @@ int main(int argc, char *argv[])
out: out:
ls_free(ls_arr, ls_size); ls_free(ls_arr, ls_size);
lxc_free_array((void **)grps, free);
exit(ret); exit(ret);
} }
...@@ -308,7 +319,8 @@ static void ls_free_arr(char **arr, size_t size) ...@@ -308,7 +319,8 @@ static void ls_free_arr(char **arr, size_t size)
static int ls_get(struct ls **m, size_t *size, const struct lxc_arguments *args, static int ls_get(struct ls **m, size_t *size, const struct lxc_arguments *args,
const char *basepath, const char *parent, unsigned int lvl, const char *basepath, const char *parent, unsigned int lvl,
char **lockpath, size_t len_lockpath) char **lockpath, size_t len_lockpath, char **grps_must,
size_t grps_must_len)
{ {
/* As ls_get() is non-tail recursive we face the inherent danger of /* As ls_get() is non-tail recursive we face the inherent danger of
* blowing up the stack at some level of nesting. To have at least some * blowing up the stack at some level of nesting. To have at least some
...@@ -397,7 +409,7 @@ static int ls_get(struct ls **m, size_t *size, const struct lxc_arguments *args, ...@@ -397,7 +409,7 @@ static int ls_get(struct ls **m, size_t *size, const struct lxc_arguments *args,
bool running = c->is_running(c); bool running = c->is_running(c);
char *grp_tmp = ls_get_groups(c, running); char *grp_tmp = ls_get_groups(c, running);
if (!ls_has_all_grps(grp_tmp, args->groups)) { if (!ls_has_all_grps(grp_tmp, grps_must, grps_must_len)) {
free(grp_tmp); free(grp_tmp);
goto put_and_next; goto put_and_next;
} }
...@@ -477,6 +489,8 @@ static int ls_get(struct ls **m, size_t *size, const struct lxc_arguments *args, ...@@ -477,6 +489,8 @@ static int ls_get(struct ls **m, size_t *size, const struct lxc_arguments *args,
/* Send in the parent for the next nesting level. */ /* Send in the parent for the next nesting level. */
wargs.parent = l->name; wargs.parent = l->name;
wargs.args = args; wargs.args = args;
wargs.grps_must = grps_must;
wargs.grps_must_len = grps_must_len;
pid_t out; pid_t out;
...@@ -528,7 +542,7 @@ static int ls_get(struct ls **m, size_t *size, const struct lxc_arguments *args, ...@@ -528,7 +542,7 @@ static int ls_get(struct ls **m, size_t *size, const struct lxc_arguments *args,
if (ls_remove_lock(path, name, lockpath, &len_lockpath, true) == -1) if (ls_remove_lock(path, name, lockpath, &len_lockpath, true) == -1)
goto put_and_next; goto put_and_next;
ls_get(m, size, args, newpath, l->name, lvl + 1, lockpath, len_lockpath); ls_get(m, size, args, newpath, l->name, lvl + 1, lockpath, len_lockpath, grps_must, grps_must_len);
free(newpath); free(newpath);
/* Remove the lock. No need to check for failure here. */ /* Remove the lock. No need to check for failure here. */
...@@ -672,8 +686,10 @@ static unsigned int ls_get_term_width(void) ...@@ -672,8 +686,10 @@ static unsigned int ls_get_term_width(void)
return ws.ws_col; return ws.ws_col;
} }
static bool ls_has_all_grps(const char *has, const char *must) static bool ls_has_all_grps(const char *has, char **must, size_t must_len)
{ {
bool bret = false;
if (!has && must) if (!has && must)
return false; return false;
else if (!must) else if (!must)
...@@ -682,36 +698,25 @@ static bool ls_has_all_grps(const char *has, const char *must) ...@@ -682,36 +698,25 @@ static bool ls_has_all_grps(const char *has, const char *must)
char **tmp_has = lxc_string_split_and_trim(has, ','); char **tmp_has = lxc_string_split_and_trim(has, ',');
size_t tmp_has_len = lxc_array_len((void **)tmp_has); size_t tmp_has_len = lxc_array_len((void **)tmp_has);
char **tmp_must = lxc_string_split_and_trim(must, ','); /* Don't do any unnecessary work. */
size_t tmp_must_len = lxc_array_len((void **)tmp_must); if (must_len > tmp_has_len)
goto out;
if (tmp_must_len > tmp_has_len)
tmp_must_len = tmp_has_len = 0;
bool broke_out = false, ran_once = false; size_t i, j;
char **s, **t; for (i = 0; i < must_len; i++) {
/* Check if container has all relevant groups. */ for (j = 0; j < tmp_has_len; j++)
for (s = tmp_must; (tmp_must_len > 0) && (tmp_has_len > 0) && s && *s; s++) { if (strcmp(must[i], tmp_has[j]) == 0)
if (broke_out)
broke_out = false;
else if (!broke_out && ran_once)
break; break;
for (t = tmp_has; t && *t; t++) { if (j == tmp_has_len)
ran_once = true;
if (strcmp(*s, *t) == 0) {
broke_out = true;
break; break;
} }
} if (i == must_len)
} bret = true;
out:
lxc_free_array((void **)tmp_has, free); lxc_free_array((void **)tmp_has, free);
lxc_free_array((void **)tmp_must, free);
if (!broke_out) return bret;
return false;
return true;
} }
static struct ls *ls_new(struct ls **ls, size_t *size) static struct ls *ls_new(struct ls **ls, size_t *size)
...@@ -949,7 +954,7 @@ static int ls_get_wrapper(void *wrap) ...@@ -949,7 +954,7 @@ static int ls_get_wrapper(void *wrap)
/* &(char *){NULL} is no magic. It's just a compound literal which /* &(char *){NULL} is no magic. It's just a compound literal which
* avoids having a pointless variable in main() that serves no purpose * avoids having a pointless variable in main() that serves no purpose
* here. */ * here. */
ls_get(&m, &len, wargs->args, "", wargs->parent, wargs->nestlvl, &(char *){NULL}, 0); ls_get(&m, &len, wargs->args, "", wargs->parent, wargs->nestlvl, &(char *){NULL}, 0, wargs->grps_must, wargs->grps_must_len);
if (!m) if (!m)
goto out; goto out;
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment