On Thu, Oct 17, 2019 at 21:59:49 -0400, Laine Stump wrote: > From: Laine Stump <laine@xxxxxxxxxx> > > Remember when the MAC address of libvirt-created bridges weren't > stable, and changed as guests were started and stopped? Pepperidge > Farms remembers. > > (No, I would never actually push a comment like that! Just wanted to > get your attention). > > When libvirt first implemented a stable and configurable MAC address > for the bridges created for libvirt virtual networks (commit > 5754dbd56d, in libvirt v8.8.0) most distro stable releases didn't Looks like we can finally travel in time :-) v0.8.8 is the correct version. > support explicitly setting the MAC address of a bridge; the bridge > just always assumed the lowest numbered MAC of all attached > interfaces. Because of this, we stabilized the bridge MAC address by > creating a "dummy" tap interface with a MAC address guaranteed to be > lower than any of the guest tap devices' MACs (which all started with > 0xFE, so it's not difficult to do) and attached it to the bridge - > this was the inception of the "virbr0-nic" device that has confused so > many people over the years. > > Even though the linux kernel had recently gained support for > explicitly setting a bridge MAC, we deemed it unnecessary to set the > MAC that way, because the other (indirect) method worked everywhere. > > But recently there have been reports that the bridge MAC address was > not following the setting in the network config, and mismatched the > MAC of the dummy tap device (which was still correct). It turns out > that this is due to a change in systemd/udev that persists whatever > MAC address is set for a bridge when it's initially started: > > https://github.com/systemd/systemd/blob/master/NEWS#L499 > > which was the result of: > > https://github.com/systemd/systemd/issues/3374 > > (apparently if there is no MAC saved for a bridge by the name of a > bridge being created, the random MAC generated during creation is > saved, and then that same MAC is used to explicitly set the MAC each > time it is created). Once a bridge has an explicitly set MAC, the "use > the lowest numbered MAC of attached devices" rule is ignored, so our > dummy tap device is like the goggles - it does nothing! (well, almost). > > We could whine about changes in default behavior, etc. etc., but > because the change was in response to actual user problems, that seems > likely a fruitless task. Fortunately, time has marched on, and even > distro releases that are old enough that they are no longer supported > by upstream libvirt (e.g. RHEL6) have support for explicitly setting a > bridge device MAC address, either during creation or with a separate > ioctl after creation, so we can now do that. > > The method is to add a mac arg to virNetDevBridgeCreate(). In the case > of platforms where the bridge is created with a netlink RTM_NEWLINK > message, we just add the desired mac to the message. For platforms > that still use an ioctl (either SIOCBRADDBR or SIOCIFCREATE2), we make > a separate call to virNetDevSetMAC() after creating the bridge. > > This makes the dummy tap pointless for purposes of setting the MAC > address, but it is still useful for IPv6 DAD initialization (which > apparently requires at least one interface to be attached to the > bridge and online), so it hasn't been removed. (I'm considered making > another patch to add the dummy tap device only if the network needs > IPv6 DAD, but haven't decided yet if it's worthwhile). > > (NB: we can safely *always* call virNetDevBridgeCreate() with > &def->mac from the network driver because, in spite of the existence > of a "mac_specified" bool in the config suggesting that it may not > always be present, in reality a mac address will always be added to > any network that doesn't have one - this is guaranteed in all cases by > commit a47ae7c004) > > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1760851 > Signed-off-by: Laine Stump <laine@xxxxxxxxxx> > --- > NB: I was unable to test the calling of virNetDevSetMAC() from the > SIOCIFCREATE2 (BSD) version of virNetDevBridgeCreate(); even though I > managed to get a FreeBSD system setup and libvirt built there, when I > tried to start the default network the SIOCIFCREATE2 ioctl itself > failed, so it never even got to the virNetDevSetMAC(). I would > sincerely appreciate if someone more up to speed with libvirt on > FreeBSD/OpenBSD could check that out and let me know if it works > properly. > > src/network/bridge_driver.c | 2 +- > src/util/virnetdevbridge.c | 43 ++++++++++++++++++++++++++++++------- > src/util/virnetdevbridge.h | 2 +- > 3 files changed, 37 insertions(+), 10 deletions(-) > > diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c > index 6862818698..4f01bf6664 100644 > --- a/src/network/bridge_driver.c > +++ b/src/network/bridge_driver.c > @@ -2498,7 +2498,7 @@ networkStartNetworkVirtual(virNetworkDriverStatePtr driver, > def->name); > return -1; > } > - if (virNetDevBridgeCreate(def->bridge) < 0) > + if (virNetDevBridgeCreate(def->bridge, &def->mac) < 0) > return -1; > > if (def->mac_specified) { > diff --git a/src/util/virnetdevbridge.c b/src/util/virnetdevbridge.c > index edf4cc6236..d3a1e3c13e 100644 > --- a/src/util/virnetdevbridge.c > +++ b/src/util/virnetdevbridge.c > @@ -379,7 +379,7 @@ virNetDevBridgePortSetUnicastFlood(const char *brname G_GNUC_UNUSED, > */ > #if defined(HAVE_STRUCT_IFREQ) && defined(SIOCBRADDBR) > static int > -virNetDevBridgeCreateWithIoctl(const char *brname) > +virNetDevBridgeCreateWithIoctl(const char *brname, const virMacAddr *mac) Could you move the new parameter on its own line? > { > VIR_AUTOCLOSE fd = -1; > > @@ -392,22 +392,35 @@ virNetDevBridgeCreateWithIoctl(const char *brname) > return -1; > } > > + if (virNetDevSetMAC(brname, mac) < 0) { > + virErrorPtr savederr; > + > + virErrorPreserveLast(&savederr); > + ignore_value(ioctl(fd, SIOCBRDELBR, brname)); > + virErrorRestore(&savederr); > + return -1; > + } > + > return 0; > } > #endif > > #if defined(__linux__) && defined(HAVE_LIBNL) > int > -virNetDevBridgeCreate(const char *brname) > +virNetDevBridgeCreate(const char *brname, const virMacAddr *mac) And the same hare. > { > /* use a netlink RTM_NEWLINK message to create the bridge */ > int error = 0; > + virNetlinkNewLinkData data = { > + .mac = mac, > + }; > + > > - if (virNetlinkNewLink(brname, "bridge", NULL, &error) < 0) { > + if (virNetlinkNewLink(brname, "bridge", &data, &error) < 0) { > # if defined(HAVE_STRUCT_IFREQ) && defined(SIOCBRADDBR) > if (error == -EOPNOTSUPP) { > /* fallback to ioctl if netlink doesn't support creating bridges */ > - return virNetDevBridgeCreateWithIoctl(brname); > + return virNetDevBridgeCreateWithIoctl(brname, mac); > } > # endif > if (error < 0) > @@ -423,15 +436,17 @@ virNetDevBridgeCreate(const char *brname) > > #elif defined(HAVE_STRUCT_IFREQ) && defined(SIOCBRADDBR) > int > -virNetDevBridgeCreate(const char *brname) > +virNetDevBridgeCreate(const char *brname, > + const virMacAddr *mac) So that all prototypes look like this one. > { > - return virNetDevBridgeCreateWithIoctl(brname); > + return virNetDevBridgeCreateWithIoctl(brname, mac); > } > > > #elif defined(HAVE_STRUCT_IFREQ) && defined(SIOCIFCREATE2) > int > -virNetDevBridgeCreate(const char *brname) > +virNetDevBridgeCreate(const char *brname, > + const virMacAddr *mac) > { > struct ifreq ifr; > VIR_AUTOCLOSE s = -1; > @@ -448,10 +463,21 @@ virNetDevBridgeCreate(const char *brname) > if (virNetDevSetName(ifr.ifr_name, brname) == -1) > return -1; > > + if (virNetDevSetMAC(brname, mac) < 0) { > + virErrorPtr savederr; > + > + virErrorPreserveLast(&savederr); > + ignore_value(virNetDevBridgeDelete(brname)); > + virErrorRestore(&savederr); > + return -1; > + } > + > return 0; > } > #else > -int virNetDevBridgeCreate(const char *brname) > +int > +virNetDevBridgeCreate(const char *brname, > + const virMacAddr *mac ATTRIBUTE_UNUSED) > { > virReportSystemError(ENOSYS, > _("Unable to create bridge %s"), brname); > @@ -528,6 +554,7 @@ virNetDevBridgeDelete(const char *brname) > _("Unable to remove bridge %s"), > brname); > return -1; > + > } > > return 0; Drop this hunk, please. > diff --git a/src/util/virnetdevbridge.h b/src/util/virnetdevbridge.h > index 88284d6bed..c47597dec9 100644 > --- a/src/util/virnetdevbridge.h > +++ b/src/util/virnetdevbridge.h > @@ -21,7 +21,7 @@ > #include "internal.h" > #include "virmacaddr.h" > > -int virNetDevBridgeCreate(const char *brname) > +int virNetDevBridgeCreate(const char *brname, const virMacAddr *mac) > ATTRIBUTE_NONNULL(1) G_GNUC_WARN_UNUSED_RESULT; > int virNetDevBridgeDelete(const char *brname) > ATTRIBUTE_NONNULL(1) G_GNUC_WARN_UNUSED_RESULT; Reviewed-by: Jiri Denemark <jdenemar@xxxxxxxxxx> -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
