On Tue, 18 Sep 2012 14:32:02 -0700 (PDT)
Christian Seiler <christ...@iwakd.de> wrote:
> Hi,
>
> > Do you think mallocing an fd_set and using FD_SET() and friends
> > would be better? The (dispose|finish) loops would visit FD_SETSIZE
> > bits with an FD_ISSET() test, which is more work than you have
> > currently with the early out, but we would probably save on the
> > initialization with FD_ZERO(). I don't know if
> > lxc_cgroup_(dispose|finish)_attach is performance critical.
>
> I don't think performance is that much of an issue here, but to me it
> seems that using fd_set logic would complicate things quite a bit
> unnecessarily. The current logic is already a bit complicated because
> the cgroup task files have to be opened before setns() but written to
> only after the fork() call when we know the pid which happens after
> setns(). Having a simple array with a loop over it appears to be much
> more straight-forward to me, especially since iterating over an fd_set
> is kind of convoluted.
>
> > Or I can just add a comment :)
>
> My suggestion would be to do just that unless someone has a good
> reason to change the current logic.
>
> All IMHO of course, I just wrote the initial patch, in the end other
> people get to decide what goes in. ;-)
>
> Regards,
> Christian
Just to make clear what I was suggesting, here is a patch. It may not
be any better/clearer than the original code, its less storage (1024
bytes vs 128) and handles more fd's (not that there will ever be that
many), but its more code.
diff --git a/src/lxc/cgroup.c b/src/lxc/cgroup.c
index a02ebc2..63eac93 100644
--- a/src/lxc/cgroup.c
+++ b/src/lxc/cgroup.c
@@ -31,6 +31,7 @@
#include <dirent.h>
#include <fcntl.h>
#include <sys/types.h>
+#include <sys/select.h>
#include <sys/stat.h>
#include <sys/param.h>
#include <sys/inotify.h>
@@ -322,8 +323,8 @@ static int lxc_one_cgroup_attach(const char *name, struct
mntent *mntent, pid_t
int lxc_cgroup_dispose_attach(void *data)
{
- int *fds = data;
- int ret, err;
+ fd_set *fds = data;
+ int fd, ret, err;
if (!fds) {
return 0;
@@ -331,10 +332,12 @@ int lxc_cgroup_dispose_attach(void *data)
ret = 0;
- for (; *fds >= 0; fds++) {
- err = lxc_one_cgroup_dispose_attach(*fds);
- if (err) {
- ret = err;
+ for (fd = 0; fd < FD_SETSIZE; fd++) {
+ if (FD_ISSET(fd, fds)) {
+ err = lxc_one_cgroup_dispose_attach(fd);
+ if (err) {
+ ret = err;
+ }
}
}
@@ -345,21 +348,22 @@ int lxc_cgroup_dispose_attach(void *data)
int lxc_cgroup_finish_attach(void *data, pid_t pid)
{
- int *fds = data;
- int err;
+ fd_set *fds = data;
+ int fd, err;
if (!fds) {
return 0;
}
- for (; *fds >= 0; fds++) {
- err = lxc_one_cgroup_finish_attach(*fds, pid);
- if (err) {
- /* get rid of the rest of them */
- lxc_cgroup_dispose_attach(data);
- return -1;
+ for (fd = 0; fd < FD_SETSIZE; fd++) {
+ if (FD_ISSET(fd, fds)) {
+ err = lxc_one_cgroup_finish_attach(fd, pid);
+ if (err) {
+ /* get rid of the rest of them */
+ lxc_cgroup_dispose_attach(data);
+ return -1;
+ }
}
- *fds = -1;
}
free(data);
@@ -373,9 +377,9 @@ int lxc_cgroup_prepare_attach(const char *name, void **data)
FILE *file = NULL;
int err = -1;
int found = 0;
- int *fds;
+ int fd;
+ fd_set *fds;
int i;
- static const int MAXFDS = 256;
file = setmntent(MTAB, "r");
if (!file) {
@@ -386,19 +390,17 @@ int lxc_cgroup_prepare_attach(const char *name, void
**data)
/* create a large enough buffer for all practical
* use cases
*/
- fds = malloc(sizeof(int) * MAXFDS);
+ fds = malloc(sizeof(*fds));
if (!fds) {
err = -1;
goto out;
}
- for (i = 0; i < MAXFDS; i++) {
- fds[i] = -1;
- }
+ FD_ZERO(fds);
err = 0;
i = 0;
while ((mntent = getmntent(file))) {
- if (i >= MAXFDS - 1) {
+ if (i >= FD_SETSIZE) {
ERROR("too many cgroups to attach to, aborting");
lxc_cgroup_dispose_attach(fds);
errno = ENOMEM;
@@ -416,12 +418,13 @@ int lxc_cgroup_prepare_attach(const char *name, void
**data)
INFO("[%d] found cgroup mounted at '%s',opts='%s'",
++found, mntent->mnt_dir, mntent->mnt_opts);
- fds[i] = lxc_one_cgroup_prepare_attach(name, mntent);
- if (fds[i] < 0) {
- err = fds[i];
+ fd = lxc_one_cgroup_prepare_attach(name, mntent);
+ if (fd < 0) {
+ err = fd;
lxc_cgroup_dispose_attach(fds);
goto out;
}
+ FD_SET(fd, fds);
i++;
};
------------------------------------------------------------------------------
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and
threat landscape has changed and how IT managers can respond. Discussions
will include endpoint security, mobile security and the latest in malware
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/
_______________________________________________
Lxc-devel mailing list
Lxc-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/lxc-devel
