Re: [libvirt] [PATCH v2 1/2] util: Introduce flags field for macvtap creation

On Wed, Aug 27, 2014 at 10:34:13AM -0400, Matthew Rosato wrote: > Currently, there is one flag passed in during macvtap creation > (withTap) -- Let's convert this field to an unsigned int flag > field for future expansion. > > Signed-off-by: Matthew Rosato <mjrosato(a)linux.vnet.ibm.com&gt; > --- > src/lxc/lxc_process.c | 4 ++-- > src/qemu/qemu_command.c | 6 ++++-- > src/util/virnetdevmacvlan.c | 28 +++++++++++++++++----------- > src/util/virnetdevmacvlan.h | 14 ++++++++++---- > 4 files changed, 33 insertions(+), 19 deletions(-) > > diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c > index 3353dc1..ed30c37 100644 > --- a/src/lxc/lxc_process.c > +++ b/src/lxc/lxc_process.c > @@ -331,12 +331,12 @@ char > *virLXCProcessSetupInterfaceDirect(virConnectPtr conn, > net->ifname, &net->mac, > virDomainNetGetActualDirectDev(net), > virDomainNetGetActualDirectMode(net), > - false, false, def->uuid, > + false, def->uuid, > virDomainNetGetActualVirtPortProfile(net), > &res_ifname, > VIR_NETDEV_VPORT_PROFILE_OP_CREATE, > cfg->stateDir, > - virDomainNetGetActualBandwidth(net)) < 0) > + virDomainNetGetActualBandwidth(net), 0) < 0) > goto cleanup; > > ret = res_ifname; > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c > index 9241f57..1f71fa3 100644 > --- a/src/qemu/qemu_command.c > +++ b/src/qemu/qemu_command.c > @@ -177,6 +177,7 @@ qemuPhysIfaceConnect(virDomainDefPtr def, > char *res_ifname = NULL; > int vnet_hdr = 0; > virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); > + unsigned int macvlan_create_flags = > VIR_NETDEV_MACVLAN_CREATE_WITH_TAP; > > if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_VNET_HDR) && > net->model && STREQ(net->model, "virtio")) > @@ -186,11 +187,12 @@ qemuPhysIfaceConnect(virDomainDefPtr def, > net->ifname, &net->mac, > virDomainNetGetActualDirectDev(net), > virDomainNetGetActualDirectMode(net), > - true, vnet_hdr, def->uuid, > + vnet_hdr, def->uuid, > virDomainNetGetActualVirtPortProfile(net), > &res_ifname, > vmop, cfg->stateDir, > - virDomainNetGetActualBandwidth(net)); > + virDomainNetGetActualBandwidth(net), > + macvlan_create_flags); > if (rc >= 0) { > virDomainAuditNetDevice(def, net, res_ifname, true); > VIR_FREE(net->ifname); > diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c > index 054194b..9ddeca4 100644 > --- a/src/util/virnetdevmacvlan.c > +++ b/src/util/virnetdevmacvlan.c > @@ -790,6 +790,7 @@ virNetDevMacVLanVPortProfileRegisterCallback(const > char *ifname, > * @macaddress: The MAC address for the macvtap device > * @linkdev: The interface name of the NIC to connect to the external > bridge > * @mode: int describing the mode for 'bridge', 'vepa', 'private' or > 'passthru'. > + * @flags: OR of virNetDevMacVLanCreateFlags. I took the liberty of moving this as a last parameter as well. > * @vnet_hdr: 1 to enable IFF_VNET_HDR, 0 to disable it > * @vmuuid: The UUID of the VM the macvtap belongs to > * @virtPortProfile: pointer to object holding the virtual port > profile data > @@ -797,25 +798,29 @@ > virNetDevMacVLanVPortProfileRegisterCallback(const char *ifname, > * interface will be stored into if everything succeeded. It is up > * to the caller to free the string. > * > - * Returns file descriptor of the tap device in case of success with > @withTap, > - * otherwise returns 0; returns -1 on error. > + * Returns file descriptor of the tap device in case of success with > + * @flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP, otherwise returns 0; > returns > + * -1 on error. > */ > int virNetDevMacVLanCreateWithVPortProfile(const char *tgifname, > const virMacAddr *macaddress, > const char *linkdev, > virNetDevMacVLanMode mode, > - bool withTap, > int vnet_hdr, > const unsigned char *vmuuid, > virNetDevVPortProfilePtr > virtPortProfile, > char **res_ifname, > virNetDevVPortProfileOp vmOp, > char *stateDir, > - virNetDevBandwidthPtr > bandwidth) > + virNetDevBandwidthPtr > bandwidth, > + unsigned int flags) > { > - const char *type = withTap ? "macvtap" : "macvlan"; > - const char *prefix = withTap ? MACVTAP_NAME_PREFIX : > MACVLAN_NAME_PREFIX; > - const char *pattern = withTap ? MACVTAP_NAME_PATTERN : > MACVLAN_NAME_PATTERN; > + const char *type = (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) ? > + "macvtap" : "macvlan"; > + const char *prefix = (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) ? > + MACVTAP_NAME_PREFIX : MACVLAN_NAME_PREFIX; > + const char *pattern = (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) ? > + MACVTAP_NAME_PATTERN : MACVLAN_NAME_PATTERN; > int c, rc; > char ifname[IFNAMSIZ]; > int retries, do_retry = 0; > @@ -903,7 +908,7 @@ int virNetDevMacVLanCreateWithVPortProfile(const > char *tgifname, > goto disassociate_exit; > } > > - if (withTap) { > + if (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) { > if ((rc = virNetDevMacVLanTapOpen(cr_ifname, 10)) < 0) > goto disassociate_exit; > > @@ -922,7 +927,7 @@ int virNetDevMacVLanCreateWithVPortProfile(const > char *tgifname, > } > > if (virNetDevBandwidthSet(cr_ifname, bandwidth, false) < 0) { > - if (withTap) > + if (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) > VIR_FORCE_CLOSE(rc); /* sets rc to -1 */ > else > rc = -1; > @@ -1066,15 +1071,16 @@ int > virNetDevMacVLanCreateWithVPortProfile(const char *ifname > ATTRIBUTE_UNUSED, > const virMacAddr > *macaddress ATTRIBUTE_UNUSED, > const char *linkdev > ATTRIBUTE_UNUSED, > virNetDevMacVLanMode mode > ATTRIBUTE_UNUSED, > - bool withTap > ATTRIBUTE_UNUSED, > int vnet_hdr ATTRIBUTE_UNUSED, > const unsigned char *vmuuid > ATTRIBUTE_UNUSED, > virNetDevVPortProfilePtr > virtPortProfile ATTRIBUTE_UNUSED, > char **res_ifname > ATTRIBUTE_UNUSED, > virNetDevVPortProfileOp > vmop ATTRIBUTE_UNUSED, > char *stateDir > ATTRIBUTE_UNUSED, > - virNetDevBandwidthPtr > bandwidth ATTRIBUTE_UNUSED) > + virNetDevBandwidthPtr > bandwidth ATTRIBUTE_UNUSED, > + unsigned int flags) > { > + virCheckFlags(0, NULL); The flag check could be fixed not to report in such cases, but it's easier just to check the flags for now. But it should be: virCheckFlags(0, -1); > virReportSystemError(ENOSYS, "%s", > _("Cannot create macvlan devices on this > platform")); > return -1; > diff --git a/src/util/virnetdevmacvlan.h b/src/util/virnetdevmacvlan.h > index 9b15a31..e92da4a 100644 > --- a/src/util/virnetdevmacvlan.h > +++ b/src/util/virnetdevmacvlan.h > @@ -40,6 +40,12 @@ typedef enum { > } virNetDevMacVLanMode; > VIR_ENUM_DECL(virNetDevMacVLanMode) > > +typedef enum { > + VIR_NETDEV_MACVLAN_CREATE_NONE = 0, > + /* Create with a tap device */ > + VIR_NETDEV_MACVLAN_CREATE_WITH_TAP = 1 << 0, You aligned only one value. Unless you object to it, I'll align them together without too many whitespaces. Other than that, it looks fine, ACK after 1.2.8, I'll push it with the changes mentioned after the release if you're OK with that.

Martin