<div class="gmail_quote">On Thu, Mar 1, 2012 at 12:48 PM, Laine Stump <<a href="mailto:laine@laine.org">laine@laine.org</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> With an additional new bool added to determine whether or not to discourage the use of the supplied MAC address by the bridge itself, virNetDevTapCreateInBridgePort had three booleans (well, 2 bools and an int used as a bool) in the arg list, which made it increasingly difficult to follow what was going on. This patch combines those three into a single flags arg, which not only shortens the arg list, but makes it more self-documenting. --- Does this make more sense as a PATCH 2/1 to be associated with the first patch in this thread: <a href="http://www.redhat.com/archives/libvir-list/2012-February/msg00760.html" target="_blank">http://www.redhat.com/archives/libvir-list/2012-February/msg00760.html</a> or should I squash them both together? (I'm leaning towards two separate patches, but could be convinced either way) src/network/bridge_driver.c | 3 +- src/qemu/qemu_command.c | 13 ++++++----- src/uml/uml_conf.c | 8 +++--- src/util/virnetdevtap.c | 46 +++++++++++++++++++++++++----------------- src/util/virnetdevtap.h | 20 +++++++++++++----- 5 files changed, 54 insertions(+), 36 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 3e1e031..cf75d26 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -1766,7 +1766,8 @@ networkStartNetworkVirtual(struct network_driver *driver, } if (virNetDevTapCreateInBridgePort(network->def->bridge, &macTapIfName, network->def->mac, - false, 0, false, NULL, NULL) < 0) { + NULL, NULL, + VIR_NETDEV_TAP_CREATE_USE_MAC_FOR_BRIDGE) < 0) { VIR_FREE(macTapIfName); goto err0; } diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 7e121c7..acfd38c 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -178,7 +178,7 @@ qemuNetworkIfaceConnect(virDomainDefPtr def, char *brname = NULL; int err; int tapfd = -1; - int vnet_hdr = 0; + unsigned int tap_create_flags = VIR_NETDEV_TAP_CREATE_IFUP; bool template_ifname = false; int actualType = virDomainNetGetActualType(net); @@ -240,12 +240,13 @@ qemuNetworkIfaceConnect(virDomainDefPtr def, } if (qemuCapsGet(qemuCaps, QEMU_CAPS_VNET_HDR) && - net->model && STREQ(net->model, "virtio")) - vnet_hdr = 1; + net->model && STREQ(net->model, "virtio")) { + tap_create_flags |= VIR_NETDEV_TAP_CREATE_VNET_HDR; + } - err = virNetDevTapCreateInBridgePort(brname, &net->ifname, net->mac, true, - vnet_hdr, true, &tapfd, - virDomainNetGetActualVirtPortProfile(net)); + err = virNetDevTapCreateInBridgePort(brname, &net->ifname, net->mac, &tapfd, + virDomainNetGetActualVirtPortProfile(net), + tap_create_flags); virDomainAuditNetDevice(def, net, "/dev/net/tun", tapfd >= 0); if (err < 0) { if (template_ifname) diff --git a/src/uml/uml_conf.c b/src/uml/uml_conf.c index c7b29a0..89fdd9f 100644 --- a/src/uml/uml_conf.c +++ b/src/uml/uml_conf.c @@ -1,7 +1,7 @@ /* * uml_conf.c: UML driver configuration * - * Copyright (C) <a href="tel:2006-2011" value="+37120062011">2006-2011</a> Red Hat, Inc. + * Copyright (C) <a href="tel:2006-2012" value="+37120062012">2006-2012</a> Red Hat, Inc. * Copyright (C) 2006 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -138,9 +138,9 @@ umlConnectTapDevice(virConnectPtr conn, template_ifname = true; } - if (virNetDevTapCreateInBridgePort(bridge, &net->ifname, net->mac, true, - 0, true, NULL, - virDomainNetGetActualVirtPortProfile(net)) < 0) { + if (virNetDevTapCreateInBridgePort(bridge, &net->ifname, net->mac, NULL, + virDomainNetGetActualVirtPortProfile(net), + VIR_NETDEV_TAP_CREATE_IFUP) < 0) { if (template_ifname) VIR_FREE(net->ifname); goto error; diff --git a/src/util/virnetdevtap.c b/src/util/virnetdevtap.c index 868ba57..fb0a8d2 100644 --- a/src/util/virnetdevtap.c +++ b/src/util/virnetdevtap.c @@ -107,22 +107,25 @@ virNetDevProbeVnetHdr(int tapfd) #ifdef TUNSETIFF /** - * brCreateTap: + * virNetDevTapCreate: * @ifname: the interface name - * @vnet_hr: whether to try enabling IFF_VNET_HDR * @tapfd: file descriptor return value for the new tap device + * @flags: OR of virNetDevTapCreateFlags. Only one flag is recognized: + * + * VIR_NETDEV_TAP_CREATE_VNET_HDR + * - Enable IFF_VNET_HDR on the tap device * * Creates a tap interface. * If the @tapfd parameter is supplied, the open tap device file * descriptor will be returned, otherwise the TAP device will be made - * persistent and closed. The caller must use brDeleteTap to remove - * a persistent TAP devices when it is no longer needed. + * persistent and closed. The caller must use virNetDevTapDelete to + * remove a persistent TAP devices when it is no longer needed. * * Returns 0 in case of success or an errno code in case of failure. */ int virNetDevTapCreate(char **ifname, - int vnet_hdr ATTRIBUTE_UNUSED, - int *tapfd) + int *tapfd, + unsigned int flags ATTRIBUTE_UNUSED) { int fd; struct ifreq ifr; @@ -139,7 +142,8 @@ int virNetDevTapCreate(char **ifname, ifr.ifr_flags = IFF_TAP|IFF_NO_PI; # ifdef IFF_VNET_HDR - if (vnet_hdr && virNetDevProbeVnetHdr(fd)) + if ((flags & VIR_NETDEV_TAP_CREATE_VNET_HDR) && + virNetDevProbeVnetHdr(fd)) ifr.ifr_flags |= IFF_VNET_HDR; # endif @@ -228,8 +232,8 @@ cleanup: } #else /* ! TUNSETIFF */ int virNetDevTapCreate(char **ifname ATTRIBUTE_UNUSED, - int vnet_hdr ATTRIBUTE_UNUSED, - int *tapfd ATTRIBUTE_UNUSED) + int *tapfd ATTRIBUTE_UNUSED, + unsigned int flags ATTRIBUTE_UNUSED) { virReportSystemError(ENOSYS, "%s", _("Unable to create TAP devices on this platform")); @@ -249,17 +253,23 @@ int virNetDevTapDelete(const char *ifname ATTRIBUTE_UNUSED) * @brname: the bridge name * @ifname: the interface name (or name template) * @macaddr: desired MAC address (VIR_MAC_BUFLEN long) - * @discourage: whether bridge should be discouraged from using macaddr - * @vnet_hdr: whether to try enabling IFF_VNET_HDR * @tapfd: file descriptor return value for the new tap device * @virtPortProfile: bridge/port specific configuration + * @flags: OR of virNetDevTapCreateFlags: + + * VIR_NETDEV_TAP_CREATE_IFUP + * - Bring the interface up + * VIR_NETDEV_TAP_CREATE_VNET_HDR + * - Enable IFF_VNET_HDR on the tap device + * VIR_NETDEV_TAP_CREATE_USE_MAC_FOR_BRIDGE + * - Set this interface's MAC as the bridge's MAC address * * This function creates a new tap device on a bridge. @ifname can be either * a fixed name or a name template with '%d' for dynamic name allocation. * in either case the final name for the bridge will be stored in @ifname. * If the @tapfd parameter is supplied, the open tap device file * descriptor will be returned, otherwise the TAP device will be made - * persistent and closed. The caller must use brDeleteTap to remove + * persistent and closed. The caller must use virNetDevTapDelete to remove * a persistent TAP devices when it is no longer needed. * * Returns 0 in case of success or -1 on failure @@ -267,15 +277,13 @@ int virNetDevTapDelete(const char *ifname ATTRIBUTE_UNUSED) int virNetDevTapCreateInBridgePort(const char *brname, char **ifname, const unsigned char *macaddr, - bool discourage, - int vnet_hdr, - bool up, int *tapfd, - virNetDevVPortProfilePtr virtPortProfile) + virNetDevVPortProfilePtr virtPortProfile, + unsigned int flags) { unsigned char tapmac[VIR_MAC_BUFLEN]; - if (virNetDevTapCreate(ifname, vnet_hdr, tapfd) < 0) + if (virNetDevTapCreate(ifname, tapfd, flags) < 0) return -1; /* We need to set the interface MAC before adding it @@ -285,7 +293,7 @@ int virNetDevTapCreateInBridgePort(const char *brname, * device before we set our static MAC. */ memcpy(tapmac, macaddr, VIR_MAC_BUFLEN); - if (discourage) + if (!(flags & VIR_NETDEV_TAP_CREATE_USE_MAC_FOR_BRIDGE)) tapmac[0] = 0xFE; /* Discourage bridge from using TAP dev MAC */ if (virNetDevSetMAC(*ifname, tapmac) < 0) @@ -308,7 +316,7 @@ int virNetDevTapCreateInBridgePort(const char *brname, goto error; } - if (virNetDevSetOnline(*ifname, up) < 0) + if (virNetDevSetOnline(*ifname, !!(flags & VIR_NETDEV_TAP_CREATE_IFUP)) < 0) goto error; return 0; diff --git a/src/util/virnetdevtap.h b/src/util/virnetdevtap.h index fc50e22..971b166 100644 --- a/src/util/virnetdevtap.h +++ b/src/util/virnetdevtap.h @@ -27,21 +27,29 @@ # include "virnetdevvportprofile.h" int virNetDevTapCreate(char **ifname, - int vnet_hdr, - int *tapfd) + int *tapfd, + unsigned int flags) ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; int virNetDevTapDelete(const char *ifname) ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; +typedef enum { + VIR_NETDEV_TAP_CREATE_NONE = 0, + /* Bring the interface up */ + VIR_NETDEV_TAP_CREATE_IFUP = 1 << 0, + /* Enable IFF_VNET_HDR on the tap device */ + VIR_NETDEV_TAP_CREATE_VNET_HDR = 1 << 1, + /* Set this interface's MAC as the bridge's MAC address */ + VIR_NETDEV_TAP_CREATE_USE_MAC_FOR_BRIDGE = 1 << 2, +} virNetDevTapCreateFlags; + int virNetDevTapCreateInBridgePort(const char *brname, char **ifname, const unsigned char *macaddr, - bool discourage, - int vnet_hdr, - bool up, int *tapfd, - virNetDevVPortProfilePtr virtPortProfile) + virNetDevVPortProfilePtr virtPortProfile, + unsigned int flags) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) ATTRIBUTE_RETURN_CHECK; -- 1.7.7.6 -- libvir-list mailing list <a href="mailto:libvir-list@redhat.com">libvir-list@redhat.com</a> <a href="https://www.redhat.com/mailman/listinfo/libvir-list" target="_blank">https://www.redhat.com/mailman/listinfo/libvir-list</a> </blockquote></div><div>Tested this patch together with 2/1 and it works fine for me. I will soon send out the 3/1 patch</div><div>that sets the vm-uuid.</div>