8:17 p.m.
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(a)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)
>>