[libvirt] [PATCH v3 07/10] conf: Wire up the vhost-scsi connection from/to XML
Eric Farman
farman at linux.vnet.ibm.com
Wed Nov 16 20:13:15 UTC 2016
On 11/11/2016 04:53 PM, John Ferlan wrote:
> need a commit message here.
>
> On 11/08/2016 01:26 PM, Eric Farman wrote:
>> Signed-off-by: Eric Farman <farman at linux.vnet.ibm.com>
>> ---
>> docs/schemas/domaincommon.rng | 23 ++++++++++++
>> src/conf/domain_audit.c | 7 ++++
>> src/conf/domain_conf.c | 81 +++++++++++++++++++++++++++++++++++++++++--
>> 3 files changed, 109 insertions(+), 2 deletions(-)
>>
> Beyond the "Host" to "SCSIHost" type changes...
>
> Since this is where the device is being added - this is when the address
> adjustment functions should be addressed. I got really hung up to day
> trying to look for examples - hopefully I didn't make too much of a mess...
>
> First off, the virDomainHostdevDefParseXML should be adjusted to ensure
> that if the user does provide an address - that it is valid for the
> device. IOW: Follow the SUBSYS_TYPE_PCI: case more or less to ensure
> the def->info->type if not NONE is either TYPE_PCI or TYPE_CCW.
Okay.
>
> The virDomainDeviceDefPostParseInternal post processing code should
> handle setting an address if a valid one (checked earlier) isn't
> supplied. This would save the address in the domain/config XML. For PCI
> it would be qemuDomainAssignDevicePCISlots... I'm not clear if doing the
> same for CCW is ever done.
Well, that is what the last hunk you commented on in patch 5 was about.
On s390, we don't have a PCI address being assigned, but rather a CCW
address. Previously, we only supported scsi hostdevs, which used a
drive address that was handled in yet another place. So, if no guest ccw
address is specified, we primed it via
qemuDomainPrimeVirtioDeviceAddresses. (If I just comment that hunk out
now, my guest crashes on boot; so it's now tied into something else that
isn't immediately obvious to me.)
As to the PCI addresses...
>
> In any case, qemuDomainAssignDevicePCISlots does go through the hostdev
> list and reserves PCI address for PCI hostdevs, which I believe would
> also work for this would be. Again, I'm not clear if/why CCW would be
> handled.
I think what you're saying is that the hostdevs loop in
qemuDomainAssignDevicePCISlots should be expanded to cover both a PCI
hostdev (which it does today), and a scsi_host hostdev with PCI
address. Correct?
>
> NB: I haven't gone and looked for every new subsys case to ensure that
> things were addressed for the case's type - just ran out of time and
> energy. But that is something that should be done by the end of patch8
> (which I assume now gets merged into here eventually).
Yeah, merging is fine. Was just trying to keep the ancillary stuff
(e.g., tests) separated since some of these patches are already unwieldy.
>
>> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
>> index 19d45fd..bb903ef 100644
>> --- a/docs/schemas/domaincommon.rng
>> +++ b/docs/schemas/domaincommon.rng
>> @@ -3974,6 +3974,7 @@
>> <ref name="hostdevsubsyspci"/>
>> <ref name="hostdevsubsysusb"/>
>> <ref name="hostdevsubsysscsi"/>
>> + <ref name="hostdevsubsyshost"/>
>> </choice>
>> </define>
>>
>> @@ -4102,6 +4103,28 @@
>> </element>
>> </define>
>>
>> + <define name="hostdevsubsyshost">
>> + <attribute name="type">
>> + <value>scsi_host</value>
>> + </attribute>
>> + <element name="source">
>> + <choice>
>> + <group>
>> + <attribute name="protocol">
>> + <choice>
>> + <value>vhost</value> <!-- vhost, required -->
>> + </choice>
>> + </attribute>
>> + <attribute name="wwpn">
>> + <data type="string">
>> + <param name="pattern">(naa\.)[0-9a-fA-F]{16}</param>
>> + </data>
>> + </attribute>
>> + </group>
>> + </choice>
>> + </element>
>> + </define>
>> +
>> <define name="hostdevcapsstorage">
>> <attribute name="type">
>> <value>storage</value>
>> diff --git a/src/conf/domain_audit.c b/src/conf/domain_audit.c
>> index 2decf02..844b3cd 100644
>> --- a/src/conf/domain_audit.c
>> +++ b/src/conf/domain_audit.c
>> @@ -392,6 +392,7 @@ virDomainAuditHostdev(virDomainObjPtr vm, virDomainHostdevDefPtr hostdev,
>> virDomainHostdevSubsysUSBPtr usbsrc = &hostdev->source.subsys.u.usb;
>> virDomainHostdevSubsysPCIPtr pcisrc = &hostdev->source.subsys.u.pci;
>> virDomainHostdevSubsysSCSIPtr scsisrc = &hostdev->source.subsys.u.scsi;
>> + virDomainHostdevSubsysHostPtr hostsrc = &hostdev->source.subsys.u.host;
>>
>> virUUIDFormat(vm->def->uuid, uuidstr);
>> if (!(vmname = virAuditEncode("vm", vm->def->name))) {
>> @@ -444,6 +445,12 @@ virDomainAuditHostdev(virDomainObjPtr vm, virDomainHostdevDefPtr hostdev,
>> }
>> break;
>> }
>> + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_HOST:
>> + if (VIR_STRDUP_QUIET(address, hostsrc->wwpn) < 0) {
>> + VIR_WARN("OOM while encoding audit message");
>> + goto cleanup;
>> + }
>> + break;
>> default:
>> VIR_WARN("Unexpected hostdev type while encoding audit message: %d",
>> hostdev->source.subsys.type);
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index b8a3366..75cacd9 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -2323,6 +2323,9 @@ void virDomainHostdevDefClear(virDomainHostdevDefPtr def)
>> } else {
>> VIR_FREE(scsisrc->u.host.adapter);
>> }
>> + } else if (def->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_HOST) {
>> + virDomainHostdevSubsysHostPtr hostsrc = &def->source.subsys.u.host;
>> + VIR_FREE(hostsrc->wwpn);
>> }
>> break;
>> }
>> @@ -6092,6 +6095,55 @@ virDomainHostdevSubsysSCSIDefParseXML(xmlNodePtr sourcenode,
>> return ret;
>> }
>>
>> +static int
>> +virDomainHostdevSubsysHostDefParseXML(xmlNodePtr sourcenode,
>> + virDomainHostdevDefPtr def)
>> +{
>> + char *protocol = NULL;
>> + virDomainHostdevSubsysHostPtr hostsrc = &def->source.subsys.u.host;
>> +
>> + if ((protocol = virXMLPropString(sourcenode, "protocol"))) {
>> + hostsrc->protocol =
>> + virDomainHostdevSubsysHostProtocolTypeFromString(protocol);
>> + if (hostsrc->protocol <= 0) {
>> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>> + _("Unknown scsi_host subsystem protocol '%s'"),
>> + protocol);
>> + goto cleanup;
>> + }
>> + }
> Since protocol is required the logic is :
>
> if (!(protocol = virXMLPropString(sourcenode, "protocol")))
> virReportError(... _("Missing vhost-scsi 'protocol' attribute'"));
> return -1;
> }
>
> if ((hostsrc->protocol =
> virDomainHostdevSubsysHostProtocolTypeFromString(protocol)) <= 0) {
> virReportError(...);
> goto cleanup;
> }
>
>> +
>> + switch ((virDomainHostdevSubsysHostProtocolType) hostsrc->protocol) {
>> + case VIR_DOMAIN_HOSTDEV_SUBSYS_HOST_PROTOCOL_TYPE_VHOST:
>> + if (!(hostsrc->wwpn = virXMLPropString(sourcenode, "wwpn"))) {
>> + virReportError(VIR_ERR_XML_ERROR, "%s",
>> + _("missing vhost-scsi hostdev source path name"));
> Technically it's missing the required 'wwpn' value
>
>> + goto cleanup;
>> + }
>> +
>> + if (!STRPREFIX(hostsrc->wwpn, "naa.") ||
>> + strlen(hostsrc->wwpn) != strlen("naa.") + 16) {
> I assume the wwpn minus the naa. needs to be valid - see virValidateWWN
> and just pass wwpn + 4
>
>> + virReportError(VIR_ERR_XML_ERROR, "%s", _("malformed 'wwpn' value"));
>> + goto cleanup;
>> + }
>> + break;
>> + case VIR_DOMAIN_HOSTDEV_SUBSYS_HOST_PROTOCOL_TYPE_NONE:
>> + case VIR_DOMAIN_HOSTDEV_SUBSYS_HOST_PROTOCOL_TYPE_LAST:
>> + virReportError(VIR_ERR_XML_ERROR,
>> + _("Invalid hostdev protocol '%s'"),
>> + virDomainHostdevSubsysHostProtocolTypeToString(def->source.subsys.type));
> Use hostsrc->protocol for the argument
>
> John
>
>> + goto cleanup;
>> + break;
>> + }
>> +
>> + return 0;
>> +
>> + cleanup:
>> + VIR_FREE(hostsrc->wwpn);
>> + VIR_FREE(protocol);
>> + return -1;
>> +}
>> +
>>
>> static int
>> virDomainHostdevDefParseXMLSubsys(xmlNodePtr node,
>> @@ -6216,6 +6268,11 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr node,
>> goto error;
>> break;
>>
>> + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_HOST:
>> + if (virDomainHostdevSubsysHostDefParseXML(sourcenode, def) < 0)
>> + goto error;
>> + break;
>> +
>> default:
>> virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>> _("address type='%s' not supported in hostdev interfaces"),
>> @@ -13908,7 +13965,13 @@ virDomainHostdevMatchSubsys(virDomainHostdevDefPtr a,
>> else
>> return virDomainHostdevMatchSubsysSCSIHost(a, b);
>> case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_HOST:
>> - /* Fall through for now */
>> + if (a->source.subsys.u.host.protocol !=
>> + b->source.subsys.u.host.protocol)
>> + return 0;
>> + if (STREQ(a->source.subsys.u.host.wwpn, b->source.subsys.u.host.wwpn))
>> + return 1;
>> + else
>> + return 0;
>> case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST:
>> return 0;
>> }
>> @@ -20815,9 +20878,11 @@ virDomainHostdevDefFormatSubsys(virBufferPtr buf,
>> unsigned int flags,
>> bool includeTypeInAddr)
>> {
>> + bool closedSource = false;
>> virDomainHostdevSubsysUSBPtr usbsrc = &def->source.subsys.u.usb;
>> virDomainHostdevSubsysPCIPtr pcisrc = &def->source.subsys.u.pci;
>> virDomainHostdevSubsysSCSIPtr scsisrc = &def->source.subsys.u.scsi;
>> + virDomainHostdevSubsysHostPtr hostsrc = &def->source.subsys.u.host;
>> virDomainHostdevSubsysSCSIHostPtr scsihostsrc = &scsisrc->u.host;
>> virDomainHostdevSubsysSCSIiSCSIPtr iscsisrc = &scsisrc->u.iscsi;
>>
>> @@ -20858,6 +20923,15 @@ virDomainHostdevDefFormatSubsys(virBufferPtr buf,
>> protocol, iscsisrc->path);
>> }
>>
>> + if (def->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_HOST) {
>> + const char *protocol =
>> + virDomainHostdevSubsysHostProtocolTypeToString(hostsrc->protocol);
>> + closedSource = true;
>> +
>> + virBufferAsprintf(buf, " protocol='%s' wwpn='%s'/",
>> + protocol, hostsrc->wwpn);
>> + }
>> +
>> virBufferAddLit(buf, ">\n");
>>
>> virBufferAdjustIndent(buf, 2);
>> @@ -20911,6 +20985,8 @@ virDomainHostdevDefFormatSubsys(virBufferPtr buf,
>> scsihostsrc->unit);
>> }
>> break;
>> + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_HOST:
>> + break;
>> default:
>> virReportError(VIR_ERR_INTERNAL_ERROR,
>> _("unexpected hostdev type %d"),
>> @@ -20926,7 +21002,8 @@ virDomainHostdevDefFormatSubsys(virBufferPtr buf,
>> }
>>
>> virBufferAdjustIndent(buf, -2);
>> - virBufferAddLit(buf, "</source>\n");
>> + if (!closedSource)
>> + virBufferAddLit(buf, "</source>\n");
>>
>> return 0;
>> }
>>
More information about the libvir-list
mailing list