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; Reviewed-by: John Ferlan <jferlan(a)redhat.com&gt; --- docs/formatdomain.html.in | 23 +++- docs/schemas/domaincommon.rng | 34 +----- docs/schemas/storagecommon.rng | 50 ++++++++ src/conf/domain_conf.c | 38 ++++++ src/libvirt_private.syms | 3 + src/util/virstoragefile.c | 131 +++++++++++++++++++++ src/util/virstoragefile.h | 14 +++ .../disk-virtio-scsi-reservations.xml | 49 ++++++++ .../disk-virtio-scsi-reservations.xml | 1 + tests/qemuxml2xmltest.c | 2 + 10 files changed, 314 insertions(+), 31 deletions(-) create mode 100644 tests/qemuxml2argvdata/disk-virtio-scsi-reservations.xml create mode 120000 tests/qemuxml2xmloutdata/disk-virtio-scsi-reservations.xml diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index ada0df227f..6ed2fd349d 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -2565,7 +2565,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; @@ -2928,6 +2931,24 @@ <a href="formatstorageencryption.html">Storage Encryption</a> page for more information. </dd> + <dt><code>reservations</code></dt> + <dd><span class="since">Since libvirt 4.3.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 enables 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 a helper process. When the PR is unmanaged, then hypervisor + acts as a client and path to server 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 4cab55f05d..93084887fb 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1530,6 +1530,9 @@ <optional> <ref name="encryption"/> </optional> + <optional> + <ref name="reservations"/> + </optional> <zeroOrMore> <ref name='devSeclabel'/> </zeroOrMore> @@ -2434,18 +2437,6 @@ </attribute> </optional> </define> - <define name="reconnect"> - <element name="reconnect"> - <attribute name="enabled"> - <ref name="virYesNo"/> - </attribute> - <optional> - <attribute name="timeout"> - <ref name="unsignedInt"/> - </attribute> - </optional> - </element> - </define> <!-- An interface description can either be of type bridge in which case @@ -2494,24 +2485,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> - <optional> - <ref name="reconnect"/> - </optional> - <empty/> - </element> + <ref name="unixSocketSource"/> <ref name="interface-options"/> </interleave> </group> diff --git a/docs/schemas/storagecommon.rng b/docs/schemas/storagecommon.rng index edee1b0845..eed0b33347 100644 --- a/docs/schemas/storagecommon.rng +++ b/docs/schemas/storagecommon.rng @@ -39,6 +39,56 @@ </element> </define> + <define name="reconnect"> + <element name="reconnect"> + <attribute name="enabled"> + <ref name="virYesNo"/> + </attribute> + <optional> + <attribute name="timeout"> + <ref name="unsignedInt"/> + </attribute> + </optional> + </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> + <optional> + <ref name="reconnect"/> + </optional> + <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 65b429460a..c454fb1b9b 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5205,6 +5205,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 devices")); + 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 @@ -8608,6 +8615,31 @@ virDomainDiskSourcePrivateDataParse(xmlNodePtr node, } +static int +virDomainDiskSourcePRParse(xmlNodePtr node, + xmlXPathContextPtr ctxt, + virStoragePRDefPtr *pr) +{ + xmlNodePtr saveNode = ctxt->node; + int ret = -1; + + ctxt->node = node; + + if (!(ctxt->node = virXPathNode("./reservations", ctxt))) { + ret = 0; + goto cleanup; + } + + if (!(*pr = virStoragePRDefParseXML(ctxt))) + goto cleanup; + + ret = 0; + cleanup: + ctxt->node = saveNode; + return ret; +} + + static int virDomainStorageSourceParse(xmlNodePtr node, xmlXPathContextPtr ctxt, @@ -8654,6 +8686,9 @@ virDomainStorageSourceParse(xmlNodePtr node, !(src->encryption = virStorageEncryptionParseNode(tmp, ctxt))) goto cleanup; + if (virDomainDiskSourcePRParse(node, ctxt, &src->pr) < 0) + goto cleanup; + if (virSecurityDeviceLabelDefParseXML(&src->seclabels, &src->nseclabels, ctxt, flags) < 0) goto cleanup; @@ -22928,6 +22963,9 @@ virDomainStorageSourceFormat(virBufferPtr attrBuf, virStorageEncryptionFormat(childBuf, src->encryption) < 0) return -1; + if (src->pr) + virStoragePRDefFormat(childBuf, src->pr); + return 0; } diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index d2728749fb..f81cb5df93 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2797,6 +2797,9 @@ virStorageNetHostDefFree; virStorageNetHostTransportTypeFromString; virStorageNetHostTransportTypeToString; virStorageNetProtocolTypeToString; +virStoragePRDefFormat; +virStoragePRDefFree; +virStoragePRDefParseXML; virStorageSourceBackingStoreClear; virStorageSourceClear; virStorageSourceCopy; diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 531540ac91..42ef24e9d5 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1892,6 +1892,136 @@ virStorageAuthDefFormat(virBufferPtr buf, } +void +virStoragePRDefFree(virStoragePRDefPtr prd) +{ + if (!prd) + return; + + VIR_FREE(prd->path); + VIR_FREE(prd); +} + + +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"), enabled); + goto cleanup; + } + + if (prd->enabled == VIR_TRISTATE_BOOL_YES) { + if (!(managed = virXPathString("string(./@managed)", ctxt))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing @managed attribute for <reservations/>")); + goto cleanup; + } + + if ((prd->managed = virTristateBoolTypeFromString(managed)) <= 0) { + virReportError(VIR_ERR_XML_ERROR, + _("invalid value for 'managed': %s"), 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 for <reservations/>")); + goto cleanup; + } + + if (!path) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing path for <reservations/>")); + goto cleanup; + } + + if (!mode) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing connection mode for <reservations/>")); + goto cleanup; + } + + if (STRNEQ(type, "unix")) { + virReportError(VIR_ERR_XML_ERROR, + _("unsupported connection type for <reservations/>: %s"), + type); + goto cleanup; + } + + if (STRNEQ(mode, "client")) { + virReportError(VIR_ERR_XML_ERROR, + _("unsupported connection mode for <reservations/>: %s"), + mode); + goto cleanup; + } + + VIR_STEAL_PTR(prd->path, path); + } + } + + ret = prd; + prd = NULL; + + cleanup: + VIR_FREE(mode); + VIR_FREE(path); + VIR_FREE(type); + VIR_FREE(managed); + VIR_FREE(enabled); + virStoragePRDefFree(prd); + return ret; +} + + +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) @@ -2264,6 +2394,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 d129e81978..69e7e20e17 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,11 @@ virStorageAuthDefPtr virStorageAuthDefParse(xmlNodePtr node, xmlXPathContextPtr ctxt); void virStorageAuthDefFormat(virBufferPtr buf, virStorageAuthDefPtr authdef); +void virStoragePRDefFree(virStoragePRDefPtr prd); +virStoragePRDefPtr virStoragePRDefParseXML(xmlXPathContextPtr ctxt); +void virStoragePRDefFormat(virBufferPtr buf, + virStoragePRDefPtr prd); + virSecurityDeviceLabelDefPtr virStorageSourceGetSecurityLabelDef(virStorageSourcePtr src, const char *model); diff --git a/tests/qemuxml2argvdata/disk-virtio-scsi-reservations.xml b/tests/qemuxml2argvdata/disk-virtio-scsi-reservations.xml new file mode 100644 index 0000000000..036c6e3c25 --- /dev/null +++ b/tests/qemuxml2argvdata/disk-virtio-scsi-reservations.xml @@ -0,0 +1,49 @@ +<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'> + <driver name='qemu' type='raw'/> + <source dev='/dev/HostVG/QEMUGuest1'> + <reservations enabled='yes' managed='yes'/> + </source> + <target dev='sda' bus='scsi'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <disk type='block' device='lun'> + <driver name='qemu' type='raw'/> + <source dev='/dev/HostVG/QEMUGuest2'> + <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='1'/> + </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.xml b/tests/qemuxml2xmloutdata/disk-virtio-scsi-reservations.xml new file mode 120000 index 0000000000..dde52fd1db --- /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 9e77b9fb13..871f01edd7 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -387,6 +387,8 @@ mymain(void) QEMU_CAPS_VIRTIO_SCSI); DO_TEST("disk-virtio-scsi-num_queues", QEMU_CAPS_VIRTIO_SCSI); + DO_TEST("disk-virtio-scsi-reservations", + QEMU_CAPS_VIRTIO_SCSI); DO_TEST("disk-virtio-scsi-cmd_per_lun", QEMU_CAPS_VIRTIO_SCSI); DO_TEST("disk-virtio-scsi-max_sectors", -- 2.16.1

Couple of reasons for that: a) there's no monitor command to change path where the pr-helper connects to, or b) there's no monitor command to introduce a new pr-helper for a disk that already exists. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; Reviewed-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/libvirt_private.syms | 1 + src/qemu/qemu_domain.c | 8 ++++++++ src/util/virstoragefile.c | 19 +++++++++++++++++++ src/util/virstoragefile.h | 2 ++ 4 files changed, 30 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index f81cb5df93..27c5bb5bce 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2799,6 +2799,7 @@ virStorageNetHostTransportTypeToString; virStorageNetProtocolTypeToString; virStoragePRDefFormat; virStoragePRDefFree; +virStoragePRDefIsEqual; virStoragePRDefParseXML; virStorageSourceBackingStoreClear; virStorageSourceClear; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 326c939c85..0dc5a88e79 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -7836,6 +7836,14 @@ qemuDomainDiskChangeSupported(virDomainDiskDefPtr disk, CHECK_EQ(src->readonly, "readonly", true); CHECK_EQ(src->shared, "shared", true); + if (!virStoragePRDefIsEqual(disk->src->pr, + orig_disk->src->pr)) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("cannot modify field '%s' of the disk"), + "reservations"); + return false; + } + #undef CHECK_EQ return true; diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 42ef24e9d5..d0050eca1f 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -2022,6 +2022,25 @@ virStoragePRDefFormat(virBufferPtr buf, } +bool +virStoragePRDefIsEqual(virStoragePRDefPtr a, + virStoragePRDefPtr b) +{ + if (!a && !b) + return true; + + if (!a || !b) + return false; + + if (a->enabled != b->enabled || + a->managed != b->managed || + STRNEQ_NULLABLE(a->path, b->path)) + return false; + + return true; +} + + virSecurityDeviceLabelDefPtr virStorageSourceGetSecurityLabelDef(virStorageSourcePtr src, const char *model) diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 69e7e20e17..09e9ae73f1 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -382,6 +382,8 @@ void virStoragePRDefFree(virStoragePRDefPtr prd); virStoragePRDefPtr virStoragePRDefParseXML(xmlXPathContextPtr ctxt); void virStoragePRDefFormat(virBufferPtr buf, virStoragePRDefPtr prd); +bool virStoragePRDefIsEqual(virStoragePRDefPtr a, + virStoragePRDefPtr b); virSecurityDeviceLabelDefPtr virStorageSourceGetSecurityLabelDef(virStorageSourcePtr src, -- 2.16.1

For command line we need two things: 1) -object pr-manager-helper,id=$alias,path=$socketPath 2) -drive file.pr-manager=$alias In -object pr-manager-helper we tell qemu which socket to connect to, then in -drive file-pr-manager we just reference the object the drive in question should use. For managed PR helper the alias is always "pr-helper0" and socket path "${vm->priv->libDir}/pr-helper0.sock". It was decided in reviews to previous versions of this patch that it should not allocate memory in order to simplify code. This promise is not fulfilled though. For instance, qemuBuildPRManagerInfoProps() is an offender. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/libvirt_private.syms | 2 + src/qemu/qemu_alias.c | 18 +++ src/qemu/qemu_alias.h | 4 + src/qemu/qemu_command.c | 134 +++++++++++++++++++++ src/qemu/qemu_command.h | 5 + src/qemu/qemu_domain.c | 22 ++++ src/qemu/qemu_domain.h | 3 + src/qemu/qemu_process.h | 1 + src/util/virstoragefile.c | 14 +++ src/util/virstoragefile.h | 2 + .../disk-virtio-scsi-reservations.args | 38 ++++++ tests/qemuxml2argvtest.c | 4 + 12 files changed, 247 insertions(+) create mode 100644 tests/qemuxml2argvdata/disk-virtio-scsi-reservations.args diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 27c5bb5bce..108b44f6f4 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2799,7 +2799,9 @@ virStorageNetHostTransportTypeToString; virStorageNetProtocolTypeToString; virStoragePRDefFormat; virStoragePRDefFree; +virStoragePRDefIsEnabled; virStoragePRDefIsEqual; +virStoragePRDefIsManaged; virStoragePRDefParseXML; virStorageSourceBackingStoreClear; virStorageSourceClear; diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c index 95d1e0370a..9a3a92c82d 100644 --- a/src/qemu/qemu_alias.c +++ b/src/qemu/qemu_alias.c @@ -773,3 +773,21 @@ qemuAliasChardevFromDevAlias(const char *devAlias) return ret; } + + +const char * +qemuDomainGetManagedPRAlias(void) +{ + return "pr-helper0"; +} + + +char * +qemuDomainGetUnmanagedPRAlias(const virDomainDiskDef *disk) +{ + char *ret; + + ignore_value(virAsprintf(&ret, "pr-helper-%s", disk->info.alias)); + + return ret; +} diff --git a/src/qemu/qemu_alias.h b/src/qemu/qemu_alias.h index 8c744138ce..8e391c3f0c 100644 --- a/src/qemu/qemu_alias.h +++ b/src/qemu/qemu_alias.h @@ -92,4 +92,8 @@ char *qemuAliasTLSObjFromSrcAlias(const char *srcAlias) char *qemuAliasChardevFromDevAlias(const char *devAlias) ATTRIBUTE_NONNULL(1); +const char * qemuDomainGetManagedPRAlias(void); + +char *qemuDomainGetUnmanagedPRAlias(const virDomainDiskDef *disk); + #endif /* __QEMU_ALIAS_H__*/ diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index b666f3715f..8b1b5934b1 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1462,6 +1462,28 @@ qemuDiskSourceGetProps(virStorageSourcePtr src) } +static int +qemuBuildDriveSourcePR(virBufferPtr buf, + virDomainDiskDefPtr disk) +{ + char *alias = NULL; + const char *defaultAlias = NULL; + + if (!virStoragePRDefIsEnabled(disk->src->pr)) + return 0; + + if (virStoragePRDefIsManaged(disk->src->pr)) + defaultAlias = qemuDomainGetManagedPRAlias(); + else if (!(alias = qemuDomainGetUnmanagedPRAlias(disk))) + return -1; + + + virBufferAsprintf(buf, ",file.pr-manager=%s", alias ? alias : defaultAlias); + VIR_FREE(alias); + return 0; +} + + static int qemuBuildDriveSourceStr(virDomainDiskDefPtr disk, virQEMUCapsPtr qemuCaps, @@ -1539,6 +1561,9 @@ qemuBuildDriveSourceStr(virDomainDiskDefPtr disk, if (disk->src->debug) virBufferAsprintf(buf, ",file.debug=%d", disk->src->debugLevel); + + if (qemuBuildDriveSourcePR(buf, disk) < 0) + goto cleanup; } else { if (!(source = virQEMUBuildDriveCommandlineFromJSON(srcprops))) goto cleanup; @@ -9665,6 +9690,112 @@ qemuBuildPanicCommandLine(virCommandPtr cmd, } +/** + * qemuBuildPRManagerInfoProps: + * @prd: disk PR runtime info + * @propsret: JSON properties to return + * + * Build the JSON properties for the pr-manager object. + * + * Returns: 0 on success (@propsret is NULL if no properties are needed), + * -1 on failure (with error message set). + */ +int +qemuBuildPRManagerInfoProps(virDomainObjPtr vm, + const virDomainDiskDef *disk, + virJSONValuePtr *propsret, + char **aliasret) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + char *socketPath = NULL; + char *alias = NULL; + int ret = -1; + + *propsret = NULL; + *aliasret = NULL; + + if (!virStoragePRDefIsEnabled(disk->src->pr)) + return 0; + + if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_PR_MANAGER_HELPER)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("reservations not supported with this QEMU binary")); + return ret; + } + + if (!(socketPath = qemuDomainGetPRSocketPath(vm, disk->src->pr))) + return ret; + + if (virStoragePRDefIsManaged(disk->src->pr)) { + if (VIR_STRDUP(alias, qemuDomainGetManagedPRAlias()) < 0) + goto cleanup; + } else { + if (!(alias = qemuDomainGetUnmanagedPRAlias(disk))) + goto cleanup; + } + + if (virJSONValueObjectCreate(propsret, + "s:path", socketPath, + NULL) < 0) + goto cleanup; + + VIR_STEAL_PTR(*aliasret, alias); + ret = 0; + cleanup: + VIR_FREE(alias); + VIR_FREE(socketPath); + return ret; +} + + +static int +qemuBuildMasterPRCommandLine(virDomainObjPtr vm, + virCommandPtr cmd, + const virDomainDef *def) +{ + size_t i; + bool managedAdded = false; + virJSONValuePtr props = NULL; + char *alias = NULL; + char *tmp = NULL; + int ret = -1; + + for (i = 0; i < def->ndisks; i++) { + const virDomainDiskDef *disk = def->disks[i]; + + if (virStoragePRDefIsManaged(disk->src->pr)) { + if (managedAdded) + continue; + + managedAdded = true; + } + + if (qemuBuildPRManagerInfoProps(vm, disk, &props, &alias) < 0) + goto cleanup; + + if (!props) + continue; + + if (!(tmp = virQEMUBuildObjectCommandlineFromJSON("pr-manager-helper", + alias, + props))) + goto cleanup; + VIR_FREE(alias); + virJSONValueFree(props); + props = NULL; + + virCommandAddArgList(cmd, "-object", tmp, NULL); + VIR_FREE(tmp); + } + + ret = 0; + cleanup: + VIR_FREE(alias); + virJSONValueFree(props); + return ret; +} + + /** * qemuBuildCommandLineValidate: * @@ -9833,6 +9964,9 @@ qemuBuildCommandLine(virQEMUDriverPtr driver, if (qemuBuildMasterKeyCommandLine(cmd, priv) < 0) goto error; + if (qemuBuildMasterPRCommandLine(vm, cmd, def) < 0) + goto error; + if (enableFips) virCommandAddArg(cmd, "-enable-fips"); diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 31c9da673c..da1fe679fe 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -54,6 +54,11 @@ virCommandPtr qemuBuildCommandLine(virQEMUDriverPtr driver, size_t *nnicindexes, int **nicindexes); +/* Generate the object properties for pr-manager */ +int qemuBuildPRManagerInfoProps(virDomainObjPtr vm, + const virDomainDiskDef *disk, + virJSONValuePtr *propsret, + char **alias); /* Generate the object properties for a secret */ int qemuBuildSecretInfoProps(qemuDomainSecretInfoPtr secinfo, diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 0dc5a88e79..53827d5396 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -11983,3 +11983,25 @@ qemuProcessEventFree(struct qemuProcessEvent *event) } VIR_FREE(event); } + + +char * +qemuDomainGetPRSocketPath(virDomainObjPtr vm, + virStoragePRDefPtr pr) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + const char *defaultAlias = NULL; + char *ret = NULL; + + if (!virStoragePRDefIsEnabled(pr)) + return NULL; + + if (virStoragePRDefIsManaged(pr)) { + defaultAlias = qemuDomainGetManagedPRAlias(); + ignore_value(virAsprintf(&ret, "%s/%s.sock", priv->libDir, defaultAlias)); + } else { + ignore_value(VIR_STRDUP(ret, pr->path)); + } + + return ret; +} diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 2c27dfb9f3..612d234113 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -1000,4 +1000,7 @@ qemuDomainDiskCachemodeFlags(int cachemode, bool *direct, bool *noflush); +char * qemuDomainGetPRSocketPath(virDomainObjPtr vm, + virStoragePRDefPtr pr); + #endif /* __QEMU_DOMAIN_H__ */ diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h index 9dd5c97642..3bf66f71b7 100644 --- a/src/qemu/qemu_process.h +++ b/src/qemu/qemu_process.h @@ -24,6 +24,7 @@ # include "qemu_conf.h" # include "qemu_domain.h" +# include "virstoragefile.h" int qemuProcessPrepareMonitorChr(virDomainChrSourceDefPtr monConfig, const char *domainDir); diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index d0050eca1f..e22dd727fa 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -2041,6 +2041,20 @@ virStoragePRDefIsEqual(virStoragePRDefPtr a, } +bool +virStoragePRDefIsEnabled(virStoragePRDefPtr prd) +{ + return prd && prd->enabled == VIR_TRISTATE_BOOL_YES; +} + + +bool +virStoragePRDefIsManaged(virStoragePRDefPtr prd) +{ + return prd && prd->managed == VIR_TRISTATE_BOOL_YES; +} + + virSecurityDeviceLabelDefPtr virStorageSourceGetSecurityLabelDef(virStorageSourcePtr src, const char *model) diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 09e9ae73f1..059214a517 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -384,6 +384,8 @@ void virStoragePRDefFormat(virBufferPtr buf, virStoragePRDefPtr prd); bool virStoragePRDefIsEqual(virStoragePRDefPtr a, virStoragePRDefPtr b); +bool virStoragePRDefIsEnabled(virStoragePRDefPtr prd); +bool virStoragePRDefIsManaged(virStoragePRDefPtr prd); virSecurityDeviceLabelDefPtr virStorageSourceGetSecurityLabelDef(virStorageSourcePtr src, diff --git a/tests/qemuxml2argvdata/disk-virtio-scsi-reservations.args b/tests/qemuxml2argvdata/disk-virtio-scsi-reservations.args new file mode 100644 index 0000000000..dce3fc4105 --- /dev/null +++ b/tests/qemuxml2argvdata/disk-virtio-scsi-reservations.args @@ -0,0 +1,38 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-i686 \ +-name QEMUGuest1 \ +-S \ +-object pr-manager-helper,id=pr-helper0,\ +path=/tmp/lib/domain--1-QEMUGuest1/pr-helper0.sock \ +-object pr-manager-helper,id=pr-helper-scsi0-0-0-1,\ +path=/path/to/qemu-pr-helper.sock \ +-machine pc,accel=tcg,usb=off,dump-guest-core=off \ +-m 214 \ +-smp 8,sockets=8,cores=1,threads=1 \ +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ +-display none \ +-no-user-config \ +-nodefaults \ +-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,\ +server,nowait \ +-mon chardev=charmonitor,id=monitor,mode=control \ +-rtc base=utc \ +-no-shutdown \ +-no-acpi \ +-boot c \ +-device virtio-scsi-pci,id=scsi0,num_queues=8,bus=pci.0,addr=0x3 \ +-usb \ +-drive file=/dev/HostVG/QEMUGuest1,file.pr-manager=pr-helper0,format=raw,\ +if=none,id=drive-scsi0-0-0-0,boot=on \ +-device scsi-block,bus=scsi0.0,channel=0,scsi-id=0,lun=0,\ +drive=drive-scsi0-0-0-0,id=scsi0-0-0-0 \ +-drive file=/dev/HostVG/QEMUGuest2,file.pr-manager=pr-helper-scsi0-0-0-1,\ +format=raw,if=none,id=drive-scsi0-0-0-1 \ +-device scsi-block,bus=scsi0.0,channel=0,scsi-id=0,lun=1,\ +drive=drive-scsi0-0-0-1,id=scsi0-0-0-1 \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x4 diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 74d930ebe2..ad17a5c333 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -2814,6 +2814,10 @@ mymain(void) QEMU_CAPS_PIIX_DISABLE_S3, QEMU_CAPS_PIIX_DISABLE_S4, QEMU_CAPS_ICH9_USB_EHCI1); + DO_TEST("disk-virtio-scsi-reservations", + QEMU_CAPS_DRIVE_BOOT, QEMU_CAPS_VIRTIO_SCSI, + QEMU_CAPS_SCSI_BLOCK, QEMU_CAPS_PR_MANAGER_HELPER); + /* Test disks with format probing enabled for legacy reasons. * New tests should not go in this section. */ driver.config->allowDiskFormatProbing = true; -- 2.16.1

On 04/29/2018 02:16 PM, John Ferlan wrote: I rather not. Everything else uses a function to get/set aliases. In order to stay consistent I'd rather have this as a function. Do we? Not in the code, but in config.h produced by configure. Well, that's standardized way of defining values in configure. I'm not sure that logic applies to the rest of our code. Because QEMU_PR_HELPER is detected at compile time (for the default, can be overridden in qemu.conf later). There is no other way to do it. We can't have configure generating a function like this: qemuGetPrHelperPath() { return "/usr/bin/qemu-pr-helper"; } But we can have our own code do that, esp. if other functions from the family (qemu alias handling functions that is) are functions indeed instead of some #define-s. Oh, right. Michal

On 04/23/2018 09:14 AM, Michal Privoznik wrote: 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. ... 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, Reviewed-by: John Ferlan <jferlan(a)redhat.com&gt; If you need to adjust the logic to handle this specially, then obviously I'd ask to wait for that OK. I wonder if the @prDaemonRunning logic added later on would suffice? John

Just like in previous commit, qemu-pr-helper might want to open /dev/mapper/control under certain circumstances. Therefore we have to allow it in cgroups. The change virdevmapper.c might look spurious but it isn't. After 6dd84f6850ca437 any path that we're allowing in deivces CGroup is subject to virDevMapperGetTargets() inspection. And libdevmapper returns ENXIO for the path from subject. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/qemu/qemu_cgroup.c | 33 ++++++++++++++++++++++++++++++--- src/util/virdevmapper.c | 8 +++++++- 2 files changed, 37 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index d88eb7881f..546a4c8e63 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -114,6 +114,8 @@ qemuSetupImagePathCgroup(virDomainObjPtr vm, } +#define DEVICE_MAPPER_CONTROL_PATH "/dev/mapper/control" + static int qemuSetupImageCgroupInternal(virDomainObjPtr vm, virStorageSourcePtr src, @@ -125,6 +127,10 @@ qemuSetupImageCgroupInternal(virDomainObjPtr vm, return 0; } + if (virStoragePRDefIsManaged(src->pr) && + qemuSetupImagePathCgroup(vm, DEVICE_MAPPER_CONTROL_PATH, false) < 0) + return -1; + return qemuSetupImagePathCgroup(vm, src->path, src->readonly || forceReadonly); } @@ -142,9 +148,8 @@ qemuTeardownImageCgroup(virDomainObjPtr vm, virStorageSourcePtr src) { qemuDomainObjPrivatePtr priv = vm->privateData; - int perms = VIR_CGROUP_DEVICE_READ | - VIR_CGROUP_DEVICE_WRITE | - VIR_CGROUP_DEVICE_MKNOD; + int perms = VIR_CGROUP_DEVICE_RWM; + size_t i; int ret; if (!virCgroupHasController(priv->cgroup, @@ -157,6 +162,28 @@ qemuTeardownImageCgroup(virDomainObjPtr vm, return 0; } + for (i = 0; i < vm->def->ndisks; i++) { + virStorageSourcePtr diskSrc = vm->def->disks[i]->src; + + if (src == diskSrc) + continue; + + if (virStoragePRDefIsManaged(diskSrc->pr)) + break; + } + + if (i == vm->def->ndisks) { + VIR_DEBUG("Disabling device mapper control"); + ret = virCgroupDenyDevicePath(priv->cgroup, + DEVICE_MAPPER_CONTROL_PATH, perms, true); + virDomainAuditCgroupPath(vm, priv->cgroup, "deny", + DEVICE_MAPPER_CONTROL_PATH, + virCgroupGetDevicePermsString(perms), ret); + if (ret < 0) + return ret; + } + + VIR_DEBUG("Deny path %s", src->path); ret = virCgroupDenyDevicePath(priv->cgroup, src->path, perms, true); diff --git a/src/util/virdevmapper.c b/src/util/virdevmapper.c index d2c25af003..ef4b1e480a 100644 --- a/src/util/virdevmapper.c +++ b/src/util/virdevmapper.c @@ -101,8 +101,14 @@ virDevMapperGetTargetsImpl(const char *path, dm_task_no_open_count(dmt); - if (!dm_task_run(dmt)) + if (!dm_task_run(dmt)) { + if (errno == ENXIO) { + /* In some cases devmapper realizes this late device + * is not managed by it. */ + ret = 0; + } goto cleanup; + } if (!dm_task_get_info(dmt, &info)) goto cleanup; -- 2.16.1

On 04/29/2018 02:34 PM, John Ferlan wrote: The check is there ... .. here. If PR is managed, i.e. qemu-pr-helper is started by libvirt, then it will be also placed into the same CGroup as qemu. Therefore, we need to allow access to /dev/mapper/control. But, it PR is unmanaged, then the helper process is started by somebody else (we don't care who), and therefore it's their responsibility to allow that path in CGroups (if they are placing the helper into any). The only caveat here is that qemu-pr-helper might access the path only sometimes, depending whether it was compiled with multipath support or not. But that's something libvirt can't know. Therefore it has to assume yes and allow the path. To be fair I haven't tested any other devices. Okay, I'll fix the comment. Thanks, Michal

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(). Again, despite demand to not store anything in status XML I have to take my chances and store 'priv->prDaemonRunning' (at least) in status XML. Otherwise we might forget whether we started pr-helper after libvirtd restart. What happens if we change default managed alias or default socket is undefined for now, because we don't store them in status XML. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/qemu/qemu_domain.c | 22 +++++ src/qemu/qemu_domain.h | 2 + src/qemu/qemu_process.c | 232 ++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 256 insertions(+) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 5ccdeb8e3a..fbe6a0f332 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2040,6 +2040,15 @@ qemuDomainObjPrivateXMLFormatAllowReboot(virBufferPtr buf, } +static void +qemuDomainObjPrivateXMLFormatPR(virBufferPtr buf, + qemuDomainObjPrivatePtr priv) +{ + if (priv->prDaemonRunning) + virBufferAddLit(buf, "<prDaemon running='1'/>\n"); +} + + static int qemuDomainObjPrivateXMLFormatJob(virBufferPtr buf, virDomainObjPtr vm, @@ -2180,6 +2189,8 @@ qemuDomainObjPrivateXMLFormat(virBufferPtr buf, qemuDomainObjPrivateXMLFormatAllowReboot(buf, priv->allowReboot); + qemuDomainObjPrivateXMLFormatPR(buf, priv); + if (qemuDomainObjPrivateXMLFormatBlockjobs(buf, vm) < 0) return -1; @@ -2323,6 +2334,15 @@ qemuDomainObjPrivateXMLParseAllowReboot(xmlXPathContextPtr ctxt, } +static void +qemuDomainObjPrivateXMLParsePR(xmlXPathContextPtr ctxt, + bool *prDaemonRunning) +{ + *prDaemonRunning = virXPathBoolean("count(./prDaemon[@running = '1']) > 0", + ctxt) > 0; +} + + static int qemuDomainObjPrivateXMLParseJob(virDomainObjPtr vm, qemuDomainObjPrivatePtr priv, @@ -2572,6 +2592,8 @@ qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt, qemuDomainObjPrivateXMLParseAllowReboot(ctxt, &priv->allowReboot); + qemuDomainObjPrivateXMLParsePR(ctxt, &priv->prDaemonRunning); + if (qemuDomainObjPrivateXMLParseBlockjobs(priv, ctxt) < 0) goto error; diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 612d234113..b837902e8d 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -342,6 +342,8 @@ struct _qemuDomainObjPrivate { /* Migration capabilities. Rechecked on reconnect, not to be saved in * private XML. */ virBitmapPtr migrationCaps; + + bool prDaemonRunning; }; # define QEMU_DOMAIN_PRIVATE(vm) \ diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 6a5262ae46..36cfa438ed 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2555,6 +2555,231 @@ qemuProcessResctrlCreate(virQEMUDriverPtr driver, } +static char * +qemuProcessBuildPRHelperPidfilePath(virDomainObjPtr vm, + const char *prdAlias) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + + return virPidFileBuildPath(priv->libDir, prdAlias); +} + + +static void +qemuProcessKillPRDaemon(virDomainObjPtr vm) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + virErrorPtr orig_err; + const char *prdAlias; + char *pidfile; + + if (!priv->prDaemonRunning) + return; + + if (!(prdAlias = qemuDomainGetManagedPRAlias())) { + VIR_WARN("Unable to get default PR manager alias"); + return; + } + + if (!(pidfile = qemuProcessBuildPRHelperPidfilePath(vm, prdAlias))) { + VIR_WARN("Unable to construct pr-helper pidfile path"); + return; + } + + virErrorPreserveLast(&orig_err); + if (virPidFileForceCleanupPath(pidfile) < 0) { + VIR_WARN("Unable to kill pr-helper process"); + } else { + if (unlink(pidfile) < 0 && + errno != ENOENT) { + virReportSystemError(errno, + _("Unable to remove stale pidfile %s"), + pidfile); + } else { + priv->prDaemonRunning = false; + } + } + virErrorRestore(&orig_err); + + VIR_FREE(pidfile); +} + + +static int +qemuProcessStartPRDaemonHook(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 (nfds > 0 && + 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 +qemuProcessStartPRDaemon(virDomainObjPtr vm, + const virDomainDiskDef *disk) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + virQEMUDriverPtr driver = priv->driver; + virQEMUDriverConfigPtr cfg; + const char *prdAlias; + int errfd = -1; + char *pidfile = NULL; + int pidfd = -1; + char *socketPath = NULL; + pid_t cpid = -1; + virCommandPtr cmd = NULL; + virTimeBackOffVar timebackoff; + const unsigned long long timeout = 500000; /* ms */ + int ret = -1; + + if (!virStoragePRDefIsManaged(disk->src->pr) || + priv->prDaemonRunning) + return 0; + + cfg = virQEMUDriverGetConfig(driver); + + if (!virFileIsExecutable(cfg->prHelperName)) { + virReportSystemError(errno, _("'%s' is not a suitable pr helper"), + cfg->prHelperName); + goto cleanup; + } + + prdAlias = qemuDomainGetManagedPRAlias(); + + if (!(pidfile = qemuProcessBuildPRHelperPidfilePath(vm, prdAlias))) + goto cleanup; + + /* Just try to acquire. Dummy pid will be replaced later */ + if ((pidfd = virPidFileAcquirePath(pidfile, false, -1)) < 0) + goto cleanup; + + if (!(socketPath = qemuDomainGetPRSocketPath(vm, disk->src->pr))) + goto cleanup; + + /* Remove stale socket */ + if (unlink(socketPath) < 0 && + errno != ENOENT) { + virReportSystemError(errno, + _("Unable to remove stale socket path: %s"), + socketPath); + goto cleanup; + } + + if (!(cmd = virCommandNewArgList(cfg->prHelperName, + "-k", socketPath, + "-f", pidfile, + NULL))) + goto cleanup; + + virCommandDaemonize(cmd); + /* We want our virCommand to write child PID into the pidfile + * so that we can read it even before exec(). */ + virCommandSetPidFile(cmd, pidfile); + virCommandSetErrorFD(cmd, &errfd); + + /* Place the process into the same namespace and cgroup as + * qemu (so that it shares the same view of the system). */ + virCommandSetPreExecHook(cmd, qemuProcessStartPRDaemonHook, vm); + + if (virCommandRun(cmd, NULL) < 0) + goto cleanup; + + if (virPidFileReadPath(pidfile, &cpid) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("pr helper %s didn't show up"), prdAlias); + goto cleanup; + } + + if (virTimeBackOffStart(&timebackoff, 1, timeout) < 0) + goto cleanup; + while (virTimeBackOffWait(&timebackoff)) { + char errbuf[1024] = { 0 }; + + if (virFileExists(socketPath)) + break; + + if (virProcessKill(cpid, 0) == 0) + continue; + + if (saferead(errfd, errbuf, sizeof(errbuf) - 1) < 0) { + virReportSystemError(errno, + _("pr helper %s died unexpectedly"), prdAlias); + } else { + virReportError(VIR_ERR_OPERATION_FAILED, + _("pr helper died and reported: %s"), errbuf); + } + goto cleanup; + } + + if (!virFileExists(socketPath)) { + virReportError(VIR_ERR_OPERATION_TIMEOUT, "%s", + _("pr helper socked did not show up")); + goto cleanup; + } + + if (priv->cgroup && + virCgroupAddMachineTask(priv->cgroup, cpid) < 0) + goto cleanup; + + if (qemuSecurityDomainSetPathLabel(driver->securityManager, + vm->def, socketPath, true) < 0) + goto cleanup; + + priv->prDaemonRunning = true; + ret = 1; + cleanup: + if (ret < 0) { + virCommandAbort(cmd); + if (cpid >= 0) + virProcessKillPainfully(cpid, true); + if (pidfile) + unlink(pidfile); + } + virCommandFree(cmd); + VIR_FREE(socketPath); + VIR_FORCE_CLOSE(pidfd); + VIR_FREE(pidfile); + VIR_FORCE_CLOSE(errfd); + virObjectUnref(cfg); + return ret; +} + + +static int +qemuProcessMaybeStartPRDaemon(virDomainObjPtr vm) +{ + size_t i; + int rv; + + for (i = 0; i < vm->def->ndisks; i++) { + const virDomainDiskDef *disk = vm->def->disks[i]; + + if ((rv = qemuProcessStartPRDaemon(vm, disk)) < 0) + return -1; + + if (rv > 0) + return 1; + } + + return 0; +} + + static int qemuProcessInitPasswords(virQEMUDriverPtr driver, virDomainObjPtr vm, @@ -6071,6 +6296,10 @@ qemuProcessLaunch(virConnectPtr conn, if (qemuProcessResctrlCreate(driver, vm) < 0) goto cleanup; + VIR_DEBUG("Setting up PR daemon"); + if (qemuProcessMaybeStartPRDaemon(vm) < 0) + goto cleanup; + VIR_DEBUG("Setting domain security labels"); if (qemuSecuritySetAllLabel(driver, vm, @@ -6598,6 +6827,9 @@ void qemuProcessStop(virQEMUDriverPtr driver, /* Remove the master key */ qemuDomainMasterKeyRemove(priv); + /* Do this before we delete the tree and remove pidfile. */ + qemuProcessKillPRDaemon(vm); + virFileDeleteTree(priv->libDir); virFileDeleteTree(priv->channelTargetDir); -- 2.16.1

On 04/29/2018 07:01 PM, John Ferlan wrote: Can we? I'm not sure we can do that, IOW I don't know how to do that. Which is going to be ugly. We would have to try and guess aliases whereas if we save it in the status XML we know exactly the alias we need. Look at it from different angle - step away from pr-manager object and think of generic device. We generate alias for it. And save it in the status XML. Why? We can just generate the alias whenever we need it. Also, we know all the changes made to alias generating code so we can try all older versions until we find the right one. Or we can query for it (assuming answer to my question above is yes). Okay. I've done all the changes. Thanks, Michal

On 04/23/2018 09:14 AM, Michal Privoznik wrote: and again here a simple: if (VIR_STRDUP(*aliasret, QEMU_DOMAIN_MANAGED_PR_ALIAS) < 0) would suffice Reviewed-by: John Ferlan <jferlan(a)redhat.com&gt; John /me waits for the tester that starts/stops 100's of times in a row ;-) [...]