On 04/10/2018 10:58 AM, Michal Privoznik wrote:
If qemu-pr-helper is compiled with multipath support the first thing it does is opening /dev/mapper/control. Since we're going
s/opening/open
to be running it inside qemu namespace we need to create it there. Unfortunately, we don't know if it was compiled with or without multipath so we have to create it anyway.
Not sure this explains whether it's multipath or namespaces that's the focus/cause of this patch. I thought multipath allows multiple ways to address the same LUN and namespace provides a mechanism to "control" device events and protections so that something like udev doesn't change something behind our back. For PR it's a mechanism to allow certain ioctl calls to be able to control ownership and/or write processing to the LUN so that it's not possible to have two writers. So does this patch add /dev/mapper/control to the namespace list because that's what will "control" the ioctl's to the LUN used by PR ???
BTW: This might be the ugliest piece of code I've ever written but @devMapperControl really needs to be type of char * otherwise some crazy check in VIR_APPEND_ELEMENT fails.
This hunk probably doesn't belong in a commit message... and well doesn't necessarily provide the confidence level for acceptance ;-)!
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 0856f04406..6fe4eb57e1 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -108,6 +108,7 @@ VIR_ENUM_IMPL(qemuDomainNamespace, QEMU_DOMAIN_NS_LAST, #define PROC_MOUNTS "/proc/mounts" #define DEVPREFIX "/dev/" #define DEV_VFIO "/dev/vfio/vfio" +#define DEVICE_MAPPER_CONTROL_PATH "/dev/mapper/control"
struct _qemuDomainLogContext { @@ -10269,6 +10270,11 @@ qemuDomainSetupDisk(virQEMUDriverConfigPtr cfg ATTRIBUTE_UNUSED, goto cleanup; }
+ /* qemu-pr-helper might require access to /dev/mapper/control. */ + if (virStoragePRDefIsEnabled(disk->src->pr) && + qemuDomainCreateDevice(DEVICE_MAPPER_CONTROL_PATH, data, true) < 0) + goto cleanup; + ret = 0; cleanup: VIR_FREE(dst); @@ -11281,6 +11287,9 @@ qemuDomainNamespaceSetupDisk(virDomainObjPtr vm, const char **paths = NULL; size_t npaths = 0; int ret = -1; + /* This is very nasty but we need it to work around some + * stupid checks in VIR_APPEND_ELEMENT macro. */ + char *devMapperControl = (char *) DEVICE_MAPPER_CONTROL_PATH;
alternatively char *dmPath = NULL;
if (!qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) return 0; @@ -11296,6 +11305,11 @@ qemuDomainNamespaceSetupDisk(virDomainObjPtr vm, goto cleanup; }
+ /* qemu-pr-helper might require access to /dev/mapper/control. */ + if (virStoragePRDefIsEnabled(src->pr) && + VIR_APPEND_ELEMENT_COPY(paths, npaths, devMapperControl) < 0)
Alternatively: VIR_STRDUP(dmPath, DEVICE_MAPPER_CONTROL_PATH) < 0) && VIR_APPEND_ELEMENT_COPY(paths, npaths, dmPath)
+ goto cleanup; + if (qemuDomainNamespaceMknodPaths(vm, paths, npaths) < 0) return -1;
BTW: Existing, but this leaks @paths
VIR_FREE(dmPath); John