[PATCHv2 3/5] netdevmacvlan: Use helper function to create unique macvlan/macvtap name
Laine Stump
laine at redhat.com
Mon Dec 14 02:17:26 UTC 2020
On 12/13/20 8:20 PM, Shi Lei wrote:
> On 2020-12-14 at 08:58, Laine Stump wrote:
>> On 12/9/20 10:00 PM, Shi Lei wrote:
>>> Simplify ReserveName/GenerateName for macvlan and macvtap by using
>>> common functions.
>>>
>>> Signed-off-by: Shi Lei <shi_lei at massclouds.com>
>>> ---
>>> src/libvirt_private.syms | 1 -
>>> src/lxc/lxc_process.c | 2 +-
>>> src/qemu/qemu_process.c | 2 +-
>>> src/util/virnetdevmacvlan.c | 177 +++++-------------------------------
>>> src/util/virnetdevmacvlan.h | 14 +--
>>> 5 files changed, 26 insertions(+), 170 deletions(-)
>> You probably don't have the libxl driver enabled in your builds, so you
>> missed this change (which I've squashed in):
>>
>> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
>> index 6af274cb1b..3488d7ec08 100644
>> --- a/src/libxl/libxl_driver.c
>> +++ b/src/libxl/libxl_driver.c
>> @@ -364,7 +364,7 @@ libxlReconnectNotifyNets(virDomainDefPtr def)
>> * impolite.
>> */
>> if (virDomainNetGetActualType(net) == VIR_DOMAIN_NET_TYPE_DIRECT)
>> - virNetDevMacVLanReserveName(net->ifname);
>> + virNetDevReserveName(net->ifname);
>>
> Sorry for that. I fix it and send V3.
No reason to send a v3 for something that simple! I could just fix it up
here.
However, if you are going to send a V3, I just sent a comment about an
addition that is needed in Patch 1 that is a bit more than just an
omitted function name change - you'll want to put that (or something
similar) in if you repost.
Also, it occurred to me just now while looking at that - it would be
nice if the name change from VIR_NET_GENERATED_TAP_PREFIX to
VIR_NET_GENERATED_VNET_PREFIX was a patch by itself (prerequisite to all
the other patches). That one isn't really necessary, though.
So if you're okay with me making the change I just posted in my reply
for Patch 1, I can just push it all right now - no need for a V3 (I
think! I'm still looking at patch 4 and 5 one last time - I should be
done looking at them in 30 minutes or so).
>
> Shi Lei
>
>>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>>> index 64ef01e1..4d6ae84b 100644
>>> --- a/src/libvirt_private.syms
>>> +++ b/src/libvirt_private.syms
>>> @@ -2655,7 +2655,6 @@ virNetDevMacVLanDelete;
>>> virNetDevMacVLanDeleteWithVPortProfile;
>>> virNetDevMacVLanIsMacvtap;
>>> virNetDevMacVLanModeTypeFromString;
>>> -virNetDevMacVLanReserveName;
>>> virNetDevMacVLanRestartWithVPortProfile;
>>> virNetDevMacVLanTapOpen;
>>> virNetDevMacVLanTapSetup;
>>> diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c
>>> index 0f818e2e..eb29431e 100644
>>> --- a/src/lxc/lxc_process.c
>>> +++ b/src/lxc/lxc_process.c
>>> @@ -1641,7 +1641,7 @@ virLXCProcessReconnectNotifyNets(virDomainDefPtr def)
>>> * impolite.
>>> */
>>> if (virDomainNetGetActualType(net) == VIR_DOMAIN_NET_TYPE_DIRECT)
>>> - virNetDevMacVLanReserveName(net->ifname);
>>> + virNetDevReserveName(net->ifname);
>>>
>>> if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK) {
>>> if (!conn && !(conn = virGetConnectNetwork()))
>>> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
>>> index 1d54f201..6244ade4 100644
>>> --- a/src/qemu/qemu_process.c
>>> +++ b/src/qemu/qemu_process.c
>>> @@ -3390,7 +3390,7 @@ qemuProcessNotifyNets(virDomainDefPtr def)
>>> */
>>> switch (virDomainNetGetActualType(net)) {
>>> case VIR_DOMAIN_NET_TYPE_DIRECT:
>>> - virNetDevMacVLanReserveName(net->ifname);
>>> + virNetDevReserveName(net->ifname);
>>> break;
>>> case VIR_DOMAIN_NET_TYPE_BRIDGE:
>>> case VIR_DOMAIN_NET_TYPE_NETWORK:
>>> diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c
>>> index 72f0d670..36b13133 100644
>>> --- a/src/util/virnetdevmacvlan.c
>>> +++ b/src/util/virnetdevmacvlan.c
>>> @@ -45,7 +45,6 @@ VIR_ENUM_IMPL(virNetDevMacVLanMode,
>>>
>>> # include <net/if.h>
>>> # include <linux/if_tun.h>
>>> -# include <math.h>
>>>
>>> # include "viralloc.h"
>>> # include "virlog.h"
>>> @@ -59,129 +58,6 @@ VIR_ENUM_IMPL(virNetDevMacVLanMode,
>>>
>>> VIR_LOG_INIT("util.netdevmacvlan");
>>>
>>> -# define VIR_NET_GENERATED_MACVTAP_PATTERN VIR_NET_GENERATED_MACVTAP_PREFIX "%d"
>>> -# define VIR_NET_GENERATED_MACVLAN_PATTERN VIR_NET_GENERATED_MACVLAN_PREFIX "%d"
>>> -# define VIR_NET_GENERATED_PREFIX \
>>> - ((flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) ? \
>>> - VIR_NET_GENERATED_MACVTAP_PREFIX : VIR_NET_GENERATED_MACVLAN_PREFIX)
>>> -
>>> -
>>> -virMutex virNetDevMacVLanCreateMutex = VIR_MUTEX_INITIALIZER;
>>> -static int virNetDevMacVTapLastID = -1;
>>> -static int virNetDevMacVLanLastID = -1;
>>> -
>>> -
>>> -static void
>>> -virNetDevMacVLanReserveNameInternal(const char *name)
>>> -{
>>> - unsigned int id;
>>> - const char *idstr = NULL;
>>> - int *lastID = NULL;
>>> - int len;
>>> -
>>> - if (STRPREFIX(name, VIR_NET_GENERATED_MACVTAP_PREFIX)) {
>>> - lastID = &virNetDevMacVTapLastID;
>>> - len = strlen(VIR_NET_GENERATED_MACVTAP_PREFIX);
>>> - } else if (STRPREFIX(name, VIR_NET_GENERATED_MACVLAN_PREFIX)) {
>>> - lastID = &virNetDevMacVTapLastID;
>>> - len = strlen(VIR_NET_GENERATED_MACVLAN_PREFIX);
>>> - } else {
>>> - return;
>>> - }
>>> -
>>> - VIR_INFO("marking device in use: '%s'", name);
>>> -
>>> - idstr = name + len;
>>> -
>>> - if (virStrToLong_ui(idstr, NULL, 10, &id) >= 0) {
>>> - if (*lastID < (int)id)
>>> - *lastID = id;
>>> - }
>>> -}
>>> -
>>> -
>>> -/**
>>> - * virNetDevMacVLanReserveName:
>>> - * @name: name of an existing macvtap/macvlan device
>>> - *
>>> - * Set the value of virNetDevMacV(Lan|Tap)LastID to assure that any
>>> - * new device created with an autogenerated name will use a number
>>> - * higher than the number in the given device name.
>>> - *
>>> - * Returns nothing.
>>> - */
>>> -void
>>> -virNetDevMacVLanReserveName(const char *name)
>>> -{
>>> - virMutexLock(&virNetDevMacVLanCreateMutex);
>>> - virNetDevMacVLanReserveNameInternal(name);
>>> - virMutexUnlock(&virNetDevMacVLanCreateMutex);
>>> -}
>>> -
>>> -
>>> -/**
>>> - * virNetDevMacVLanGenerateName:
>>> - * @ifname: pointer to pointer to string containing template
>>> - * @lastID: counter to add to the template to form the name
>>> - *
>>> - * generate a new (currently unused) name for a new macvtap/macvlan
>>> - * device based on the template string in @ifname - replace %d with
>>> - * ++(*counter), and keep trying new values until one is found
>>> - * that doesn't already exist, or we've tried 10000 different
>>> - * names. Once a usable name is found, replace the template with the
>>> - * actual name.
>>> - *
>>> - * Returns 0 on success, -1 on failure.
>>> - */
>>> -static int
>>> -virNetDevMacVLanGenerateName(char **ifname, unsigned int flags)
>>> -{
>>> - const char *prefix;
>>> - const char *iftemplate;
>>> - int *lastID;
>>> - int id;
>>> - double maxIDd;
>>> - int maxID = INT_MAX;
>>> - int attempts = 0;
>>> -
>>> - if (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) {
>>> - prefix = VIR_NET_GENERATED_MACVTAP_PREFIX;
>>> - iftemplate = VIR_NET_GENERATED_MACVTAP_PREFIX "%d";
>>> - lastID = &virNetDevMacVTapLastID;
>>> - } else {
>>> - prefix = VIR_NET_GENERATED_MACVLAN_PREFIX;
>>> - iftemplate = VIR_NET_GENERATED_MACVLAN_PREFIX "%d";
>>> - lastID = &virNetDevMacVLanLastID;
>>> - }
>>> -
>>> - maxIDd = pow(10, IFNAMSIZ - 1 - strlen(prefix));
>>> - if (maxIDd <= (double)INT_MAX)
>>> - maxID = (int)maxIDd;
>>> -
>>> - do {
>>> - g_autofree char *try = NULL;
>>> -
>>> - id = ++(*lastID);
>>> -
>>> - /* reset before overflow */
>>> - if (*lastID == maxID)
>>> - *lastID = -1;
>>> -
>>> - try = g_strdup_printf(iftemplate, id);
>>> -
>>> - if (!virNetDevExists(try)) {
>>> - g_free(*ifname);
>>> - *ifname = g_steal_pointer(&try);
>>> - return 0;
>>> - }
>>> - } while (++attempts < 10000);
>>> -
>>> - virReportError(VIR_ERR_INTERNAL_ERROR,
>>> - _("no unused %s names available"),
>>> - *ifname);
>>> - return -1;
>>> -}
>>> -
>>>
>>> /**
>>> * virNetDevMacVLanIsMacvtap:
>>> @@ -209,13 +85,10 @@ virNetDevMacVLanIsMacvtap(const char *ifname)
>>> * virNetDevMacVLanCreate:
>>> *
>>> * @ifname: The name the interface is supposed to have; optional parameter
>>> - * @type: The type of device, i.e., "macvtap", "macvlan"
>>> * @macaddress: The MAC address of the device
>>> * @srcdev: The name of the 'link' device
>>> * @macvlan_mode: The macvlan mode to use
>>> - * @retry: Pointer to integer that will be '1' upon return if an interface
>>> - * with the same name already exists and it is worth to try
>>> - * again with a different name
>>> + * @flags: OR of virNetDevMacVLanCreateFlags.
>>> *
>>> * Create a macvtap device with the given properties.
>>> *
>>> @@ -223,19 +96,21 @@ virNetDevMacVLanIsMacvtap(const char *ifname)
>>> */
>>> int
>>> virNetDevMacVLanCreate(const char *ifname,
>>> - const char *type,
>>> const virMacAddr *macaddress,
>>> const char *srcdev,
>>> - uint32_t macvlan_mode)
>>> + uint32_t macvlan_mode,
>>> + unsigned int flags)
>>> {
>>> int error = 0;
>>> int ifindex = 0;
>>> + const char *type = (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) ?
>>> + VIR_NET_GENERATED_MACVTAP_PREFIX :
>>> + VIR_NET_GENERATED_MACVLAN_PREFIX;
>>> virNetlinkNewLinkData data = {
>>> .macvlan_mode = &macvlan_mode,
>>> .mac = macaddress,
>>> };
>>>
>>> -
>>> if (virNetDevGetIndex(srcdev, &ifindex) < 0)
>>> return -1;
>>>
>>> @@ -795,7 +670,6 @@ virNetDevMacVLanCreateWithVPortProfile(const char *ifnameRequested,
>>> size_t tapfdSize,
>>> unsigned int flags)
>>> {
>>> - const char *type = VIR_NET_GENERATED_PREFIX;
>>> g_autofree char *ifname = NULL;
>>> uint32_t macvtapMode;
>>> int vf = -1;
>>> @@ -832,8 +706,6 @@ virNetDevMacVLanCreateWithVPortProfile(const char *ifnameRequested,
>>> return -1;
>>> }
>>>
>>> - virMutexLock(&virNetDevMacVLanCreateMutex);
>>> -
>>> if (ifnameRequested) {
>>> int rc;
>>> bool isAutoName > @@ -842,10 +714,8 @@ virNetDevMacVLanCreateWithVPortProfile(const
>> char *ifnameRequested,
>>>
>>> VIR_INFO("Requested macvtap device name: %s", ifnameRequested);
>>>
>>> - if ((rc = virNetDevExists(ifnameRequested)) < 0) {
>>> - virMutexUnlock(&virNetDevMacVLanCreateMutex);
>>> + if ((rc = virNetDevExists(ifnameRequested)) < 0)
>>> return -1;
>>> - }
>>>
>>> if (rc) {
>>> /* ifnameRequested is already being used */
>>> @@ -854,17 +724,16 @@ virNetDevMacVLanCreateWithVPortProfile(const char *ifnameRequested,
>>> virReportSystemError(EEXIST,
>>> _("Unable to create device '%s'"),
>>> ifnameRequested);
>>> - virMutexUnlock(&virNetDevMacVLanCreateMutex);
>>> return -1;
>>> }
>>> } else {
>>>
>>> /* ifnameRequested is available. try to open it */
>>>
>>> - virNetDevMacVLanReserveNameInternal(ifnameRequested);
>>> + virNetDevReserveName(ifnameRequested);
>>>
>>> - if (virNetDevMacVLanCreate(ifnameRequested, type, macaddress,
>>> - linkdev, macvtapMode) == 0) {
>>> + if (virNetDevMacVLanCreate(ifnameRequested, macaddress,
>>> + linkdev, macvtapMode, flags) == 0) {
>>>
>>> /* virNetDevMacVLanCreate() was successful - use this name */
>>> ifname = g_strdup(ifnameRequested);
>>> @@ -874,7 +743,6 @@ virNetDevMacVLanCreateWithVPortProfile(const char *ifnameRequested,
>>> * autogenerated named, so there is nothing else to
>>> * try - fail and return.
>>> */
>>> - virMutexUnlock(&virNetDevMacVLanCreateMutex);
>>> return -1;
>>> }
>>> }
>>> @@ -885,16 +753,19 @@ virNetDevMacVLanCreateWithVPortProfile(const char *ifnameRequested,
>>> * autogenerated name, so now we look for an unused
>>> * autogenerated name.
>>> */
>>> - if (virNetDevMacVLanGenerateName(&ifname, flags) < 0 ||
>>> - virNetDevMacVLanCreate(ifname, type, macaddress,
>>> - linkdev, macvtapMode) < 0) {
>>> - virMutexUnlock(&virNetDevMacVLanCreateMutex);
>>> + virNetDevGenNameType type;
>>> + if (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP)
>>> + type = VIR_NET_DEV_GEN_NAME_MACVTAP;
>>> + else
>>> + type = VIR_NET_DEV_GEN_NAME_MACVLAN;
>>> +
>>> + if (virNetDevGenerateName(&ifname, type) < 0 ||
>>> + virNetDevMacVLanCreate(ifname, macaddress,
>>> + linkdev, macvtapMode, flags) < 0)
>>> return -1;
>>> - }
>>> }
>>>
>>> /* all done creating the device */
>>> - virMutexUnlock(&virNetDevMacVLanCreateMutex);
>>>
>>> if (virNetDevVPortProfileAssociate(ifname,
>>> virtPortProfile,
>>> @@ -1050,10 +921,10 @@ bool virNetDevMacVLanIsMacvtap(const char *ifname G_GNUC_UNUSED)
>>> }
>>>
>>> int virNetDevMacVLanCreate(const char *ifname G_GNUC_UNUSED,
>>> - const char *type G_GNUC_UNUSED,
>>> const virMacAddr *macaddress G_GNUC_UNUSED,
>>> const char *srcdev G_GNUC_UNUSED,
>>> - uint32_t macvlan_mode G_GNUC_UNUSED)
>>> + uint32_t macvlan_mode G_GNUC_UNUSED,
>>> + unsigned int fflags G_GNUC_UNUSED)
>>> {
>>> virReportSystemError(ENOSYS, "%s",
>>> _("Cannot create macvlan devices on this platform"));
>>> @@ -1141,10 +1012,4 @@ int virNetDevMacVLanVPortProfileRegisterCallback(const char *ifname G_GNUC_UNUSE
>>> _("Cannot create macvlan devices on this platform"));
>>> return -1;
>>> }
>>> -
>>> -void virNetDevMacVLanReserveName(const char *name G_GNUC_UNUSED)
>>> -{
>>> - virReportSystemError(ENOSYS, "%s",
>>> - _("Cannot create macvlan devices on this platform"));
>>> -}
>>> #endif /* ! WITH_LIBNL */
>>> diff --git a/src/util/virnetdevmacvlan.h b/src/util/virnetdevmacvlan.h
>>> index 48800a8f..0a013fc2 100644
>>> --- a/src/util/virnetdevmacvlan.h
>>> +++ b/src/util/virnetdevmacvlan.h
>>> @@ -48,23 +48,15 @@ typedef enum {
>>> VIR_NETDEV_MACVLAN_VNET_HDR = 1 << 2,
>>> } virNetDevMacVLanCreateFlags;
>>>
>>> -/* libvirt will start macvtap/macvlan interface names with one of
>>> - * these prefixes when it auto-generates the name
>>> - */
>>> -#define VIR_NET_GENERATED_MACVTAP_PREFIX "macvtap"
>>> -#define VIR_NET_GENERATED_MACVLAN_PREFIX "macvlan"
>>> -
>>> -void virNetDevMacVLanReserveName(const char *name);
>>> -
>>> bool virNetDevMacVLanIsMacvtap(const char *ifname)
>>> ATTRIBUTE_NONNULL(1) G_GNUC_WARN_UNUSED_RESULT G_GNUC_NO_INLINE;
>>>
>>> int virNetDevMacVLanCreate(const char *ifname,
>>> - const char *type,
>>> const virMacAddr *macaddress,
>>> const char *srcdev,
>>> - uint32_t macvlan_mode)
>>> - ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4)
>>> + uint32_t macvlan_mode,
>>> + unsigned int flags)
>>> + ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
>>> G_GNUC_WARN_UNUSED_RESULT;
>>>
>>> int virNetDevMacVLanDelete(const char *ifname)
>>>
More information about the libvir-list
mailing list