Re: [PATCH v3 07/10] conf: Wire up the vhost-scsi connection from/to XML

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





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@xxxxxxxxxxxxxxxxxx>
---
  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;
  }


--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]