Re: [libvirt] PATCH: 7/7: Allow replacing root filesystem

6 Aug 2008


      "Daniel P. Berrange" <berrange@redhat.com> wrote:
...
...
-static int lxcContainerSetStdio(int control, const char *ttyPath)
+static int lxcContainerSetStdio(int control, int ttyfd)
 {
Hi Dan,

Not a big deal, but since ttyfd is now an input to this function,
it'd be less surprising if the caller were to close it.
Probably not worth changing...
...
int rc = -1;
-    int ttyfd;
     int open_max, i;
if (setsid() < 0) {
         lxcError(NULL, NULL, VIR_ERR_INTERNAL_ERROR,
                  _("setsid failed: %s"), strerror(errno));
-        goto error_out;
-    }
-
-    ttyfd = open(ttyPath, O_RDWR|O_NOCTTY);
-    if (ttyfd < 0) {
-        lxcError(NULL, NULL, VIR_ERR_INTERNAL_ERROR,
-                 _("open(%s) failed: %s"), ttyPath, strerror(errno));
-        goto error_out;
+        goto cleanup;
     }
if (ioctl(ttyfd, TIOCSCTTY, NULL) < 0) {
@@ -159,8 +161,6 @@
cleanup:
     close(ttyfd);
-
-error_out:
     return rc;
 }
@@ -223,6 +223,7 @@
     return 0;
 }
+
 /**
  * lxcEnableInterfaces:
  * @vm: Pointer to vm structure
@@ -252,6 +253,20 @@
     return rc;
 }
...
...
+    if (root) {
+        char *oldroot;
+        struct mntent *mntent;
+        char **mounts = NULL;
+        int nmounts = 0;
+        FILE *procmnt;
This can be "const":
...
+        struct {
+            int maj;
+            int min;
+            const char *path;
+        } devs[] = {
+            { 1, 3, "/dev/null" },
+            { 1, 5, "/dev/zero" },
+            { 1, 7, "/dev/full" },
+            { 5, 1, "/dev/console" },
Might be good to add /dev/random and /dev/urandom.
I recently had trouble on a system where udev malfunctioned,
and some /dev/{u,}random-requiring libs/services failed in unusual ways.

Also, how about adding permission bits to the table, so that
console doesn't end up being mode 0777:

      struct {
        int maj;
        int min;
        mode_t mode,
        const char *path;
      } const devs[] = {
        { 1, 3, 0666, "/dev/null" },
        { 1, 5, 0666, "/dev/zero" },
        { 1, 7, 0666, "/dev/full" },
        { 5, 1, 0600, "/dev/console" },
        { 1, 8, 0666, "/dev/random" },
        { 1, 9, 0666, "/dev/urandom" },
      };
...
+        if (virFileMakePath("/dev/pts") < 0 ||
+            mount("/.oldroot/dev/pts", "/dev/pts", NULL,
+                  MS_MOVE, NULL) < 0) {
+            lxcError(NULL, NULL, VIR_ERR_INTERNAL_ERROR,
+                     _("failed to move /dev/pts into container: %s"),
+                     strerror(errno));
+            return -1;
+        }
+
+        /* Populate /dev/ with a few important bits */
+        umask(0);
In principle, it's better never to change umask.  Otherwise, when
multi-threaded, that temporarily-cleared umask could hose a file-creation
operation in another thread -- and it'd be a race, so probably hard to
reproduce.
...
+        for (i = 0 ; i < ARRAY_CARDINALITY(devs) ; i++) {
+            dev_t dev = makedev(devs[i].maj, devs[i].min);
+            if (mknod(devs[i].path,
+                      0777 | S_IFCHR,
Dropping the umask, you might do s/0777/0/ here, and then
call chmod to set each mode to devs[i].mode
...
+                      dev) < 0) {
+                lxcError(NULL, NULL, VIR_ERR_INTERNAL_ERROR,
+                         _("failed to make device %s: %s"),
+                         devs[i].path, strerror(errno));
Returning here would leave umask set to 0.
Another reason not to change it in the first place.
...
+                return -1;
+            }
+        }
+        umask(0700);
When you do change umask, be sure to restore to the previous value.
...
+        /* Pull in rest of container's mounts */
+        for (tmp = vmDef->fss; tmp; tmp = tmp->next) {
+            char *src;
+            if (STREQ(tmp->dst, "/"))
+                continue;
+            // XXX fix
+            if (tmp->type != VIR_DOMAIN_FS_TYPE_MOUNT)
+                continue;
+
+            if (asprintf(&src, "/.oldroot/%s", tmp->src) < 0)
+                return -1;
+
+            if (virFileMakePath(tmp->dst) < 0 ||
+                mount(src, tmp->dst, NULL, MS_BIND, NULL) < 0) {
+                lxcError(NULL, NULL, VIR_ERR_INTERNAL_ERROR,
+                         _("failed to mount %s at %s for container: %s"),
+                         tmp->src, tmp->dst, strerror(errno));
Call VIR_FREE(src) here to avoid a leak.
...
+                return -1;
+            }
+            VIR_FREE(src);
+        }
+
+        if (!(procmnt = setmntent("/proc/mounts", "r"))) {
+            lxcError(NULL, NULL, VIR_ERR_INTERNAL_ERROR,
+                     _("failed to read /proc/mounts: %s"),
+                     strerror(errno));
+            return -1;
+        }
+        while ((mntent = getmntent(procmnt)) != NULL) {
+            if (!STRPREFIX(mntent->mnt_dir, "/.oldroot"))
+                continue;
+            if (VIR_REALLOC_N(mounts, nmounts+1) < 0)
Call endmntent(procmnt) here, to
avoid a potential file descriptor leak.
...
+                return -1;
...
+            mounts[nmounts++] = strdup(mntent->mnt_dir);
A failed strdup here would lead to segfault via qsort's comparator.
...
+        }
+        endmntent(procmnt);
+
+        qsort(mounts, nmounts, sizeof(mounts[0]),
+              lxcContainerChildMountSort);
+
+        for (i = 0 ; i < nmounts ; i++) {
+            if (umount(mounts[i]) < 0) {
+                lxcError(NULL, NULL, VIR_ERR_INTERNAL_ERROR,
+                         _("failed to unmount %s: %s"),
+                         mounts[i], strerror(errno));
+                return -1;
Maybe try to unmount all of them before returning, even if an
earlier one fails?

Also, be sure to free "mounts" before returning.
...
+            }
+        }
+    } else {
...

    

Re: [libvirt] PATCH: 7/7: Allow replacing root filesystem

Jim Meyering