[lxc-devel] [PATCH 1/2] fix trivial off by one error

[Dwight Engen]

On Tue, 18 Sep 2012 14:32:02 -0700 (PDT)
Christian Seiler <christian at 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++;
 	};