This is a definition that holds information on SCSI persistent reservation settings. The XML part looks like this: <reservations enabled='yes' managed='no'> <source type='unix' path='/path/to/qemu-pr-helper.sock' mode='client'/> </reservations> If @managed is set to 'yes' then the <source/> is not parsed. This design was agreed on here: https://www.redhat.com/archives/libvir-list/2017-November/msg01005.html Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- docs/formatdomain.html.in | 25 +++- docs/schemas/domaincommon.rng | 19 +-- docs/schemas/storagecommon.rng | 34 +++++ src/conf/domain_conf.c | 36 +++++ src/libvirt_private.syms | 3 + src/util/virstoragefile.c | 148 +++++++++++++++++++++ src/util/virstoragefile.h | 15 +++ .../disk-virtio-scsi-reservations-not-managed.xml | 40 ++++++ .../disk-virtio-scsi-reservations.xml | 38 ++++++ .../disk-virtio-scsi-reservations-not-managed.xml | 1 + .../disk-virtio-scsi-reservations.xml | 1 + tests/qemuxml2xmltest.c | 4 + 12 files changed, 348 insertions(+), 16 deletions(-) create mode 100644 tests/qemuxml2argvdata/disk-virtio-scsi-reservations-not-managed.xml create mode 100644 tests/qemuxml2argvdata/disk-virtio-scsi-reservations.xml create mode 120000 tests/qemuxml2xmloutdata/disk-virtio-scsi-reservations-not-managed.xml create mode 120000 tests/qemuxml2xmloutdata/disk-virtio-scsi-reservations.xml diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index d272cc1ba..23c5f071e 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -2457,7 +2457,10 @@ &lt;/disk&gt; &lt;disk type='block' device='lun'&gt; &lt;driver name='qemu' type='raw'/&gt; - &lt;source dev='/dev/sda'/&gt; + &lt;source dev='/dev/sda'&gt; + &lt;reservations enabled='yes' managed='no'&gt; + &lt;source type='unix' path='/path/to/qemu-pr-helper' mode='client'/&gt; + &lt;/reservations&gt; &lt;target dev='sda' bus='scsi'/&gt; &lt;address type='drive' controller='0' bus='0' target='3' unit='0'/&gt; &lt;/disk&gt; @@ -2819,6 +2822,26 @@ <a href="formatstorageencryption.html">Storage Encryption</a> page for more information. </dd> + <dt><code>reservations</code></dt> + <dd><span class="since">Since libvirt 3.10.0</span>, the + <code>reservations</code> can be a sub-element of the + <code>source</code> element for storage sources (QEMU driver only). + If present (and enabled) it enabled persistent reservations for + SCSI based disks. The element has one mandatory attribute + <code>enabled</code> with accepted values <code>yes</code> and + <code>no</code>. If the feature is enabled, then there's another + mandatory attribute <code>managed</code> (accepted values are the + same as for <code>enabled</code>) that enables or disables libvirt + spawning any helper process (should one be needed). However, if + libvirt is not enabled spawning helper process (i.e. hypervisor + should just use one which is already running), path to its socket + must be provided in child element <code>source</code>, which + currently accepts only the following attributes: <code>type</code> + with one value <code>unix</code>, <code>path</code> with path the + socket, and finally <code>mode</code> which accepts either + <code>server</code> or <code>client</code> and specifies the role + of hypervisor. + </dd> </dl> <p> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index f22c932f6..884081bd7 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1498,6 +1498,9 @@ <optional> <ref name="encryption"/> </optional> + <optional> + <ref name="reservations"/> + </optional> <zeroOrMore> <ref name='devSeclabel'/> </zeroOrMore> @@ -2447,21 +2450,7 @@ <value>vhostuser</value> </attribute> <interleave> - <element name="source"> - <attribute name="type"> - <value>unix</value> - </attribute> - <attribute name="path"> - <ref name="absFilePath"/> - </attribute> - <attribute name="mode"> - <choice> - <value>server</value> - <value>client</value> - </choice> - </attribute> - <empty/> - </element> + <ref name="unixSocketSource"/> <ref name="interface-options"/> </interleave> </group> diff --git a/docs/schemas/storagecommon.rng b/docs/schemas/storagecommon.rng index edee1b084..9071667b0 100644 --- a/docs/schemas/storagecommon.rng +++ b/docs/schemas/storagecommon.rng @@ -39,6 +39,40 @@ </element> </define> + <define name='unixSocketSource'> + <element name="source"> + <attribute name="type"> + <value>unix</value> + </attribute> + <attribute name="path"> + <ref name="absFilePath"/> + </attribute> + <attribute name="mode"> + <choice> + <value>server</value> + <value>client</value> + </choice> + </attribute> + <empty/> + </element> + </define> + + <define name='reservations'> + <element name='reservations'> + <attribute name='enabled'> + <ref name='virYesNo'/> + </attribute> + <optional> + <attribute name='managed'> + <ref name='virYesNo'/> + </attribute> + </optional> + <optional> + <ref name='unixSocketSource'/> + </optional> + </element> + </define> + <define name='secret'> <element name='secret'> <attribute name='type'> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index a1c25060f..222c6dd15 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5163,6 +5163,13 @@ virDomainDiskDefValidate(const virDomainDiskDef *disk) } } + if (disk->src->pr && + disk->device != VIR_DOMAIN_DISK_DEVICE_LUN) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("<reservations/> allowed only for lun disks")); + return -1; + } + /* Reject disks with a bus type that is not compatible with the * given address type. The function considers only buses that are * handled in common code. For other bus types it's not possible @@ -8579,6 +8586,29 @@ virDomainDiskSourcePrivateDataParse(xmlXPathContextPtr ctxt, } +static int +virDomainDiskSourcePRParse(xmlNodePtr node, + virStoragePRDefPtr *prsrc) +{ + xmlNodePtr child; + virStoragePRDefPtr pr = NULL; + + for (child = node->children; child; child = child->next) { + if (child->type == XML_ELEMENT_NODE && + virXMLNodeNameEqual(child, "reservations")) { + + if (!(pr = virStoragePRDefParseNode(node->doc, child))) + return -1; + + *prsrc = pr; + return 0; + } + } + + return 0; +} + + int virDomainDiskSourceParse(xmlNodePtr node, xmlXPathContextPtr ctxt, @@ -8623,6 +8653,9 @@ virDomainDiskSourceParse(xmlNodePtr node, if (virDomainDiskSourceEncryptionParse(node, &src->encryption) < 0) goto cleanup; + if (virDomainDiskSourcePRParse(node, &src->pr) < 0) + goto cleanup; + if (virDomainDiskSourcePrivateDataParse(ctxt, src, flags, xmlopt) < 0) goto cleanup; @@ -22519,6 +22552,9 @@ virDomainDiskSourceFormatInternal(virBufferPtr buf, virStorageEncryptionFormat(&childBuf, src->encryption) < 0) return -1; + if (src->pr) + virStoragePRDefFormat(&childBuf, src->pr); + if (virDomainDiskSourceFormatPrivateData(&childBuf, src, flags, xmlopt) < 0) return -1; diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index bc8cc1fba..b0dc76b91 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2717,6 +2717,9 @@ virStorageNetHostDefFree; virStorageNetHostTransportTypeFromString; virStorageNetHostTransportTypeToString; virStorageNetProtocolTypeToString; +virStoragePRDefFormat; +virStoragePRDefFree; +virStoragePRDefParseNode; virStorageSourceBackingStoreClear; virStorageSourceClear; virStorageSourceCopy; diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 5780180a9..fda6617f7 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1913,6 +1913,153 @@ virStorageAuthDefFormat(virBufferPtr buf, } +void +virStoragePRDefFree(virStoragePRDefPtr prd) +{ + if (!prd) + return; + + VIR_FREE(prd->path); + VIR_FREE(prd); +} + + +static virStoragePRDefPtr +virStoragePRDefParseXML(xmlXPathContextPtr ctxt) +{ + virStoragePRDefPtr prd, ret = NULL; + char *enabled = NULL; + char *managed = NULL; + char *type = NULL; + char *path = NULL; + char *mode = NULL; + + if (VIR_ALLOC(prd) < 0) + return NULL; + + if (!(enabled = virXPathString("string(./@enabled)", ctxt))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing @enabled attribute for <reservations/>")); + goto cleanup; + } + + if ((prd->enabled = virTristateBoolTypeFromString(enabled)) <= 0) { + virReportError(VIR_ERR_XML_ERROR, + _("invalid value for 'enabled': %s"), NULLSTR(enabled)); + goto cleanup; + } + + if (prd->enabled == VIR_TRISTATE_BOOL_YES) { + managed = virXPathString("string(./@managed)", ctxt); + if ((prd->managed = virTristateBoolTypeFromString(managed)) <= 0) { + virReportError(VIR_ERR_XML_ERROR, + _("invalid value for 'managed': %s"), NULLSTR(managed)); + goto cleanup; + } + + if (prd->managed == VIR_TRISTATE_BOOL_NO) { + type = virXPathString("string(./source[1]/@type)", ctxt); + path = virXPathString("string(./source[1]/@path)", ctxt); + mode = virXPathString("string(./source[1]/@mode)", ctxt); + + if (!type) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing connection type")); + goto cleanup; + } + + if (!path) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing path")); + goto cleanup; + } + + if (!mode) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing connection mode")); + goto cleanup; + } + + if (STRNEQ(type, "unix")) { + virReportError(VIR_ERR_XML_ERROR, + _("unsupported type: %s"), type); + goto cleanup; + } + + if (STRNEQ(mode, "client")) { + virReportError(VIR_ERR_XML_ERROR, + _("unsupported mode: %s"), mode); + goto cleanup; + } + + prd->path = path; + path = NULL; + + } + } + + ret = prd; + prd = NULL; + + cleanup: + VIR_FREE(mode); + VIR_FREE(path); + VIR_FREE(type); + VIR_FREE(managed); + VIR_FREE(enabled); + virStoragePRDefFree(prd); + return ret; +} + + +virStoragePRDefPtr +virStoragePRDefParseNode(xmlDocPtr xml, + xmlNodePtr root) +{ + xmlXPathContextPtr ctxt = NULL; + virStoragePRDefPtr prd = NULL; + + ctxt = xmlXPathNewContext(xml); + if (ctxt == NULL) { + virReportOOMError(); + goto cleanup; + } + + ctxt->node = root; + prd = virStoragePRDefParseXML(ctxt); + + cleanup: + xmlXPathFreeContext(ctxt); + return prd; +} + + +void +virStoragePRDefFormat(virBufferPtr buf, + virStoragePRDefPtr prd) +{ + virBufferAsprintf(buf, "<reservations enabled='%s'", + virTristateBoolTypeToString(prd->enabled)); + if (prd->enabled == VIR_TRISTATE_BOOL_YES) { + virBufferAsprintf(buf, " managed='%s'", + virTristateBoolTypeToString(prd->managed)); + if (prd->managed == VIR_TRISTATE_BOOL_NO) { + virBufferAddLit(buf, ">\n"); + virBufferAdjustIndent(buf, 2); + virBufferAddLit(buf, "<source type='unix'"); + virBufferEscapeString(buf, " path='%s'", prd->path); + virBufferAddLit(buf, " mode='client'/>\n"); + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</reservations>\n"); + } else { + virBufferAddLit(buf, "/>\n"); + } + } else { + virBufferAddLit(buf, "/>\n"); + } +} + + virSecurityDeviceLabelDefPtr virStorageSourceGetSecurityLabelDef(virStorageSourcePtr src, const char *model) @@ -2291,6 +2438,7 @@ virStorageSourceClear(virStorageSourcePtr def) virBitmapFree(def->features); VIR_FREE(def->compat); virStorageEncryptionFree(def->encryption); + virStoragePRDefFree(def->pr); virStorageSourceSeclabelsClear(def); virStoragePermsFree(def->perms); VIR_FREE(def->timestamps); diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index ecd806c93..f82c20cf4 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -216,6 +216,14 @@ struct _virStorageAuthDef { virSecretLookupTypeDef seclookupdef; }; +typedef struct _virStoragePRDef virStoragePRDef; +typedef virStoragePRDef *virStoragePRDefPtr; +struct _virStoragePRDef { + int enabled; /* enum virTristateBool */ + int managed; /* enum virTristateBool */ + char *path; +}; + typedef struct _virStorageDriverData virStorageDriverData; typedef virStorageDriverData *virStorageDriverDataPtr; @@ -243,6 +251,7 @@ struct _virStorageSource { bool authInherited; virStorageEncryptionPtr encryption; bool encryptionInherited; + virStoragePRDefPtr pr; virObjectPtr privateData; @@ -369,6 +378,12 @@ virStorageAuthDefPtr virStorageAuthDefCopy(const virStorageAuthDef *src); virStorageAuthDefPtr virStorageAuthDefParse(xmlDocPtr xml, xmlNodePtr root); int virStorageAuthDefFormat(virBufferPtr buf, virStorageAuthDefPtr authdef); +void virStoragePRDefFree(virStoragePRDefPtr prd); +virStoragePRDefPtr virStoragePRDefParseNode(xmlDocPtr xml, + xmlNodePtr root); +void virStoragePRDefFormat(virBufferPtr buf, + virStoragePRDefPtr prd); + virSecurityDeviceLabelDefPtr virStorageSourceGetSecurityLabelDef(virStorageSourcePtr src, const char *model); diff --git a/tests/qemuxml2argvdata/disk-virtio-scsi-reservations-not-managed.xml b/tests/qemuxml2argvdata/disk-virtio-scsi-reservations-not-managed.xml new file mode 100644 index 000000000..8ee623a70 --- /dev/null +++ b/tests/qemuxml2argvdata/disk-virtio-scsi-reservations-not-managed.xml @@ -0,0 +1,40 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>8</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-i686</emulator> + <disk type='block' device='lun'> + <source dev='/dev/HostVG/QEMUGuest1'> + <reservations enabled='yes' managed='no'> + <source type='unix' path='/path/to/qemu-pr-helper.sock' mode='client'/> + </reservations> + </source> + <target dev='sdb' bus='scsi'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='usb' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> + </controller> + <controller type='scsi' index='0' model='virtio-scsi'> + <driver queues='8'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> + </controller> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='virtio'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> + </memballoon> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/disk-virtio-scsi-reservations.xml b/tests/qemuxml2argvdata/disk-virtio-scsi-reservations.xml new file mode 100644 index 000000000..874446f7b --- /dev/null +++ b/tests/qemuxml2argvdata/disk-virtio-scsi-reservations.xml @@ -0,0 +1,38 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>8</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-i686</emulator> + <disk type='block' device='lun'> + <source dev='/dev/HostVG/QEMUGuest1'> + <reservations enabled='yes' managed='yes'/> + </source> + <target dev='sdb' bus='scsi'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='usb' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> + </controller> + <controller type='scsi' index='0' model='virtio-scsi'> + <driver queues='8'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> + </controller> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='virtio'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> + </memballoon> + </devices> +</domain> diff --git a/tests/qemuxml2xmloutdata/disk-virtio-scsi-reservations-not-managed.xml b/tests/qemuxml2xmloutdata/disk-virtio-scsi-reservations-not-managed.xml new file mode 120000 index 000000000..f96d62cb6 --- /dev/null +++ b/tests/qemuxml2xmloutdata/disk-virtio-scsi-reservations-not-managed.xml @@ -0,0 +1 @@ +../qemuxml2argvdata/disk-virtio-scsi-reservations-not-managed.xml \ No newline at end of file diff --git a/tests/qemuxml2xmloutdata/disk-virtio-scsi-reservations.xml b/tests/qemuxml2xmloutdata/disk-virtio-scsi-reservations.xml new file mode 120000 index 000000000..dde52fd1d --- /dev/null +++ b/tests/qemuxml2xmloutdata/disk-virtio-scsi-reservations.xml @@ -0,0 +1 @@ +../qemuxml2argvdata/disk-virtio-scsi-reservations.xml \ No newline at end of file diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 2be8eb2c1..3d70f13c8 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -531,6 +531,10 @@ mymain(void) QEMU_CAPS_NODEFCONFIG, QEMU_CAPS_VIRTIO_SCSI); DO_TEST("disk-virtio-scsi-num_queues", QEMU_CAPS_NODEFCONFIG, QEMU_CAPS_VIRTIO_SCSI); + DO_TEST("disk-virtio-scsi-reservations", + QEMU_CAPS_NODEFCONFIG, QEMU_CAPS_VIRTIO_SCSI); + DO_TEST("disk-virtio-scsi-reservations-not-managed", + QEMU_CAPS_NODEFCONFIG, QEMU_CAPS_VIRTIO_SCSI); DO_TEST("disk-virtio-scsi-cmd_per_lun", QEMU_CAPS_NODEFCONFIG, QEMU_CAPS_VIRTIO_SCSI); DO_TEST("disk-virtio-scsi-max_sectors", -- 2.13.6

On Thu, Jan 18, 2018 at 17:04:34 +0100, Michal Privoznik wrote: Is there a particular reason to format 'reservations' separately? Translations should not translate it, but doing this is generally deemed translation unfriendly. Rest looks good to me.

On Mon, Feb 12, 2018 at 17:19:24 +0100, Andrea Bolognani wrote: Um yes, in this case it will actually help due to the surrounding code using that pattern a lot and also tha the field should not be translated. I retract my comment. ACK

On Thu, Jan 18, 2018 at 17:04:36 +0100, Michal Privoznik wrote: Is this really a good idea? If you restart libvirtd gracefully, this will kill all the PR daemons. Also I'd prefer if all the process management code would be in qemu_process.c.

On Thu, Jan 18, 2018 at 17:04:37 +0100, Michal Privoznik wrote: I though that unmanaged PRs would be shared which would make sense given the data structure you've introduced in patch 4. Since this code would in that case make problems with hotplug I looked back at that code. So, this means that there's exactly one managed PR daemon and every unmanaged PR daemon has possibly multiple instances/connections from qemu. So, that brings me to the question: Do you really need the prHelpers hash table? If the instances are duplicated, you can store the data in virStorageSource (in the private data) and don't really need the table. If there's intent to add sharing of the pr objects in qemu, you'll need to rething this code, since generating the alias will create problems when you hotunplug a shared PR instance and try to plug back a disk with a different one.

On Thu, Jan 18, 2018 at 17:04:38 +0100, Michal Privoznik wrote: This logic should not be here. If there is the field parse it. If it's not don't parse it. Hmmm, we probably should make sure that this is always allocated in the qemu driver as a separate patch preceeding this series. Same here, this logic should not be here. Presence of the alias should be the key.

On Thu, Jan 18, 2018 at 17:04:39 +0100, Michal Privoznik wrote: This code should really just look whether the alias is allocated rather than duplicating the logic of when it's used. According qemu.git/qapi/block-core.json, the pr-manager field is supported only for '@BlockdevOptionsFile'. This means that you need to check that it's used only for local paths which eventually translate to BlockdevOptionsFile. Otherwise qemu will refuse this with any network based disk. virStorageSourceIsLocalStorage is your friend. Do you really need a single member struct for the void pointer?

Before we exec() qemu we have to spawn pr-helper processes for all managed reservations (well, technically there can only one). The only caveat there is that we should place the process into the same namespace and cgroup as qemu (so that it shares the same view of the system). But we can do that only after we've forked. That means calling the setup function between fork() and exec(). Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/qemu/qemu_process.c | 151 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 151 insertions(+) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 25ec464d3..02608c1f3 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2507,6 +2507,151 @@ qemuProcessSetupEmulator(virDomainObjPtr vm) } +static int +qemuProcessSetupOnePRDaemonHook(void *opaque) +{ + virDomainObjPtr vm = opaque; + size_t i, nfds = 0; + int *fds = NULL; + int ret = -1; + + if (virProcessGetNamespaces(vm->pid, &nfds, &fds) < 0) + return ret; + + if (virProcessSetNamespaces(nfds, fds) < 0) + goto cleanup; + + ret = 0; + cleanup: + for (i = 0; i < nfds; i++) + VIR_FORCE_CLOSE(fds[i]); + VIR_FREE(fds); + return ret; +} + + +static int +qemuProcessSetupOnePRDaemon(void *payload, + const void *name, + void *opaque) +{ + qemuDomainDiskPRObjectPtr prObj = payload; + virDomainObjPtr vm = opaque; + qemuDomainObjPrivatePtr priv = vm->privateData; + virQEMUDriverPtr driver = priv->driver; + virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + char *pidfile = NULL; + pid_t cpid = -1; + virCommandPtr cmd = NULL; + virTimeBackOffVar timebackoff; + const unsigned long long timeout = 500000; /* ms */ + int ret = -1; + + if (!prObj->managed) + return 0; + + if (!virFileIsExecutable(cfg->prHelperName)) { + virReportSystemError(errno, _("'%s' is not a suitable pr helper"), + cfg->prHelperName); + goto cleanup; + } + + if (!(pidfile = virPidFileBuildPath(cfg->stateDir, name))) + goto cleanup; + + if (unlink(pidfile) < 0 && + errno != ENOENT) { + virReportSystemError(errno, + _("Cannot remove stale PID file %s"), + pidfile); + goto cleanup; + } + + if (!(cmd = virCommandNewArgList(cfg->prHelperName, + "-k", prObj->path, + NULL))) + goto cleanup; + + virCommandDaemonize(cmd); + virCommandSetPidFile(cmd, pidfile); + virCommandSetPreExecHook(cmd, qemuProcessSetupOnePRDaemonHook, vm); + +#ifdef CAP_SYS_RAWIO + virCommandAllowCap(cmd, CAP_SYS_RAWIO); +#else + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Raw I/O is not supported on this platform")); + goto cleanup; +#endif + + if (virCommandRun(cmd, NULL) < 0) + goto cleanup; + + if (virPidFileReadPath(pidfile, &cpid) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("pr helper %s didn't show up"), (const char *) name); + goto cleanup; + } + + if (virTimeBackOffStart(&timebackoff, 1, timeout) < 0) + goto cleanup; + while (virTimeBackOffWait(&timebackoff)) { + if (virFileExists(prObj->path)) + break; + + if (virProcessKill(cpid, 0) == 0) + continue; + + virReportSystemError(errno, "%s", + _("pr helper died unexpectedly")); + goto cleanup; + } + + if (priv->cgroup && + virCgroupAddMachineTask(priv->cgroup, cpid) < 0) + goto cleanup; + + if (qemuSecurityDomainSetPathLabel(driver->securityManager, + vm->def, prObj->path, true) < 0) + goto cleanup; + + prObj->pid = cpid; + ret = 0; + cleanup: + if (ret < 0) { + virCommandAbort(cmd); + virProcessKillPainfully(cpid, true); + } + virCommandFree(cmd); + if (unlink(pidfile) < 0 && + errno != ENOENT && + !virGetLastError()) + virReportSystemError(errno, + _("Cannot remove stale PID file %s"), + pidfile); + VIR_FREE(pidfile); + virObjectUnref(cfg); + return ret; +} + + +static int +qemuProcessSetupPRDaemons(virDomainObjPtr vm) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + int ret = -1; + + if (priv->prHelpers && + virHashForEach(priv->prHelpers, + qemuProcessSetupOnePRDaemon, vm) < 0) + goto cleanup; + + ret = 0; + cleanup: + return ret; +} + + static int qemuProcessInitPasswords(virConnectPtr conn, virQEMUDriverPtr driver, @@ -5896,6 +6041,10 @@ qemuProcessLaunch(virConnectPtr conn, if (qemuProcessSetupEmulator(vm) < 0) goto cleanup; + VIR_DEBUG("Setting up PR daemons"); + if (qemuProcessSetupPRDaemons(vm) < 0) + goto cleanup; + VIR_DEBUG("Setting domain security labels"); if (qemuSecuritySetAllLabel(driver, vm, @@ -6475,6 +6624,8 @@ void qemuProcessStop(virQEMUDriverPtr driver, VIR_FREE(vm->def->seclabels[i]->imagelabel); } + qemuDomainDiskPRObjectKillAll(priv); + qemuHostdevReAttachDomainDevices(driver, vm->def); def = vm->def; -- 2.13.6

On Mon, Feb 12, 2018 at 05:54:19PM +0100, Peter Krempa wrote: Namespaces have been around since at least RHEL-5 vintage so I reckon we can assume non-zero number of namespaces. In fact perhaps we should just make virProcessGetNamespaces() return an explicit error in the unlikely case it finds zero namespaces. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

We need to keep track of spawned processes so that we can kill them when qemu process dies. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/qemu/qemu_domain.c | 130 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 130 insertions(+) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 97996b053..4079165f9 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -131,6 +131,10 @@ static virClassPtr qemuDomainSaveCookieClass; static void qemuDomainLogContextDispose(void *obj); static void qemuDomainSaveCookieDispose(void *obj); +static void +qemuDomainDiskPRObjectHashFree(void *payload, + const void *name); + static int qemuDomainOnceInit(void) { @@ -1954,6 +1958,46 @@ qemuDomainObjPrivateXMLFormatAllowReboot(virBufferPtr buf, } +static int +qemuDomainObjPrivateXMLFormatOnePR(void *payload, + const void *name, + void *data) +{ + qemuDomainDiskPRObjectPtr prObj = payload; + virBufferPtr buf = data; + + virBufferAddLit(buf, "<manager>\n"); + virBufferAdjustIndent(buf, 2); + virBufferAsprintf(buf, "<alias>%s</alias>\n", (const char *) name); + virBufferAsprintf(buf, "<managed>%s</managed>\n", + virTristateBoolTypeToString(virTristateBoolFromBool(prObj->managed))); + virBufferEscapeString(buf, "<path>%s</path>\n", prObj->path); + virBufferAsprintf(buf, "<pid>%lld</pid>\n", (long long) prObj->pid); + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</manager>\n"); + + return 0; +} + + +static void +qemuDomainObjPrivateXMLFormatPRs(virBufferPtr buf, + virDomainObjPtr vm) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + + if (!priv->prHelpers) + return; + + virBufferAddLit(buf, "<prManagers>\n"); + virBufferAdjustIndent(buf, 2); + virHashForEach(priv->prHelpers, + qemuDomainObjPrivateXMLFormatOnePR, buf); + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</prManagers>\n"); +} + + static int qemuDomainObjPrivateXMLFormat(virBufferPtr buf, virDomainObjPtr vm) @@ -2081,6 +2125,8 @@ qemuDomainObjPrivateXMLFormat(virBufferPtr buf, if (qemuDomainObjPrivateXMLFormatBlockjobs(buf, vm) < 0) return -1; + qemuDomainObjPrivateXMLFormatPRs(buf, vm); + return 0; } @@ -2213,6 +2259,87 @@ qemuDomainObjPrivateXMLParseAllowReboot(xmlXPathContextPtr ctxt, } +static qemuDomainDiskPRObjectPtr +qemuDomainObjPrivateXMLParseOnePR(xmlXPathContextPtr ctxt, + char **alias) +{ + qemuDomainDiskPRObjectPtr ret; + char *managedStr = NULL; + int managed; + char *path = NULL; + long long cpid; + + if (VIR_ALLOC(ret) < 0) + return NULL; + + if (!(managedStr = virXPathString("string(./managed)", ctxt)) || + !(*alias = virXPathString("string(./alias)", ctxt)) || + !(path = virXPathString("string(./path)", ctxt)) || + virXPathLongLong("string(./pid)", ctxt, &cpid) < 0 || + (managed = virTristateBoolTypeFromString(managedStr)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Malformed status XML in <prManagers/>")); + goto error; + } + + VIR_FREE(managedStr); + ret->managed = managed == VIR_TRISTATE_BOOL_YES; + ret->path = path; + ret->pid = cpid; + path = NULL; + + return ret; + + error: + VIR_FREE(managedStr); + VIR_FREE(*alias); + VIR_FREE(path); + VIR_FREE(ret); + return NULL; +} + + +static int +qemuDomainObjPrivateXMLParsePRs(qemuDomainObjPrivatePtr priv, + xmlXPathContextPtr ctxt) +{ + xmlNodePtr saveNode = ctxt->node; + xmlNodePtr *nodeset = NULL; + virHashTablePtr prHelpers = NULL; + size_t i, n; + int ret = -1; + + if ((n = virXPathNodeSet("./prManagers/manager", ctxt, &nodeset)) > 0) { + if (!(prHelpers = virHashCreate(10, qemuDomainDiskPRObjectHashFree))) + goto cleanup; + + for (i = 0; i < n; i++) { + qemuDomainDiskPRObjectPtr tmp; + char *alias = NULL; + + ctxt->node = nodeset[i]; + if (!(tmp = qemuDomainObjPrivateXMLParseOnePR(ctxt, &alias))) + goto cleanup; + + if (virHashAddEntry(prHelpers, alias, tmp) < 0) { + qemuDomainDiskPRObjectHashFree(tmp, alias); + VIR_FREE(alias); + goto cleanup; + } + } + } + + priv->prHelpers = prHelpers; + prHelpers = NULL; + ret = 0; + cleanup: + virHashFree(prHelpers); + VIR_FREE(nodeset); + ctxt->node = saveNode; + return ret; +} + + static int qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt, virDomainObjPtr vm, @@ -2433,6 +2560,9 @@ qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt, if (qemuDomainObjPrivateXMLParseBlockjobs(priv, ctxt) < 0) goto error; + if (qemuDomainObjPrivateXMLParsePRs(priv, ctxt) < 0) + goto error; + return 0; error: -- 2.13.6

For hotplug, we will need to know how many disks use given pr-manager so that we know if to plug it too (for the first usage) or unlpug it (for the last one). Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/qemu/qemu_domain.c | 26 ++++++++++++++++++++++++++ src/qemu/qemu_domain.h | 4 ++++ 2 files changed, 30 insertions(+) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 4079165f9..c327dfc86 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -11236,6 +11236,32 @@ qemuDomainDiskPRObjectKillAll(qemuDomainObjPrivatePtr priv) priv->prHelpers = NULL; } +size_t +qemuDomainGetPRUsageCount(const virDomainDef *def, + const char *prAlias) +{ + size_t used = 0; + size_t i; + + for (i = 0; i < def->ndisks; i++) { + const virDomainDiskDef *disk = def->disks[i]; + qemuDomainStorageSourcePrivatePtr srcPriv; + + if (!disk->src || + !disk->src->pr) + continue; + + srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(disk->src); + + if (STRNEQ(srcPriv->prAlias, prAlias)) + continue; + + used++; + } + + return used; +} + static int qemuDomainPrepareDiskPR(qemuDomainObjPrivatePtr priv, diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 5fdb5b931..231583256 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -1011,4 +1011,8 @@ qemuDomainDiskPRObjectRegister(qemuDomainObjPrivatePtr priv, void qemuDomainDiskPRObjectKillAll(qemuDomainObjPrivatePtr priv); +size_t +qemuDomainGetPRUsageCount(const virDomainDef *def, + const char *prAlias); + #endif /* __QEMU_DOMAIN_H__ */ -- 2.13.6

On Thu, Jan 18, 2018 at 17:04:44 +0100, Michal Privoznik wrote: Do we really need a wrapper that only modifies argument order?

On Thu, Jan 18, 2018 at 17:04:45 +0100, Michal Privoznik wrote: This function used in conjucntion with the JSON->commandline formatter should be used instead of qemuBuildMasterPRCommandLineHelper so that we don't have multiple implementations which need to be kept in sync ... Obviously you'll still need a wrapper for this logic ... ... but we should not have two different implementations of this. Shouldn't we use a different approach rather than comments like this?