If qemu-pr-helper is compiled with multipath support the first
thing it does is opening /dev/mapper/control. Since we're going
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.

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.

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;
 
     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)
+        goto cleanup;
+
     if (qemuDomainNamespaceMknodPaths(vm, paths, npaths) < 0)
         return -1;
 
-- 
2.16.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 04/14/2018 02:15 PM, John Ferlan wrote:
> 
> 
> 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 ???

No. It's because if compiled with multipath support, qemu-pr-helper uses
/dev/mapper/control to query devmapper version (dm_init()):

https://git.qemu.org/?p=qemu.git;a=blob;f=scsi/qemu-pr-helper.c;h=d0f83176e1a63547a28282849c228cf2e7acd5a2;hb=HEAD#l1032

/dev/mapper/control is used to talk to devmapper (kernel) - in this
particular case to check if given device is a multipath target, not to
manipulate individual devices. Because if a device is multipath target
then lowlevel stuff looks different (see do_pr_in() and do_pr_out()).

Anyway, it shouldn't matter what pr-helper needs /dev/mapper/control
for. It does so we need to create it in the namespace and allow the
helper to access it.

> 
>> 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 ;-)!

It's not because of allowing /dev/mapper/control but because of typecast
from const char * to char *.

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

Okay, I'll do this. It's cleaner.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list