On Fri, Mar 22, 2019 at 01:11:34PM -0400, Laine Stump wrote:
> 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@xxxxxxxxxx>
> > ---
> > 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?
Hm, I swear I fixed that, but perhapos I've mistakenly squashed into
a later patch in this series.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
