[libvirt] [PATCH v5 05/11] qemu_ns: Allow /dev/mapper/control for PR

[Michal Privoznik]

On 04/29/2018 02:20 PM, John Ferlan wrote:
> 
> 
> On 04/23/2018 09:14 AM, Michal Privoznik wrote:
>> If qemu-pr-helper is compiled with multipath support the first
>> thing it does is open /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.
>>
>> 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 53827d5396..5ccdeb8e3a 100644
>> --- a/src/qemu/qemu_domain.c
>> +++ b/src/qemu/qemu_domain.c
>> @@ -109,6 +109,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 {
>> @@ -10207,6 +10208,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;
>> +
> 
> This is from SetupAllDisks via qemuDomainBuildNamespace...  Does it or
> could it matter that more than one disk would be doing this? Thus having
> multiple entries?  Was too lazy to check.

It doesn't matter. The code works well with already existing files. It
has to. Imagine two disks, both with the same backing file:

  diskA = /dev/diskA -> /dev/diskBacking
  diskB = /dev/diskB -> /dev/diskBacking

Now when diskA is created in the namespace both diskA and diskBacking
files are created. Then, when diskB is created we can't fail if
diskBacking already exists. Therefore the code is written so that it
accepts pre-existing files.

> 
>>      ret = 0;
>>   cleanup:
>>      VIR_FREE(dst);
>> @@ -11218,6 +11224,7 @@ qemuDomainNamespaceSetupDisk(virDomainObjPtr vm,
>>      virStorageSourcePtr next;
>>      const char **paths = NULL;
>>      size_t npaths = 0;
>> +    char *dmPath = NULL;
>>      int ret = -1;
>>  
>>      if (!qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT))
>> @@ -11234,11 +11241,18 @@ qemuDomainNamespaceSetupDisk(virDomainObjPtr vm,
>>              goto cleanup;
>>      }
>>  
>> +    /* qemu-pr-helper might require access to /dev/mapper/control. */
>> +    if (virStoragePRDefIsEnabled(src->pr) &&
>> +        (VIR_STRDUP(dmPath, DEVICE_MAPPER_CONTROL_PATH) < 0 ||
>> +         VIR_APPEND_ELEMENT_COPY(paths, npaths, dmPath) < 0))
>> +        goto cleanup;
>> +
> 
> ... similarly when we add/hotplug a disk we could be repeating that path.
> 
> If the duplication is OK or handled in some way that wasn't obvious,

Take a look at qemuDomainCreateDevice() and search for errno == EEXIST
lines. They are always followed with ret = 0;

> 
> Reviewed-by: John Ferlan <jferlan at redhat.com>
> 

Thanks,
Michal