[libvirt] [PATCH v4 06/14] qemu_ns: Allow /dev/mapper/control for PR

[Michal Privoznik]

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