"Daniel P. Berrange" <berrange@xxxxxxxxxx> 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 {
...
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list