On Thu, Mar 21, 2019 at 09:14:13PM -0400, Cole Robinson wrote:
> On 3/19/19 8:46 AM, Daniel P. Berrangé wrote:
> > Signed-off-by: Daniel P. Berrangé <berrange@xxxxxxxxxx>
> > ---
> > src/libvirt_private.syms | 1 +
> > src/util/virnetdevtap.c | 69 ++++++++++++++++++++++++++++++++++++++++
> > src/util/virnetdevtap.h | 12 +++++++
> > 3 files changed, 82 insertions(+)
> >
> > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> > index d9494a04bb..6f5a734fdb 100644
> > --- a/src/libvirt_private.syms
> > +++ b/src/libvirt_private.syms
> > @@ -2461,6 +2461,7 @@ virNetDevTapDelete;
> > virNetDevTapGetName;
> > virNetDevTapGetRealDeviceName;
> > virNetDevTapInterfaceStats;
> > +virNetDevTapReattachBridge;
> >
> >
> > # util/virnetdevveth.h
> > diff --git a/src/util/virnetdevtap.c b/src/util/virnetdevtap.c
> > index 972f3405aa..0484c7c5a4 100644
> > --- a/src/util/virnetdevtap.c
> > +++ b/src/util/virnetdevtap.c
> > @@ -553,6 +553,75 @@ virNetDevTapAttachBridge(const char *tapname,
> > }
> >
> >
> > +/**
> > + * virNetDevTapReattachBridge:
> > + * @tapname: the tap interface name (or name template)
> > + * @brname: the bridge name
> > + * @macaddr: desired MAC address
> > + * @virtPortProfile: bridge/port specific configuration
> > + * @virtVlan: vlan tag info
> > + * @mtu: requested MTU for port (or 0 for "default")
> > + * @actualMTU: MTU actually set for port (after accounting for bridge's MTU)
> > + *
> > + * Ensures that the tap device (@tapname) is connected to the bridge
> > + * (@brname), potentially removing it from any existing bridge that
> > + * does not match.
> > + *
> > + * Returns 0 in case of success or -1 on failure
> > + */
> > +int
> > +virNetDevTapReattachBridge(const char *tapname,
> > + const char *brname,
> > + const virMacAddr *macaddr,
> > + const unsigned char *vmuuid,
> > + virNetDevVPortProfilePtr virtPortProfile,
> > + virNetDevVlanPtr virtVlan,
> > + unsigned int mtu,
> > + unsigned int *actualMTU)
> > +{
> > + bool useOVS = false;
> > + char *master = NULL;
> > +
>
> Could use VIR_AUTOFREE to simplify things a bit
>
> > + if (virNetDevGetMaster(tapname, &master) < 0)
> > + return -1;
> > +
> > + /* IFLA_MASTER for a tap on an OVS switch is always "ovs-system" */
> > + if (STREQ_NULLABLE(master, "ovs-system")) {
> > + useOVS = true;
> > + VIR_FREE(master);
> > + if (virNetDevOpenvswitchInterfaceGetMaster(tapname, &master) < 0)
> > + return -1;
> > + }
> > +
> > + /* Nothing more todo if we're on the right bridge already */
> > + if (STREQ_NULLABLE(brname, master))
> > + return 0;
> > +
> > + /* disconnect from current (incorrect) bridge, if any */
> > + if (master) {
> > + int ret;
> > + VIR_INFO("Removing %s from %s", tapname, master);
> > + if (useOVS)
> > + ret = virNetDevOpenvswitchRemovePort(master, tapname);
> > + else
> > + ret = virNetDevBridgeRemovePort(master, tapname);
> > + VIR_FREE(master);
> > + if (ret < 0)
> > + return -1;
> > + }
> > +
>
> These were non-fatal in the original code. Is it okay to change?
That's broken code IMHO. If we fail to remove the existing bridge
port, then we are about to fail to attach to the new bridge, because
a nic can only be part of 1 bridge at any time.
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
