Hi, Ok I take note about inline patches in email.
- About your version of lxc_for_each, your version is a lots better. My code was totally stupid and non-performant. In fact I based my code on something else that was needed the scandir, i simply copy/pasted by laziness. About the full path, in fact it was used only for the isdir. Again my code was really crappy. Lets start with yours. - About the errno = EINTR inside the for_each, the purpose was to report easily and generically when a callback stop the loop. But honestly, I dont care at all if that's up to the callback to set the errno, since I dont have any use of it in my code. I thought it was more generic and clean, beside that, again, I dont use it myself. Let me explain what I am trying to do, so you can probably drive me to the best solution with your deep understanding of LXC: Since I am implementing some statistics collection for LXC (liblxcstats, https://bitbucket.org/dotcloud/liblxcstats/src). What informations I am retrieving is: - # of registered containers - # of running containers - # of stopped containers At first I wasn't really concerned by counting running containers that are not registered, but hey! nothing prevent me to have more running containers than (registered - stopped) ones in my statistics. It would be more accurate in fact to avoid "forgetting" some running containers because they are not registered I guess. Maybe the best solution need to create two for_each style functions, one for registered containers, one for running ones? I let you tell me what's the best. Then, for each running containers, I am accessing to the cgroup file-system trough lxc_cgroup_get (to keep everything abstracted as much as possible by lxc) and collect all the statistics I need. So yes I need to browse every running containers, and I guess that even if the container is not registered but running, I can still collect all the statistics. Let me know what do you think! thanks a lots for your review On Tue, May 3, 2011 at 3:16 PM, Daniel Lezcano <daniel.lezc...@free.fr> wrote: > On 04/13/2011 07:49 PM, Francois-Xavier Bourlet wrote: >> >> Hi, >> >> Here's a patch with the purpose adding a way to browse containers trough >> liblxc. >> I added the function lxc-browse, that simply call back a function with >> the container name as parameter. >> It help to abstract how to browse containers without needed to know >> the underlaying structure of LXC. >> >> In case this path seem too big, at least something like the second >> path; that simply provide a way to retrieve LXCPATH; would be better >> than nothing, trough less abstracted. > > Please in the future, inline the patches. One mail per patch, that will be > easier to review. > > Thanks. > >> From 519012fa31068611096605447061bc90ffeef474 Mon Sep 17 00:00:00 2001 >> From: =?UTF-8?q?Fran=C3=A7ois-Xavier=20Bourlet?= <f...@dotcloud.com> >> Date: Wed, 13 Apr 2011 10:13:39 -0700 >> Subject: [PATCH 1/2] add lxc_browse function >> >> This function provide an interface to browse containers. It call a >> callback for every containers, giving the name as an argument. This >> function also return the number of containers browsed. >> --- >> src/lxc/Makefile.am | 3 +- >> src/lxc/lxc.h | 16 +++++++ >> src/lxc/lxc_browse.c | 108 >> ++++++++++++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 126 insertions(+), 1 deletions(-) >> create mode 100644 src/lxc/lxc_browse.c >> >> diff --git a/src/lxc/Makefile.am b/src/lxc/Makefile.am >> index df3d4dd..907b2c3 100644 >> --- a/src/lxc/Makefile.am >> +++ b/src/lxc/Makefile.am >> @@ -50,7 +50,8 @@ liblxc_so_SOURCES = \ >> mainloop.c mainloop.h \ >> af_unix.c af_unix.h \ >> \ >> - utmp.c utmp.h >> + utmp.c utmp.h \ >> + lxc_browse.c > > Actually the lxc_* are for the cli, so I suggest the rename the file > browse.c. > >> >> AM_CFLAGS=-I$(top_srcdir)/src >> >> diff --git a/src/lxc/lxc.h b/src/lxc/lxc.h >> index a091baa..b420010 100644 >> --- a/src/lxc/lxc.h >> +++ b/src/lxc/lxc.h >> @@ -161,6 +161,22 @@ extern int lxc_checkpoint(const char *name, int sfd, >> int flags); >> */ >> extern int lxc_restart(const char *, int, struct lxc_conf *, int); >> >> +/** >> + * @brief Browse containers >> + * >> + * Browse all registered containers. >> + * >> + * @param [in] cb A function pointer to a callback which takes a pointer >> to an >> + * user context and a pointer to lxcst structure. If not null, the >> callback >> + * will be called with the ctx as well as the container name. >> + * @param [in] ctx A pointer to an user defined variable which will be >> used as >> + * an argument to the callback. If the callback returns non zero >> + * lxcst_browse_containers stops and return with -1 with errno set to >> EINTR. >> + * >> + * @return the number of containers on success, -1 on error with errno >> set. >> + */ >> +int lxc_browse(int (*cb)(void *ctx, const char *name), void *ctx); >> + >> /* >> * Returns the version number of the library >> */ >> diff --git a/src/lxc/lxc_browse.c b/src/lxc/lxc_browse.c >> new file mode 100644 >> index 0000000..194ea7a >> --- /dev/null >> +++ b/src/lxc/lxc_browse.c >> @@ -0,0 +1,108 @@ >> +/* >> + * lxc: linux Container library >> + * >> + * (C) Copyright IBM Corp. 2007, 2010 >> + * >> + * Authors: >> + * Daniel Lezcano <dlezcano at fr.ibm.com> >> + * >> + * This library is free software; you can redistribute it and/or >> + * modify it under the terms of the GNU Lesser General Public >> + * License as published by the Free Software Foundation; either >> + * version 2.1 of the License, or (at your option) any later version. >> + * >> + * This library is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU >> + * Lesser General Public License for more details. >> + * >> + * You should have received a copy of the GNU Lesser General Public >> + * License along with this library; if not, write to the Free Software >> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 >> USA >> + */ >> + >> +#define _GNU_SOURCE >> +#include <stdio.h> >> +#include <stdlib.h> >> +#include <unistd.h> >> +#include <errno.h> >> +#include <dirent.h> >> +#include <string.h> >> +#include <sys/stat.h> >> +#include <err.h> >> +#undef _GNU_SOURCE >> + >> +#include "lxc.h" >> +#include "config.h" >> + >> +static int _unselect_current_and_parent(const struct dirent* d) >> +{ >> + return (strcmp(d->d_name, ".") && strcmp(d->d_name, "..")); >> +} >> + >> +static int _isdir(const char *path) >> +{ >> + struct stat sb; >> + >> + if (stat(path, &sb) == -1) { >> + warn("_isdir, can't stat path: %s", path); >> + return 0; >> + } >> + >> + return (sb.st_mode & S_IFDIR); >> +} > > These two static functions are not needed, see below. > >> + >> +static char *_join_path(char *dest, size_t size, >> + const char *left, const char *right) >> +{ >> + if (*right != '/') { >> + if (strlen(left) + 1 + strlen(right) > size) { >> + warn("_join_path, path overflow: %s + / + %s", left, right); >> + return 0; >> + } >> + strcpy(dest, left); >> + strcat(dest, "/"); >> + strcat(dest, right); >> + } else { >> + if (strlen(left) + strlen(right) > size) { >> + warn("_join_path, path overflow: %s", right); >> + return 0; >> + } >> + strcpy(dest, right); >> + } >> + return dest; >> +} >> >> +int lxc_browse(int (*cb)(void*, const char*), void *ctx) >> +{ >> + int i; >> + struct dirent **c_vec; >> + char buf[PATH_MAX]; >> + int count; >> + >> + count = scandir(LXCPATH, &c_vec, &_unselect_current_and_parent, >> &versionsort); >> + if (count == -1) >> + return (-1); >> + >> + for (i = 0; i < count; ++i) >> + { >> + if (_isdir( >> + _join_path(buf, sizeof(buf), LXCPATH, >> c_vec[i]->d_name)) >> + ) >> + { >> + if (cb(ctx, c_vec[i]->d_name)) { >> + errno = EINTR; >> + goto abort_by_cb; >> + } >> + } >> + free(c_vec[i]); >> + } >> + free(c_vec); >> + return count; >> + >> +abort_by_cb: >> + for (; i < count; ++i) >> + free(c_vec[i]); >> + free(c_vec); >> + return -1; >> +} > > > This function could be rewritten in a simpler way. It kills the 'isdir', > 'joinpath' and '_unselect_current_and_parent'. > The errno checking with EINTR should be checked within the callback, not > outside. This is why I removed the test. > It is preferable to let the callback to build the absolute path if it needs > it, so only the container name is added. > > By the way, this function will browse only the created container not the > running ones. Is it what you expect François-Xavier ? Or do you need also > the running containers ? I ask you that because you can have running > container without being present in the LXCPATH directory. > > int lxc_for_each(int (*callback)(const char *name, void *data)) > { > DIR *dir; > struct dirent dirent, *direntp; > int ret = 0; > int nr = 0; > > dir = opendir(LXCPATH); > if (!dir) > return -1; > > while (!readdir_r(dir, &dirent, &direntp)) { > > if (!direntp) > break; > > if (direntp->d_name[0] == '.') > continue; > > if (direntp->d_type != DT_DIR) > continue; > > ret = callback(direntp->d_name, data); > if (ret < 0) > goto out; > > nr++; > } > > ret = nr; > out: > closedir(dir); > > return ret; > } > > >> From aad1b3ccbda4c4731b2bb689f05a9607755baf11 Mon Sep 17 00:00:00 2001 >> From: =?UTF-8?q?Fran=C3=A7ois-Xavier=20Bourlet?= <f...@dotcloud.com> >> Date: Wed, 13 Apr 2011 10:13:46 -0700 >> Subject: [PATCH 2/2] add lxc_path() >> >> This function simply return the internal build configuration value >> LXCPATH. >> --- >> src/lxc/conf.c | 5 +++++ >> src/lxc/conf.h | 7 +++++++ >> 2 files changed, 12 insertions(+), 0 deletions(-) >> >> diff --git a/src/lxc/conf.c b/src/lxc/conf.c >> index ae5b259..9a3d8d2 100644 >> --- a/src/lxc/conf.c >> +++ b/src/lxc/conf.c >> @@ -1748,3 +1748,8 @@ int lxc_setup(const char *name, struct lxc_conf >> *lxc_conf) >> >> return 0; >> } >> + >> +const char* lxc_path() >> +{ >> + return LXCPATH; >> +} >> diff --git a/src/lxc/conf.h b/src/lxc/conf.h >> index 712cb3a..59 > > > -- François-Xavier Bourlet ------------------------------------------------------------------------------ WhatsUp Gold - Download Free Network Management Software The most intuitive, comprehensive, and cost-effective network management toolset available today. Delivers lowest initial acquisition cost and overall TCO of any competing solution. http://p.sf.net/sfu/whatsupgold-sd _______________________________________________ Lxc-devel mailing list Lxc-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/lxc-devel
