The following pull request was submitted through Github. It can be accessed and reviewed at: https://github.com/lxc/lxcfs/pull/78
This e-mail was sent by the LXC bot, direct replies will not reach the author unless they happen to be subscribed to this list. === Description (from pull-request) === The first 3 commits are pretty straight forward. As for the last one I don't know whether this kind of reorganization is desired by you at all. Just let me know.
From e390800e6f8140ac85ed9bda0f657f55e9203ede Mon Sep 17 00:00:00 2001 From: Wolfgang Bumiller <[email protected]> Date: Fri, 5 Feb 2016 11:50:32 +0100 Subject: [PATCH 1/4] bindings: return value type fixup Signed-off-by: Wolfgang Bumiller <[email protected]> --- bindings.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bindings.c b/bindings.c index 4bb0631..5394b44 100644 --- a/bindings.c +++ b/bindings.c @@ -665,7 +665,7 @@ bool cgfs_list_children(const char *controller, const char *cgroup, char ***list (*list)[0] = NULL; if (!tmpc) - return NULL; + return false; /* basedir / tmpc / cgroup \0 */ len = strlen(basedir) + strlen(tmpc) + strlen(cgroup) + 3; @@ -815,7 +815,7 @@ bool cgfs_list_keys(const char *controller, const char *cgroup, struct cgfs_file *keys = NULL; if (!tmpc) - return NULL; + return false; /* basedir / tmpc / cgroup \0 */ len = strlen(basedir) + strlen(tmpc) + strlen(cgroup) + 3; From 620525f6c82955fc0a5ae62e1f51374d9a22f3cb Mon Sep 17 00:00:00 2001 From: Wolfgang Bumiller <[email protected]> Date: Fri, 5 Feb 2016 12:10:15 +0100 Subject: [PATCH 2/4] bindings: even more concise must_strcat_pid We already assume tmp[] is big enough when using an unsized sprintf(), considering it contains a single pid number and is 30 bytes we can assume it was also big enough to hold the terminating null byte. Signed-off-by: Wolfgang Bumiller <[email protected]> --- bindings.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/bindings.c b/bindings.c index 5394b44..6e6be6e 100644 --- a/bindings.c +++ b/bindings.c @@ -1052,9 +1052,8 @@ static void must_strcat_pid(char **src, size_t *sz, size_t *asz, pid_t pid) *src = tmp; *asz += BUF_RESERVE_SIZE; } - memcpy((*src) +*sz , tmp, tmplen); + memcpy((*src) +*sz , tmp, tmplen+1); /* include the \0 */ *sz += tmplen; - (*src)[*sz] = '\0'; } /* From 9ef33e4f52d23e102c911f4658eaeb45aa70e616 Mon Sep 17 00:00:00 2001 From: Wolfgang Bumiller <[email protected]> Date: Fri, 5 Feb 2016 12:30:17 +0100 Subject: [PATCH 3/4] tests: don't actually install liblxcfstest.so Signed-off-by: Wolfgang Bumiller <[email protected]> --- Makefile.am | 3 ++- tests/test_reload.sh | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/Makefile.am b/Makefile.am index 734fa3b..c835d56 100644 --- a/Makefile.am +++ b/Makefile.am @@ -27,7 +27,8 @@ liblxcfstest_la_LDFLAGS = $(AM_CFLAGS) -shared \ noinst_HEADERS = bindings.h sodir=$(libdir) -lib_LTLIBRARIES = liblxcfs.la liblxcfstest.la +lib_LTLIBRARIES = liblxcfs.la +EXTRA_LTLIBRARIES = liblxcfstest.la lxcfs_SOURCES = lxcfs.c lxcfs_LDADD = liblxcfs.la -ldl diff --git a/tests/test_reload.sh b/tests/test_reload.sh index 4560ba2..e0fd605 100755 --- a/tests/test_reload.sh +++ b/tests/test_reload.sh @@ -37,7 +37,8 @@ cleanup() { trap cleanup EXIT SIGHUP SIGINT SIGTERM -( cd ${topdir}; DESTDIR=${installdir} make install ) +( cd ${topdir}; DESTDIR=${installdir} make install) +( cd ${topdir}; make liblxcfstest.la && cp .libs/liblxcfstest.* "${libdir}" ) export LD_LIBRARY_PATH=${libdir} ${bindir}/lxcfs -p ${pidfile} ${testdir} & From 0d91d28d92431f75b4e6fbbe5e9916eedc41eedc Mon Sep 17 00:00:00 2001 From: Wolfgang Bumiller <[email protected]> Date: Fri, 5 Feb 2016 11:52:23 +0100 Subject: [PATCH 4/4] bindings: avoid allocating an unused buffer cgfs_list_children() and cgfs_list_keys() follow the same pattern with the differences being that one lists directories, the other files, and that cgfs_list_children() always allocates an empty list while cgfs_list_keys() NULL-initializes the list. Both have a case which returns an error after a list has been allocated, and in both cases the cleanup code is guarded with an if(list). In both cases on success the caller assumes the list is non-empty which is why cgfs_list_children() returned a list with a terminating NULL-entry. This deduplicates the iteration code into a function with a flag for whether regular files or directories are of interest and a callback to create the list element. Signed-off-by: Wolfgang Bumiller <[email protected]> --- bindings.c | 126 ++++++++++++++++++++----------------------------------------- 1 file changed, 41 insertions(+), 85 deletions(-) diff --git a/bindings.c b/bindings.c index 6e6be6e..c093a7e 100644 --- a/bindings.c +++ b/bindings.c @@ -649,21 +649,19 @@ FILE *open_pids_file(const char *controller, const char *cgroup) return fopen(pathname, "w"); } -bool cgfs_list_children(const char *controller, const char *cgroup, char ***list) +static bool cgfs_iterate_cgroup(const char *controller, const char *cgroup, bool directories, + void ***list, size_t typesize, + void* (*iterator)(const char*, const char*, const char*)) { size_t len; char *dirname, *tmpc = find_mounted_controller(controller); char pathname[MAXPATHLEN]; - size_t sz = 0, asz = BATCH_SIZE; + size_t sz = 0, asz = 0; struct dirent dirent, *direntp; DIR *dir; int ret; - do { - *list = malloc(asz * sizeof(char *)); - } while (!*list); - (*list)[0] = NULL; - + *list = NULL; if (!tmpc) return false; @@ -698,20 +696,19 @@ bool cgfs_list_children(const char *controller, const char *cgroup, char ***list fprintf(stderr, "%s: failed to stat %s: %s\n", __func__, pathname, strerror(errno)); continue; } - if (!S_ISDIR(mystat.st_mode)) + if ((!directories && !S_ISREG(mystat.st_mode)) || + (directories && !S_ISDIR(mystat.st_mode))) continue; if (sz+2 >= asz) { - char **tmp; + void **tmp; asz += BATCH_SIZE; do { - tmp = realloc(*list, asz * sizeof(char *)); + tmp = realloc(*list, asz * typesize); } while (!tmp); *list = tmp; } - do { - (*list)[sz] = strdup(direntp->d_name); - } while (!(*list)[sz]); + (*list)[sz] = (*iterator)(controller, cgroup, direntp->d_name); (*list)[sz+1] = NULL; sz++; } @@ -722,6 +719,20 @@ bool cgfs_list_children(const char *controller, const char *cgroup, char ***list return true; } +static void *make_children_list_entry(const char *controller, const char *cgroup, const char *dir_entry) +{ + char *dup; + do { + dup = strdup(dir_entry); + } while (!dup); + return dup; +} + +bool cgfs_list_children(const char *controller, const char *cgroup, char ***list) +{ + return cgfs_iterate_cgroup(controller, cgroup, true, (void***)list, sizeof(*list), &make_children_list_entry); +} + void free_key(struct cgfs_files *k) { if (!k) @@ -803,76 +814,19 @@ struct cgfs_files *cgfs_get_key(const char *controller, const char *cgroup, cons return newkey; } -bool cgfs_list_keys(const char *controller, const char *cgroup, struct cgfs_files ***keys) +static void *make_key_list_entry(const char *controller, const char *cgroup, const char *dir_entry) { - size_t len; - char *dirname, *tmpc = find_mounted_controller(controller); - char pathname[MAXPATHLEN]; - size_t sz = 0, asz = 0; - struct dirent dirent, *direntp; - DIR *dir; - int ret; - - *keys = NULL; - if (!tmpc) - return false; - - /* basedir / tmpc / cgroup \0 */ - len = strlen(basedir) + strlen(tmpc) + strlen(cgroup) + 3; - dirname = alloca(len); - snprintf(dirname, len, "%s/%s/%s", basedir, tmpc, cgroup); - - dir = opendir(dirname); - if (!dir) - return false; - - while (!readdir_r(dir, &dirent, &direntp)) { - struct stat mystat; - int rc; - - if (!direntp) - break; - - if (!strcmp(direntp->d_name, ".") || - !strcmp(direntp->d_name, "..")) - continue; - - rc = snprintf(pathname, MAXPATHLEN, "%s/%s", dirname, direntp->d_name); - if (rc < 0 || rc >= MAXPATHLEN) { - fprintf(stderr, "%s: pathname too long under %s\n", __func__, dirname); - continue; - } - - ret = lstat(pathname, &mystat); - if (ret) { - fprintf(stderr, "%s: failed to stat %s: %s\n", __func__, pathname, strerror(errno)); - continue; - } - if (!S_ISREG(mystat.st_mode)) - continue; - - if (sz+2 >= asz) { - struct cgfs_files **tmp; - asz += BATCH_SIZE; - do { - tmp = realloc(*keys, asz * sizeof(struct cgfs_files *)); - } while (!tmp); - *keys = tmp; - } - (*keys)[sz] = cgfs_get_key(controller, cgroup, direntp->d_name); - (*keys)[sz+1] = NULL; - if (!(*keys)[sz]) { - fprintf(stderr, "%s: Error getting files under %s:%s\n", - __func__, controller, cgroup); - continue; - } - sz++; + struct cgfs_files *entry = cgfs_get_key(controller, cgroup, dir_entry); + if (!entry) { + fprintf(stderr, "%s: Error getting files under %s:%s\n", + __func__, controller, cgroup); } - if (closedir(dir) < 0) { - fprintf(stderr, "%s: failed closedir for %s: %s\n", __func__, dirname, strerror(errno)); - return false; - } - return true; + return entry; +} + +bool cgfs_list_keys(const char *controller, const char *cgroup, struct cgfs_files ***keys) +{ + return cgfs_iterate_cgroup(controller, cgroup, false, (void***)keys, sizeof(*keys), &make_key_list_entry); } bool is_child_cgroup(const char *controller, const char *cgroup, const char *f) @@ -1695,10 +1649,12 @@ int cg_readdir(const char *path, void *buf, fuse_fill_dir_t filler, off_t offset ret = 0; goto out; } - for (i = 0; clist[i]; i++) { - if (filler(buf, clist[i], NULL, 0) != 0) { - ret = -EIO; - goto out; + if (clist) { + for (i = 0; clist[i]; i++) { + if (filler(buf, clist[i], NULL, 0) != 0) { + ret = -EIO; + goto out; + } } } ret = 0;
_______________________________________________ lxc-devel mailing list [email protected] http://lists.linuxcontainers.org/listinfo/lxc-devel
