On Thu, Mar 21, 2019 at 09:07:36PM -0400, Cole Robinson wrote:
> On 3/19/19 8:46 AM, Daniel P. Berrangé wrote:
> > Ports allocated on virtual networks with type=nat|route|open all get
> > given an actual type of 'network'.
> >
> > Only ports in networks with type=bridge use an actual type of 'bridge'.
> >
> > This distinction makes little sense since the virtualization drivers
> > will treat both actual types in exactly the same way, as they're all
> > just bridge devices a VM needs to be connected to.
> >
> > This doesn't affect user visible XML since the "actual" device XML
> > is internal only, but we need code to convert the data upgrades.
> >
> > Signed-off-by: Daniel P. Berrangé <berrange@xxxxxxxxxx>
> > ---
> > src/network/bridge_driver.c | 20 ++++++++++++--------
> > 1 file changed, 12 insertions(+), 8 deletions(-)
> >
>
> Conceptually this makes sense. I wonder what the original motivation for
> the differentiation was. This also makes me wish there was a separate
> enum for the internal actual->type field, it would make ambiguous code
> much more readable
>
> > diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> > index 4770dd1e4e..40122612c8 100644
> > --- a/src/network/bridge_driver.c
> > +++ b/src/network/bridge_driver.c
> > @@ -4454,11 +4454,7 @@ networkAllocateActualDevice(virNetworkPtr net,
> > case VIR_NETWORK_FORWARD_NAT:
> > case VIR_NETWORK_FORWARD_ROUTE:
> > case VIR_NETWORK_FORWARD_OPEN:
> > - /* for these forward types, the actual net type really *is*
> > - * NETWORK; we just keep the info from the portgroup in
> > - * iface->data.network.actual
> > - */
> > - iface->data.network.actual->type = VIR_DOMAIN_NET_TYPE_NETWORK;
> > + iface->data.network.actual->type = VIR_DOMAIN_NET_TYPE_BRIDGE;
> >
> > /* we also store the bridge device and macTableManager settings
> > * in iface->data.network.actual->data.bridge for later use
> > @@ -4832,6 +4828,15 @@ networkNotifyActualDevice(virNetworkPtr net,
> > netdef->bridge) < 0))
> > goto error;
> >
> > + /* Older libvirtd uses actualType==network, but we now
> > + * just use actualType==bridge, as nothing needs to
> > + * distinguish the two cases, and this simplifies virt
> > + * drive code */
> > + if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK) {
> > + iface->data.network.actual->type = VIR_DOMAIN_NET_TYPE_BRIDGE;
> > + actualType = VIR_DOMAIN_NET_TYPE_BRIDGE;
> > + }
> > +
>
> Why do this here and not at the status XML parse time? This code path is
> only called on reconnected devices correct? So that should always be
> hitting the status XML parser too AFAICT. That would make it easier to
> audit any TYPE_NETWORK paths in the status parser as well
This has to come after the code that is a few lines higher outside
the diffstat. So the XML parser is too early. That said we might
delete the earlier code entirely.
> qemu_driver.c processNicRxFilterChangedEvent also has a
>
> if (virDomainNetGetActualType(def) == VIR_DOMAIN_NET_TYPE_NETWORK) {
>
> which looks sketchy after this. And it's involving bandwidth stuff so it
> may affect the previous patch too.
Yeah, that looks like something i missed.
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
