On 12.12.2016 12:47, Daniel P. Berrange wrote:
> 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
Btw: my compiler does not warn me. Even with -Wextra :-(
>>
>> @@ -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.
Okay, I'm sticking with:
- return virBitmapSetBit(priv->namespaces, ns);
+ if (virBitmapSetBit(priv->namespaces, ns) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Unable to enable namespace: %s"),
+ qemuDomainNamespaceTypeToString(ns));
+ return -1;
+ }
+
+ return 0;
Which makes sense from another POV: if virBitmapNew() fails, it also
reports error, however virBitmapSetBit() does not. So upon failure
qemuDomainEnableNamespace() might and might not reported error.
Michal
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
