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@xxxxxxxxxx> >> --- >> 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@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
