On Mon, Dec 12, 2016 at 10:55:55AM +0000, Daniel P. Berrange wrote:
> On Wed, Dec 07, 2016 at 09:36:15AM +0100, Michal Privoznik wrote:
> > Prime time. When it comes to spawning qemu process and
> > relabelling all the devices it's going to touch, there's inherent
> > race with other applications in the system (e.g. udev). Instead
> > of trying convincing udev to not touch libvirt managed devices,
> > we can create a separate mount namespace for the qemu, and mount
> > our own /dev there. Of course this puts more work onto us as we
> > have to maintain /dev files on each domain start and device
> > hot(un-)plug. On the other hand, this enhances security also.
> >
> > >From technical POV, on domain startup process the parent
> > (libvirtd) creates:
> >
> > /var/lib/libvirt/qemu/$domain.dev
> > /var/lib/libvirt/qemu/$domain.devpts
> >
> > The child (which is going to be qemu eventually) calls unshare()
> > to create new mount namespace. From now on anything that child
> > does is invisible to the parent. Child then mounts tmpfs on
> > $domain.dev (so that it still sees original /dev from the host)
> > and creates some devices (as explained in one of the previous
> > patches). The devices have to be created exactly as they are in
> > the host (including perms, seclabels, ACLs, ...). After that it
> > moves $domain.dev mount to /dev.
> >
> > What's the $domain.devpts mount there for then you ask? QEMU can
> > create PTYs for some chardevs. And historically we exposed the
> > host ends in our domain XML allowing users to connect to them.
> > Therefore we must preserve devpts mount to be shared with the
> > host's one.
> >
> > To make this patch as small as possible, creating of devices
> > configured for domain in question is implemented in next patches.
> >
> > Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
> > ---
> > src/qemu/qemu_domain.c | 377 +++++++++++++++++++++++++++++++++++++++++++++++-
> > src/qemu/qemu_domain.h | 20 +++
> > src/qemu/qemu_process.c | 13 ++
> > 3 files changed, 408 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> > index 4aae14d9d..50b401f79 100644
> > --- a/src/qemu/qemu_domain.c
> > +++ b/src/qemu/qemu_domain.c
> > @@ -55,6 +55,9 @@
> >
> > #include <sys/time.h>
> > #include <fcntl.h>
> > +#if defined(HAVE_SYS_MOUNT_H)
> > +# include <sys/mount.h>
> > +#endif
> >
> > #include <libxml/xpathInternals.h>
> >
> > @@ -86,6 +89,10 @@ VIR_ENUM_IMPL(qemuDomainAsyncJob, QEMU_ASYNC_JOB_LAST,
> > "start",
> > );
> >
> > +VIR_ENUM_IMPL(qemuDomainNamespace, QEMU_DOMAIN_NS_LAST,
> > + "mount",
> > +);
> > +
> >
> > struct _qemuDomainLogContext {
> > int refs;
> > @@ -146,6 +153,31 @@ qemuDomainAsyncJobPhaseFromString(qemuDomainAsyncJob job,
> > }
> >
> >
> > +bool
> > +qemuDomainNamespaceEnabled(virDomainObjPtr vm,
> > + qemuDomainNamespace ns)
> > +{
> > + qemuDomainObjPrivatePtr priv = vm->privateData;
> > +
> > + return priv->namespaces &&
> > + virBitmapIsBitSet(priv->namespaces, ns);
> > +}
> > +
> > +
> > +static bool
> > +qemuDomainEnableNamespace(virDomainObjPtr vm,
> > + qemuDomainNamespace ns)
> > +{
> > + qemuDomainObjPrivatePtr priv = vm->privateData;
> > +
> > + if (!priv->namespaces &&
> > + !(priv->namespaces = virBitmapNew(QEMU_DOMAIN_NS_LAST)))
> > + return -1;
> > +
> > + return virBitmapSetBit(priv->namespaces, ns);
>
>
> Returning -1 from a method returning 'bool' is not what you want :-)
> Also causes a compile warning in the caller about comparing
> bool with '< 0'. Looking at the callers, I think you probably
> wanted
>
> @@ -169,7 +169,7 @@ qemuDomainNamespaceEnabled(virDomainObjPtr vm,
> }
>
>
> -static bool
> +static int
> qemuDomainEnableNamespace(virDomainObjPtr vm,
> qemuDomainNamespace ns)
> {
> @@ -179,7 +179,9 @@ qemuDomainEnableNamespace(virDomainObjPtr vm,
> !(priv->namespaces = virBitmapNew(QEMU_DOMAIN_NS_LAST)))
> return -1;
>
> - return virBitmapSetBit(priv->namespaces, ns);
> + virBitmapSetBit(priv->namespaces, ns);
> +
> + return 0;
> }
>
>
> ACK with such a change made.
Except that checking return value of virBitmapSetBit is also required.
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
