Re: [libvirt] [PATCH v3 17/36] conf: add APIs to convert virDomainNetDef to virNetworkPortDef

On 3/19/19 8:46 AM, Daniel P. Berrangé wrote: > Helper APIs are needed to > > - Populate basic virNetworkPortDef from virDomainNetDef > - Set a virDomainActualNetDef from virNetworkPortDef > - Populate a full virNetworkPortDef from virDomainActualNetDef > > Signed-off-by: Daniel P. Berrangé <berrange(a)redhat.com&gt; > --- > src/conf/domain_conf.c | 272 +++++++++++++++++++++++++++++++++++++++ > src/conf/domain_conf.h | 17 +++ > src/libvirt_private.syms | 3 + > 3 files changed, 292 insertions(+) > > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c > index d283feaca6..ee4d586d77 100644 > --- a/src/conf/domain_conf.c > +++ b/src/conf/domain_conf.c > @@ -39,6 +39,7 @@ > #include "virbuffer.h" > #include "virlog.h" > #include "nwfilter_conf.h" > +#include "virnetworkportdef.h" > #include "storage_conf.h" > #include "virstoragefile.h" > #include "virfile.h" > @@ -30161,6 +30162,277 @@ virDomainNetTypeSharesHostView(const virDomainNetDef *net) > return false; > } > +virNetworkPortDefPtr > +virDomainNetDefToNetworkPort(virDomainDefPtr dom, > + virDomainNetDefPtr iface) > +{ > + virNetworkPortDefPtr port; > + > + if (iface->type != VIR_DOMAIN_NET_TYPE_NETWORK) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("Expected an interface of type 'network' not '%s'"), > + virDomainNetTypeToString(iface->type)); > + return NULL; > + } > + > + if (VIR_ALLOC(port) < 0) > + return NULL; > + > + virUUIDGenerate(port->uuid); > + > + memcpy(port->owneruuid, dom->uuid, VIR_UUID_BUFLEN); > + if (VIR_STRDUP(port->ownername, dom->name) < 0) > + goto error; It's not important, but  was there any reason you put the above two items out of order wrt the virNetworkPortDef struct? Having them in order would make it simpler to verify nothing had been missed. > + > + if (VIR_STRDUP(port->group, iface->data.network.portgroup) < 0) > + goto error; > + > + memcpy(&port->mac, &iface->mac, VIR_MAC_BUFLEN); > + > + if (virNetDevVPortProfileCopy(&port->virtPortProfile, iface->virtPortProfile) < 0) > + goto error; > + > + if (virNetDevBandwidthCopy(&port->bandwidth, iface->bandwidth) < 0) > + goto error; > + > + if (virNetDevVlanCopy(&port->vlan, &iface->vlan) < 0) > + goto error; > + > + port->trustGuestRxFilters = iface->trustGuestRxFilters; > + > + return port; > + > + error: > + virNetworkPortDefFree(port); > + return NULL; > +} > + > +int > +virDomainNetDefActualFromNetworkPort(virDomainNetDefPtr iface, > + virNetworkPortDefPtr port) > +{ > + virDomainActualNetDefPtr actual = NULL; > + > + if (iface->type != VIR_DOMAIN_NET_TYPE_NETWORK) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("Expected an interface of type 'network' not '%s'"), > + virDomainNetTypeToString(iface->type)); > + return -1; > + } > + > + if (VIR_ALLOC(actual) < 0) > + return -1; > + > + switch ((virNetworkPortPlugType)port->plugtype) { > + case VIR_NETWORK_PORT_PLUG_TYPE_NONE: > + break; > + > + case VIR_NETWORK_PORT_PLUG_TYPE_BRIDGE: > + if (VIR_STRDUP(actual->data.bridge.brname, > + port->plug.bridge.brname) < 0) > + goto error; > + actual->data.bridge.macTableManager = port->plug.bridge.macTableManager; > + break; none of the cases set actual->type. > + > + case VIR_NETWORK_PORT_PLUG_TYPE_DIRECT: > + if (VIR_STRDUP(actual->data.direct.linkdev, > + port->plug.direct.linkdev) < 0) > + goto error; > + actual->data.direct.mode = port->plug.direct.mode; > + break; > + > + case VIR_NETWORK_PORT_PLUG_TYPE_HOSTDEV_PCI: > + actual->data.hostdev.def.parent = iface; > + actual->data.hostdev.def.info = &iface->info; Again, it would be simpler to verify if the assignments were in the same order as the attributes are in the definition of virDomainHostdevDef. (It appears there are some that aren't being set (startupPolicy, missing, readonly, shareable, origStates), but that's just because they're never used in this context anyway, (as proven by the fact that they don't have a counterpart in virNetworkPort. > + actual->data.hostdev.def.mode = VIR_DOMAIN_HOSTDEV_MODE_SUBSYS; > + actual->data.hostdev.def.managed = port->plug.hostdevpci.managed; > + actual->data.hostdev.def.source.subsys.type = VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI; > + actual->data.hostdev.def.source.subsys.u.pci.addr = port->plug.hostdevpci.addr; > + switch ((virNetworkForwardDriverNameType)port->plug.hostdevpci.driver) { > + case VIR_NETWORK_FORWARD_DRIVER_NAME_DEFAULT: > + actual->data.hostdev.def.source.subsys.u.pci.backend = > + VIR_DOMAIN_HOSTDEV_PCI_BACKEND_DEFAULT; > + break; > + > + case VIR_NETWORK_FORWARD_DRIVER_NAME_KVM: > + actual->data.hostdev.def.source.subsys.u.pci.backend = > + VIR_DOMAIN_HOSTDEV_PCI_BACKEND_KVM; > + break; Sigh. More legacy KVM pci device assignment stuff. > + > + case VIR_NETWORK_FORWARD_DRIVER_NAME_VFIO: > + actual->data.hostdev.def.source.subsys.u.pci.backend = > + VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO; > + break; > + > + case VIR_NETWORK_FORWARD_DRIVER_NAME_LAST: > + default: > + virReportEnumRangeError(virNetworkForwardDriverNameType, > + port->plug.hostdevpci.driver); > + goto error; > + } > + > + break; > + > + case VIR_NETWORK_PORT_PLUG_TYPE_LAST: > + default: > + virReportEnumRangeError(virNetworkPortPlugType, port->plugtype); > + goto error; > + } > + > + if (virNetDevVPortProfileCopy(&actual->virtPortProfile, port->virtPortProfile) < 0) > + goto error; > + > + if (virNetDevBandwidthCopy(&actual->bandwidth, port->bandwidth) < 0) > + goto error; > + > + if (virNetDevVlanCopy(&actual->vlan, &port->vlan) < 0) > + goto error; > + > + actual->class_id = port->class_id; > + actual->trustGuestRxFilters = port->trustGuestRxFilters; > + > + virDomainActualNetDefFree(iface->data.network.actual); > + iface->data.network.actual = actual; > + > + return 0; > + > + error: > + virDomainActualNetDefFree(actual); > + return -1; > +} > + > +virNetworkPortDefPtr > +virDomainNetDefActualToNetworkPort(virDomainDefPtr dom, > + virDomainNetDefPtr iface) > +{ > + virDomainActualNetDefPtr actual; > + virNetworkPortDefPtr port; > + > + if (!iface->data.network.actual) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("Missing actual data for interface '%s'"), > + iface->ifname); > + return NULL; > + } > + > + actual = iface->data.network.actual; > + > + if (iface->type != VIR_DOMAIN_NET_TYPE_NETWORK) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("Expected an interface of type 'network' not '%s'"), > + virDomainNetTypeToString(iface->type)); > + return NULL; > + } > + > + if (VIR_ALLOC(port) < 0) > + return NULL; > + > + /* Bad - we need to preserve original port uuid */ So is this acceptable as-is because of the limited ways you end up using  virDomainNetDefActualToNetworkPort()? Or is it something that needs to be fixed?