This looks good to me. Acked-by: Kyle Mestery <kmestery@xxxxxxxxx> On Aug 14, 2012, at 2:04 AM, Laine Stump wrote: > virtPortProfile is now used by 4 different types of network devices > (NETWORK, BRIDGE, DIRECT, and HOSTDEV), and it's getting cumbersome to > replicate so much code in 4 different places just because each type > has the virtPortProfile in a slightly different place. This patch puts > a single virtPortProfile in a common place (outside the type-specific > union) in both virDomainNetDef and virDomainActualNetDef, and adjusts > the parse and format code (and the few other places where it is used) > accordingly. > > Note that when a <virtualport> element is found, the parse functions > verify that the interface is of a type that supports one, otherwise an > error is generated (CONFIG_UNSUPPORTED in the case of <interface>, and > INTERNAL in the case of <actual>, since the contents of <actual> are > always generated by libvirt itself). > --- > src/conf/domain_conf.c | 125 +++++++++++++++----------------------------- > src/conf/domain_conf.h | 10 ++-- > src/network/bridge_driver.c | 16 +++--- > src/qemu/qemu_driver.c | 2 +- > src/qemu/qemu_hotplug.c | 19 +++---- > 5 files changed, 63 insertions(+), 109 deletions(-) > > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c > index d8c0969..e239909 100644 > --- a/src/conf/domain_conf.c > +++ b/src/conf/domain_conf.c > @@ -1018,20 +1018,18 @@ virDomainActualNetDefFree(virDomainActualNetDefPtr def) > switch (def->type) { > case VIR_DOMAIN_NET_TYPE_BRIDGE: > VIR_FREE(def->data.bridge.brname); > - VIR_FREE(def->data.bridge.virtPortProfile); > break; > case VIR_DOMAIN_NET_TYPE_DIRECT: > VIR_FREE(def->data.direct.linkdev); > - VIR_FREE(def->data.direct.virtPortProfile); > break; > case VIR_DOMAIN_NET_TYPE_HOSTDEV: > virDomainHostdevDefClear(&def->data.hostdev.def); > - VIR_FREE(def->data.hostdev.virtPortProfile); > break; > default: > break; > } > > + VIR_FREE(def->virtPortProfile); > virNetDevBandwidthFree(def->bandwidth); > > VIR_FREE(def); > @@ -1059,14 +1057,12 @@ void virDomainNetDefFree(virDomainNetDefPtr def) > case VIR_DOMAIN_NET_TYPE_NETWORK: > VIR_FREE(def->data.network.name); > VIR_FREE(def->data.network.portgroup); > - VIR_FREE(def->data.network.virtPortProfile); > virDomainActualNetDefFree(def->data.network.actual); > break; > > case VIR_DOMAIN_NET_TYPE_BRIDGE: > VIR_FREE(def->data.bridge.brname); > VIR_FREE(def->data.bridge.ipaddr); > - VIR_FREE(def->data.bridge.virtPortProfile); > break; > > case VIR_DOMAIN_NET_TYPE_INTERNAL: > @@ -1075,12 +1071,10 @@ void virDomainNetDefFree(virDomainNetDefPtr def) > > case VIR_DOMAIN_NET_TYPE_DIRECT: > VIR_FREE(def->data.direct.linkdev); > - VIR_FREE(def->data.direct.virtPortProfile); > break; > > case VIR_DOMAIN_NET_TYPE_HOSTDEV: > virDomainHostdevDefClear(&def->data.hostdev.def); > - VIR_FREE(def->data.hostdev.virtPortProfile); > break; > > case VIR_DOMAIN_NET_TYPE_USER: > @@ -1088,6 +1082,7 @@ void virDomainNetDefFree(virDomainNetDefPtr def) > break; > } > > + VIR_FREE(def->virtPortProfile); > VIR_FREE(def->script); > VIR_FREE(def->ifname); > > @@ -4371,6 +4366,7 @@ virDomainActualNetDefParseXML(xmlNodePtr node, > int ret = -1; > xmlNodePtr save_ctxt = ctxt->node; > xmlNodePtr bandwidth_node = NULL; > + xmlNodePtr virtPortNode; > char *type = NULL; > char *mode = NULL; > char *addrtype = NULL; > @@ -4403,15 +4399,24 @@ virDomainActualNetDefParseXML(xmlNodePtr node, > goto error; > } > > - if (actual->type == VIR_DOMAIN_NET_TYPE_BRIDGE) { > - xmlNodePtr virtPortNode = virXPathNode("./virtualport", ctxt); > - if (virtPortNode && > - (!(actual->data.bridge.virtPortProfile = > - virNetDevVPortProfileParse(virtPortNode)))) > + virtPortNode = virXPathNode("./virtualport", ctxt); > + if (virtPortNode) { > + if (actual->type == VIR_DOMAIN_NET_TYPE_BRIDGE || > + actual->type == VIR_DOMAIN_NET_TYPE_DIRECT || > + actual->type == VIR_DOMAIN_NET_TYPE_HOSTDEV) { > + if (!(actual->virtPortProfile > + = virNetDevVPortProfileParse(virtPortNode))) { > + goto error; > + } > + } else { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("<virtualport> element unsupported for type='%s'" > + " in interface's <actual> element"), type); > goto error; > - } else if (actual->type == VIR_DOMAIN_NET_TYPE_DIRECT) { > - xmlNodePtr virtPortNode; > + } > + } > > + if (actual->type == VIR_DOMAIN_NET_TYPE_DIRECT) { > actual->data.direct.linkdev = virXPathString("string(./source[1]/@dev)", ctxt); > > mode = virXPathString("string(./source[1]/@mode)", ctxt); > @@ -4425,14 +4430,7 @@ virDomainActualNetDefParseXML(xmlNodePtr node, > } > actual->data.direct.mode = m; > } > - > - virtPortNode = virXPathNode("./virtualport", ctxt); > - if (virtPortNode && > - (!(actual->data.direct.virtPortProfile = > - virNetDevVPortProfileParse(virtPortNode)))) > - goto error; > } else if (actual->type == VIR_DOMAIN_NET_TYPE_HOSTDEV) { > - xmlNodePtr virtPortNode = virXPathNode("./virtualport", ctxt); > virDomainHostdevDefPtr hostdev = &actual->data.hostdev.def; > > hostdev->parent.type = VIR_DOMAIN_DEVICE_NET; > @@ -4453,12 +4451,6 @@ virDomainActualNetDefParseXML(xmlNodePtr node, > hostdev, flags) < 0) { > goto error; > } > - > - if (virtPortNode && > - (!(actual->data.hostdev.virtPortProfile = > - virNetDevVPortProfileParse(virtPortNode)))) { > - goto error; > - } > } > > bandwidth_node = virXPathNode("./bandwidth", ctxt); > @@ -4518,7 +4510,6 @@ virDomainNetDefParseXML(virCapsPtr caps, > char *linkstate = NULL; > char *addrtype = NULL; > virNWFilterHashTablePtr filterparams = NULL; > - virNetDevVPortProfilePtr virtPort = NULL; > virDomainActualNetDefPtr actual = NULL; > xmlNodePtr oldnode = ctxt->node; > int ret; > @@ -4565,14 +4556,22 @@ virDomainNetDefParseXML(virCapsPtr caps, > xmlStrEqual(cur->name, BAD_CAST "source")) { > dev = virXMLPropString(cur, "dev"); > mode = virXMLPropString(cur, "mode"); > - } else if (!virtPort && > - (def->type == VIR_DOMAIN_NET_TYPE_DIRECT || > - def->type == VIR_DOMAIN_NET_TYPE_NETWORK || > - def->type == VIR_DOMAIN_NET_TYPE_BRIDGE || > - def->type == VIR_DOMAIN_NET_TYPE_HOSTDEV) && > - xmlStrEqual(cur->name, BAD_CAST "virtualport")) { > - if (!(virtPort = virNetDevVPortProfileParse(cur))) > + } else if (!def->virtPortProfile > + && xmlStrEqual(cur->name, BAD_CAST "virtualport")) { > + if (def->type == VIR_DOMAIN_NET_TYPE_NETWORK || > + def->type == VIR_DOMAIN_NET_TYPE_BRIDGE || > + def->type == VIR_DOMAIN_NET_TYPE_DIRECT || > + def->type == VIR_DOMAIN_NET_TYPE_HOSTDEV) { > + if (!(def->virtPortProfile > + = virNetDevVPortProfileParse(cur))) { > + goto error; > + } > + } else { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > + _("<virtualport> element unsupported for" > + " <interface type='%s'>"), type); > goto error; > + } > } else if (!network && > (def->type == VIR_DOMAIN_NET_TYPE_SERVER || > def->type == VIR_DOMAIN_NET_TYPE_CLIENT || > @@ -4689,8 +4688,6 @@ virDomainNetDefParseXML(virCapsPtr caps, > network = NULL; > def->data.network.portgroup = portgroup; > portgroup = NULL; > - def->data.network.virtPortProfile = virtPort; > - virtPort = NULL; > def->data.network.actual = actual; > actual = NULL; > break; > @@ -4719,8 +4716,6 @@ virDomainNetDefParseXML(virCapsPtr caps, > def->data.bridge.ipaddr = address; > address = NULL; > } > - def->data.bridge.virtPortProfile = virtPort; > - virtPort = NULL; > break; > > case VIR_DOMAIN_NET_TYPE_CLIENT: > @@ -4784,8 +4779,6 @@ virDomainNetDefParseXML(virCapsPtr caps, > def->data.direct.mode = VIR_NETDEV_MACVLAN_MODE_VEPA; > } > > - def->data.direct.virtPortProfile = virtPort; > - virtPort = NULL; > def->data.direct.linkdev = dev; > dev = NULL; > > @@ -4814,8 +4807,6 @@ virDomainNetDefParseXML(virCapsPtr caps, > hostdev, flags) < 0) { > goto error; > } > - def->data.hostdev.virtPortProfile = virtPort; > - virtPort = NULL; > break; > > case VIR_DOMAIN_NET_TYPE_USER: > @@ -4938,7 +4929,6 @@ cleanup: > VIR_FREE(port); > VIR_FREE(ifname); > VIR_FREE(dev); > - VIR_FREE(virtPort); > virDomainActualNetDefFree(actual); > VIR_FREE(script); > VIR_FREE(bridge); > @@ -11582,12 +11572,6 @@ virDomainActualNetDefFormat(virBufferPtr buf, > case VIR_DOMAIN_NET_TYPE_BRIDGE: > virBufferEscapeString(buf, " <source bridge='%s'/>\n", > def->data.bridge.brname); > - if (def->data.bridge.virtPortProfile) { > - virBufferAdjustIndent(buf, 6); > - if (virNetDevVPortProfileFormat(def->data.bridge.virtPortProfile, buf) < 0) > - return -1; > - virBufferAdjustIndent(buf, -6); > - } > break; > > case VIR_DOMAIN_NET_TYPE_DIRECT: > @@ -11604,10 +11588,6 @@ virDomainActualNetDefFormat(virBufferPtr buf, > return ret; > } > virBufferAsprintf(buf, " mode='%s'/>\n", mode); > - virBufferAdjustIndent(buf, 8); > - if (virNetDevVPortProfileFormat(def->data.direct.virtPortProfile, buf) < 0) > - goto error; > - virBufferAdjustIndent(buf, -8); > break; > > case VIR_DOMAIN_NET_TYPE_HOSTDEV: > @@ -11616,10 +11596,6 @@ virDomainActualNetDefFormat(virBufferPtr buf, > flags, true) < 0) { > return -1; > } > - if (virNetDevVPortProfileFormat(def->data.hostdev.virtPortProfile, > - buf) < 0) { > - return -1; > - } > virBufferAdjustIndent(buf, -8); > break; > > @@ -11632,6 +11608,8 @@ virDomainActualNetDefFormat(virBufferPtr buf, > } > > virBufferAdjustIndent(buf, 8); > + if (virNetDevVPortProfileFormat(def->virtPortProfile, buf) < 0) > + return -1; > if (virNetDevBandwidthFormat(def->bandwidth, buf) < 0) > goto error; > virBufferAdjustIndent(buf, -8); > @@ -11675,10 +11653,6 @@ virDomainNetDefFormat(virBufferPtr buf, > virBufferEscapeString(buf, " portgroup='%s'", > def->data.network.portgroup); > virBufferAddLit(buf, "/>\n"); > - virBufferAdjustIndent(buf, 6); > - if (virNetDevVPortProfileFormat(def->data.network.virtPortProfile, buf) < 0) > - return -1; > - virBufferAdjustIndent(buf, -6); > if ((flags & VIR_DOMAIN_XML_INTERNAL_ACTUAL_NET) && > (virDomainActualNetDefFormat(buf, def->data.network.actual, flags) < 0)) > return -1; > @@ -11698,12 +11672,6 @@ virDomainNetDefFormat(virBufferPtr buf, > if (def->data.bridge.ipaddr) > virBufferAsprintf(buf, " <ip address='%s'/>\n", > def->data.bridge.ipaddr); > - if (def->data.bridge.virtPortProfile) { > - virBufferAdjustIndent(buf, 6); > - if (virNetDevVPortProfileFormat(def->data.bridge.virtPortProfile, buf) < 0) > - return -1; > - virBufferAdjustIndent(buf, -6); > - } > break; > > case VIR_DOMAIN_NET_TYPE_SERVER: > @@ -11728,10 +11696,6 @@ virDomainNetDefFormat(virBufferPtr buf, > virBufferAsprintf(buf, " mode='%s'", > virNetDevMacVLanModeTypeToString(def->data.direct.mode)); > virBufferAddLit(buf, "/>\n"); > - virBufferAdjustIndent(buf, 6); > - if (virNetDevVPortProfileFormat(def->data.direct.virtPortProfile, buf) < 0) > - return -1; > - virBufferAdjustIndent(buf, -6); > break; > > case VIR_DOMAIN_NET_TYPE_HOSTDEV: > @@ -11740,10 +11704,6 @@ virDomainNetDefFormat(virBufferPtr buf, > flags, true) < 0) { > return -1; > } > - if (virNetDevVPortProfileFormat(def->data.hostdev.virtPortProfile, > - buf) < 0) { > - return -1; > - } > virBufferAdjustIndent(buf, -6); > break; > > @@ -11752,6 +11712,11 @@ virDomainNetDefFormat(virBufferPtr buf, > break; > } > > + virBufferAdjustIndent(buf, 6); > + if (virNetDevVPortProfileFormat(def->virtPortProfile, buf) < 0) > + return -1; > + virBufferAdjustIndent(buf, -6); > + > virBufferEscapeString(buf, " <script path='%s'/>\n", > def->script); > if (def->ifname && > @@ -15082,21 +15047,17 @@ virDomainNetGetActualVirtPortProfile(virDomainNetDefPtr iface) > { > switch (iface->type) { > case VIR_DOMAIN_NET_TYPE_DIRECT: > - return iface->data.direct.virtPortProfile; > case VIR_DOMAIN_NET_TYPE_BRIDGE: > - return iface->data.bridge.virtPortProfile; > case VIR_DOMAIN_NET_TYPE_HOSTDEV: > - return iface->data.hostdev.virtPortProfile; > + return iface->virtPortProfile; > case VIR_DOMAIN_NET_TYPE_NETWORK: > if (!iface->data.network.actual) > return NULL; > switch (iface->data.network.actual->type) { > case VIR_DOMAIN_NET_TYPE_DIRECT: > - return iface->data.network.actual->data.direct.virtPortProfile; > case VIR_DOMAIN_NET_TYPE_BRIDGE: > - return iface->data.network.actual->data.bridge.virtPortProfile; > case VIR_DOMAIN_NET_TYPE_HOSTDEV: > - return iface->data.network.actual->data.hostdev.virtPortProfile; > + return iface->data.network.actual->virtPortProfile; > default: > return NULL; > } > diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h > index 3f25ad2..f36f7d6 100644 > --- a/src/conf/domain_conf.h > +++ b/src/conf/domain_conf.h > @@ -770,18 +770,16 @@ struct _virDomainActualNetDef { > union { > struct { > char *brname; > - virNetDevVPortProfilePtr virtPortProfile; > } bridge; > struct { > char *linkdev; > int mode; /* enum virMacvtapMode from util/macvtap.h */ > - virNetDevVPortProfilePtr virtPortProfile; > } direct; > struct { > virDomainHostdevDef def; > - virNetDevVPortProfilePtr virtPortProfile; > } hostdev; > } data; > + virNetDevVPortProfilePtr virtPortProfile; > virNetDevBandwidthPtr bandwidth; > }; > > @@ -810,7 +808,6 @@ struct _virDomainNetDef { > struct { > char *name; > char *portgroup; > - virNetDevVPortProfilePtr virtPortProfile; > /* actual has info about the currently used physical > * device (if the network is of type > * bridge/private/vepa/passthrough). This is saved in the > @@ -824,7 +821,6 @@ struct _virDomainNetDef { > struct { > char *brname; > char *ipaddr; > - virNetDevVPortProfilePtr virtPortProfile; > } bridge; > struct { > char *name; > @@ -832,13 +828,13 @@ struct _virDomainNetDef { > struct { > char *linkdev; > int mode; /* enum virMacvtapMode from util/macvtap.h */ > - virNetDevVPortProfilePtr virtPortProfile; > } direct; > struct { > virDomainHostdevDef def; > - virNetDevVPortProfilePtr virtPortProfile; > } hostdev; > } data; > + /* virtPortProfile is used by network/bridge/direct/hostdev */ > + virNetDevVPortProfilePtr virtPortProfile; > struct { > bool sndbuf_specified; > unsigned long sndbuf; > diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c > index a5046f1..d9646fb 100644 > --- a/src/network/bridge_driver.c > +++ b/src/network/bridge_driver.c > @@ -2854,8 +2854,8 @@ networkAllocateActualDevice(virDomainNetDefPtr iface) > } > > /* Find the most specific virtportprofile and copy it */ > - if (iface->data.network.virtPortProfile) { > - virtport = iface->data.network.virtPortProfile; > + if (iface->virtPortProfile) { > + virtport = iface->virtPortProfile; > } else { > if (portgroup) > virtport = portgroup->virtPortProfile; > @@ -2863,14 +2863,14 @@ networkAllocateActualDevice(virDomainNetDefPtr iface) > virtport = netdef->virtPortProfile; > } > if (virtport) { > - if (VIR_ALLOC(iface->data.network.actual->data.direct.virtPortProfile) < 0) { > + if (VIR_ALLOC(iface->data.network.actual->virtPortProfile) < 0) { > virReportOOMError(); > goto cleanup; > } > /* There are no pointers in a virtualPortProfile, so a shallow copy > * is sufficient > */ > - *iface->data.network.actual->data.direct.virtPortProfile = *virtport; > + *iface->data.network.actual->virtPortProfile = *virtport; > } > > /* If there is only a single device, just return it (caller will detect > @@ -2935,8 +2935,8 @@ networkAllocateActualDevice(virDomainNetDefPtr iface) > } > } > } else if ((netdef->forwardType == VIR_NETWORK_FORWARD_PRIVATE) && > - iface->data.network.actual->data.direct.virtPortProfile && > - (iface->data.network.actual->data.direct.virtPortProfile->virtPortType > + iface->data.network.actual->virtPortProfile && > + (iface->data.network.actual->virtPortProfile->virtPortType > == VIR_NETDEV_VPORT_PROFILE_8021QBH)) { > > /* pick first dev with 0 usageCount */ > @@ -3068,8 +3068,8 @@ networkNotifyActualDevice(virDomainNetDefPtr iface) > if ((dev->usageCount > 0) && > ((netdef->forwardType == VIR_NETWORK_FORWARD_PASSTHROUGH) || > ((netdef->forwardType == VIR_NETWORK_FORWARD_PRIVATE) && > - iface->data.network.actual->data.direct.virtPortProfile && > - (iface->data.network.actual->data.direct.virtPortProfile->virtPortType > + iface->data.network.actual->virtPortProfile && > + (iface->data.network.actual->virtPortProfile->virtPortType > == VIR_NETDEV_VPORT_PROFILE_8021QBH)))) { > virReportError(VIR_ERR_INTERNAL_ERROR, > _("network '%s' claims dev='%s' is already in use by a different domain"), > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c > index 9bf89bb..21d8699 100644 > --- a/src/qemu/qemu_driver.c > +++ b/src/qemu/qemu_driver.c > @@ -4763,7 +4763,6 @@ static char *qemuDomainXMLToNative(virConnectPtr conn, > } > } else if (net->type == VIR_DOMAIN_NET_TYPE_DIRECT) { > VIR_FREE(net->data.direct.linkdev); > - VIR_FREE(net->data.direct.virtPortProfile); > > memset(net, 0, sizeof(*net)); > > @@ -4783,6 +4782,7 @@ static char *qemuDomainXMLToNative(virConnectPtr conn, > net->data.ethernet.dev = brname; > net->data.ethernet.ipaddr = ipaddr; > } > + VIR_FREE(net->virtPortProfile); > net->info.bootIndex = bootIndex; > } > for (i = 0 ; i < def->ngraphics ; i++) { > diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c > index e128e58..339906e 100644 > --- a/src/qemu/qemu_hotplug.c > +++ b/src/qemu/qemu_hotplug.c > @@ -1340,6 +1340,11 @@ int qemuDomainChangeNet(struct qemud_driver *driver, > return -1; > } > > + if (!virNetDevVPortProfileEqual(olddev->virtPortProfile, dev->virtPortProfile)) { > + virReportError(VIR_ERR_NO_SUPPORT, "%s", > + _("cannot change <virtualport> settings")); > + } > + > switch (olddev->type) { > case VIR_DOMAIN_NET_TYPE_USER: > break; > @@ -1367,8 +1372,7 @@ int qemuDomainChangeNet(struct qemud_driver *driver, > > case VIR_DOMAIN_NET_TYPE_NETWORK: > if (STRNEQ_NULLABLE(olddev->data.network.name, dev->data.network.name) || > - STRNEQ_NULLABLE(olddev->data.network.portgroup, dev->data.network.portgroup) || > - !virNetDevVPortProfileEqual(olddev->data.network.virtPortProfile, dev->data.network.virtPortProfile)) { > + STRNEQ_NULLABLE(olddev->data.network.portgroup, dev->data.network.portgroup)) { > virReportError(VIR_ERR_NO_SUPPORT, "%s", > _("cannot modify network device configuration")); > return -1; > @@ -1377,13 +1381,7 @@ int qemuDomainChangeNet(struct qemud_driver *driver, > break; > > case VIR_DOMAIN_NET_TYPE_BRIDGE: > - /* allow changing brname, but not portprofile */ > - if (!virNetDevVPortProfileEqual(olddev->data.bridge.virtPortProfile, > - dev->data.bridge.virtPortProfile)) { > - virReportError(VIR_ERR_NO_SUPPORT, "%s", > - _("cannot modify bridge network device configuration")); > - return -1; > - } > + /* allow changing brname */ > break; > > case VIR_DOMAIN_NET_TYPE_INTERNAL: > @@ -1396,8 +1394,7 @@ int qemuDomainChangeNet(struct qemud_driver *driver, > > case VIR_DOMAIN_NET_TYPE_DIRECT: > if (STRNEQ_NULLABLE(olddev->data.direct.linkdev, dev->data.direct.linkdev) || > - olddev->data.direct.mode != dev->data.direct.mode || > - !virNetDevVPortProfileEqual(olddev->data.direct.virtPortProfile, dev->data.direct.virtPortProfile)) { > + olddev->data.direct.mode != dev->data.direct.mode) { > virReportError(VIR_ERR_NO_SUPPORT, "%s", > _("cannot modify direct network device configuration")); > return -1; > -- > 1.7.11.2 > > -- > libvir-list mailing list > libvir-list@xxxxxxxxxx > https://www.redhat.com/mailman/listinfo/libvir-list -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list