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

[Daniel P. Berrange]

On Wed, Aug 06, 2008 at 11:00:05AM +0200, Jim Meyering wrote:
> "Daniel P. Berrange" <berrange at 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...

Yes, that does make sense actually - I'll change it.

> > +        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:

Yes, that's a good idea. I'll add permissions & random/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.

In this particular scenario changing umask is safe because this
code is running inside the container namespace in PID 1 - which
at this point is the container's equivalent of the initrd, just
doing some basic setup before running the 'init' process.

> > +        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

Yep, if we include permissions in the table of devs, then we
need to chmod anyway, so can do away with changing umaks anyway

> > +        }
> > +        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.

In general there's no need todo that cleanup in this particular method,
because if any part of this initialization fails, the container will
exit and the kernel will cleanup all the mounts, and of course memory
will be free'd. That said I'll add the free of 'mounts' anyway just
as good practice.


Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|