Re: Re: [PATCHv2 1/5] netdev: Introduce several helper functions for generating unique netdev name

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 2020-12-14 at 10:10, Laine Stump wrote:
>On 12/9/20 10:00 PM, Shi Lei wrote:
>> Extract ReserveName/GenerateName from netdevtap and netdevmacvlan as
>> common helper functions.
>>
>> Signed-off-by: Shi Lei <shi_lei@xxxxxxxxxxxxxx>
>> ---
>>   src/bhyve/bhyve_command.c              |   4 +-
>>   src/conf/domain_conf.c                 |   4 +-
>>   src/interface/interface_backend_udev.c |   2 +-
>>   src/libvirt_private.syms               |   2 +
>>   src/qemu/qemu_interface.c              |   8 +-
>>   src/util/virnetdev.c                   | 116 +++++++++++++++++++++++++
>>   src/util/virnetdev.h                   |  27 +++++-
>>   src/util/virnetdevtap.c                |  10 +--
>>   8 files changed, 158 insertions(+), 15 deletions(-)
>>
>> diff --git a/src/bhyve/bhyve_command.c b/src/bhyve/bhyve_command.c
>> index acf3a5a4..4cf98c0e 100644
>> --- a/src/bhyve/bhyve_command.c
>> +++ b/src/bhyve/bhyve_command.c
>> @@ -80,10 +80,10 @@ bhyveBuildNetArgStr(const virDomainDef *def,
>>       }
>>  
>>       if (!net->ifname ||
>> -        STRPREFIX(net->ifname, VIR_NET_GENERATED_TAP_PREFIX) ||
>> +        STRPREFIX(net->ifname, VIR_NET_GENERATED_VNET_PREFIX) ||
>>           strchr(net->ifname, '%')) {
>>           VIR_FREE(net->ifname);
>> -        net->ifname = g_strdup(VIR_NET_GENERATED_TAP_PREFIX "%d");
>> +        net->ifname = g_strdup(VIR_NET_GENERATED_VNET_PREFIX "%d");
>>       }
>>  
>>       if (!dryRun) {
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 23415b32..403ecab8 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -12037,7 +12037,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
>>  
>>       if (def->managed_tap != VIR_TRISTATE_BOOL_NO && ifname &&
>>           (flags & VIR_DOMAIN_DEF_PARSE_INACTIVE) &&
>> -        (STRPREFIX(ifname, VIR_NET_GENERATED_TAP_PREFIX) ||
>> +        (STRPREFIX(ifname, VIR_NET_GENERATED_VNET_PREFIX) ||
>>            STRPREFIX(ifname, VIR_NET_GENERATED_MACVTAP_PREFIX) ||
>>            STRPREFIX(ifname, VIR_NET_GENERATED_MACVLAN_PREFIX) ||
>>            (prefix && STRPREFIX(ifname, prefix)))) {
>> @@ -26460,7 +26460,7 @@ virDomainNetDefFormat(virBufferPtr buf,
>>       if (def->ifname &&
>>           (def->managed_tap == VIR_TRISTATE_BOOL_NO ||
>>            !((flags & VIR_DOMAIN_DEF_FORMAT_INACTIVE) &&
>> -           (STRPREFIX(def->ifname, VIR_NET_GENERATED_TAP_PREFIX) ||
>> +           (STRPREFIX(def->ifname, VIR_NET_GENERATED_VNET_PREFIX) ||
>>               STRPREFIX(def->ifname, VIR_NET_GENERATED_MACVTAP_PREFIX) ||
>>               STRPREFIX(def->ifname, VIR_NET_GENERATED_MACVLAN_PREFIX) ||
>>               (prefix && STRPREFIX(def->ifname, prefix)))))) {
>> diff --git a/src/interface/interface_backend_udev.c b/src/interface/interface_backend_udev.c
>> index 173c4fc3..6a94a450 100644
>> --- a/src/interface/interface_backend_udev.c
>> +++ b/src/interface/interface_backend_udev.c
>> @@ -544,7 +544,7 @@ udevBridgeScanDirFilter(const struct dirent *entry)
>>        * vnet%d. Improvements to this check are welcome.
>>        */
>>       if (strlen(entry->d_name) >= 5) {
>> -        if (STRPREFIX(entry->d_name, VIR_NET_GENERATED_TAP_PREFIX) &&
>> +        if (STRPREFIX(entry->d_name, VIR_NET_GENERATED_VNET_PREFIX) &&
>>               g_ascii_isdigit(entry->d_name[4]))
>>               return 0;
>>       }
>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>> index 992488f7..c0f50856 100644
>> --- a/src/libvirt_private.syms
>> +++ b/src/libvirt_private.syms
>> @@ -2551,6 +2551,7 @@ virNetDevDelMulti;
>>   virNetDevExists;
>>   virNetDevFeatureTypeFromString;
>>   virNetDevFeatureTypeToString;
>> +virNetDevGenerateName;
>>   virNetDevGetFeatures;
>>   virNetDevGetIndex;
>>   virNetDevGetLinkInfo;
>> @@ -2574,6 +2575,7 @@ virNetDevIfStateTypeToString;
>>   virNetDevIsVirtualFunction;
>>   virNetDevPFGetVF;
>>   virNetDevReadNetConfig;
>> +virNetDevReserveName;
>>   virNetDevRunEthernetScript;
>>   virNetDevRxFilterFree;
>>   virNetDevRxFilterModeTypeFromString;
>> diff --git a/src/qemu/qemu_interface.c b/src/qemu/qemu_interface.c
>> index 32b397d2..197c0aa2 100644
>> --- a/src/qemu/qemu_interface.c
>> +++ b/src/qemu/qemu_interface.c
>> @@ -456,10 +456,10 @@ qemuInterfaceEthernetConnect(virDomainDefPtr def,
>>           }
>>       } else {
>>           if (!net->ifname ||
>> -            STRPREFIX(net->ifname, VIR_NET_GENERATED_TAP_PREFIX) ||
>> +            STRPREFIX(net->ifname, VIR_NET_GENERATED_VNET_PREFIX) ||
>>               strchr(net->ifname, '%')) {
>>               VIR_FREE(net->ifname);
>> -            net->ifname = g_strdup(VIR_NET_GENERATED_TAP_PREFIX "%d");
>> +            net->ifname = g_strdup(VIR_NET_GENERATED_VNET_PREFIX "%d");
>>               /* avoid exposing vnet%d in getXMLDesc or error outputs */
>>               template_ifname = true;
>>           }
>> @@ -560,10 +560,10 @@ qemuInterfaceBridgeConnect(virDomainDefPtr def,
>>       }
>>  
>>       if (!net->ifname ||
>> -        STRPREFIX(net->ifname, VIR_NET_GENERATED_TAP_PREFIX) ||
>> +        STRPREFIX(net->ifname, VIR_NET_GENERATED_VNET_PREFIX) ||
>>           strchr(net->ifname, '%')) {
>>           VIR_FREE(net->ifname);
>> -        net->ifname = g_strdup(VIR_NET_GENERATED_TAP_PREFIX "%d");
>> +        net->ifname = g_strdup(VIR_NET_GENERATED_VNET_PREFIX "%d");
>>           /* avoid exposing vnet%d in getXMLDesc or error outputs */
>>           template_ifname = true;
>>       }
>> diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
>> index 5104bbe7..bd1ca1d8 100644
>> --- a/src/util/virnetdev.c
>> +++ b/src/util/virnetdev.c
>> @@ -17,6 +17,7 @@
>>    */
>>  
>>   #include <config.h>
>> +#include <math.h>
>>  
>>   #include "virnetdev.h"
>>   #include "viralloc.h"
>> @@ -95,6 +96,14 @@ VIR_LOG_INIT("util.netdev");
>>       (FEATURE_WORD(blocks, index, field) & FEATURE_FIELD_FLAG(index))
>>   #endif
>>  
>> +
>> +static virNetDevGenName
>> +virNetDevGenNames[VIR_NET_DEV_GEN_NAME_LAST] = {
>> +    {-1, VIR_NET_GENERATED_VNET_PREFIX, VIR_MUTEX_INITIALIZER},
>> +    {-1, VIR_NET_GENERATED_MACVTAP_PREFIX, VIR_MUTEX_INITIALIZER},
>> +    {-1, VIR_NET_GENERATED_MACVLAN_PREFIX, VIR_MUTEX_INITIALIZER},
>> +};
>> +
>>   typedef enum {
>>       VIR_MCAST_TYPE_INDEX_TOKEN,
>>       VIR_MCAST_TYPE_NAME_TOKEN,
>> @@ -3516,3 +3525,110 @@ virNetDevSetRootQDisc(const char *ifname,
>>  
>>       return 0;
>>   }
>> +
>> +
>> +/**
>> + * virNetDevReserveName:
>> + * @name: name of an existing network device
>> + *
>> + * Reserve a network device name, so that any new network device
>> + * created with an autogenerated name will use a number higher
>> + * than the number in the given device name.
>> + *
>> + * Returns nothing.
>> + */
>> +void
>> +virNetDevReserveName(const char *name)
>> +{
>> +    unsigned int id;
>> +    const char *idstr = NULL;
>> +    virNetDevGenNameType type;
>> +
>> +    if (STRPREFIX(name, VIR_NET_GENERATED_VNET_PREFIX))
>> +        type = VIR_NET_DEV_GEN_NAME_VNET;
>> +    else if (STRPREFIX(name, VIR_NET_GENERATED_MACVTAP_PREFIX))
>> +        type = VIR_NET_DEV_GEN_NAME_MACVTAP;
>> +    else if (STRPREFIX(name, VIR_NET_GENERATED_MACVLAN_PREFIX))
>> +        type = VIR_NET_DEV_GEN_NAME_MACVLAN;
>> +    else
>> +        return;
>> +
>> +    VIR_INFO("marking device in use: '%s'", name);
>> +
>> +    idstr = name + strlen(virNetDevGenNames[type].prefix);
>> +
>> +    if (virStrToLong_ui(idstr, NULL, 10, &id) >= 0) {
>> +        virMutexLock(&virNetDevGenNames[type].mutex);
>> +
>> +        if (virNetDevGenNames[type].lastID < (int)id)
>> +            virNetDevGenNames[type].lastID = id;
>> +
>> +        virMutexUnlock(&virNetDevGenNames[type].mutex);
>> +    }
>> +}
>> +
>> +
>> +/**
>> + * virNetDevGenerateName:
>> + * @ifname: pointer to pointer to string which can be a template,
>> + *          NULL or user-provided name.
>> + * @type: type of the network device
>> + *
>> + * generate a new (currently unused) name for a new network device based
>> + * on @ifname. If string pointed by @ifname is a template, replace %d
>> + * with the reserved id; if that string is NULL, just generate a new
>> + * name. 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.
>> + *
>> + * Note: if string pointed by @ifname is NOT a template or NULL, leave
>> + * it unchanged and return it directly.
>> + *
>> + * Returns 0 on success, -1 on failure.
>> + */
>> +int
>> +virNetDevGenerateName(char **ifname, virNetDevGenNameType type)
>> +{
>> +    int id;
>> +    const char *prefix = virNetDevGenNames[type].prefix;
>> +    double maxIDd = pow(10, IFNAMSIZ - 1 - strlen(prefix));
>> +    int maxID = INT_MAX;
>> +    int attempts = 0;
>> +
>> +    /* The @ifname is not a template, leave it unchanged. */
>> +    if (*ifname && strstr(*ifname, "%d") == NULL)
>
>
>This would still attempt to generate a name for something that had
>multiple format specifiers in it, e.g. "vnet%d%n", which could lead to
>"Bad Things(tm)". I *think* it would be sufficient to avoid this if we
>just checked for multiple occurences of %, something like this:
>
>
>    if (*ifname &&
>
>        (strchr(*ifname, '%') != strrchr(*ifname, '%') ||
>
>         strstr(*ifname, "%d") == NULL)) {
>
>        return 0;
>
>    }
>
>
>(The idea here is that if strchr and strrchr are the same, that means
>there's only a single '%'. So if we get past this check, we know that
>either the string is empty, or that it contains a single %d and no other
>format specifiers).
>
>
>If you're okay with me squashing that change, I can just do that and
>push it, or if you'd rather do it some other way and re-post, that's
>fine too - just let me know. 

Okay. I agree with your change. Thanks!

>
>
>> +        return 0;
>> +
>> +    if (maxIDd <= (double)INT_MAX)
>> +        maxID = (int)maxIDd;
>> +
>> +    do {
>> +        g_autofree char *try = NULL;
>> +
>> +        virMutexLock(&virNetDevGenNames[type].mutex);
>> +
>> +        id = ++virNetDevGenNames[type].lastID;
>> +
>> +        /* reset before overflow */
>> +        if (virNetDevGenNames[type].lastID >= maxID)
>> +            virNetDevGenNames[type].lastID = -1;
>> +
>> +        virMutexUnlock(&virNetDevGenNames[type].mutex);
>> +
>> +        if (*ifname)
>> +            try = g_strdup_printf(*ifname, id);
>> +        else
>> +            try = g_strdup_printf("%s%d", prefix, 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"),
>> +                   prefix);
>> +    return -1;
>> +}
>> diff --git a/src/util/virnetdev.h b/src/util/virnetdev.h
>> index 53e606c6..f0160127 100644
>> --- a/src/util/virnetdev.h
>> +++ b/src/util/virnetdev.h
>> @@ -38,7 +38,13 @@ typedef void virIfreq;
>>   /* Used for prefix of ifname of any tap device name generated
>>    * dynamically by libvirt, cannot be used for a persistent network name.
>>    */
>> -#define VIR_NET_GENERATED_TAP_PREFIX "vnet"
>> +#define VIR_NET_GENERATED_VNET_PREFIX "vnet"
>> +
>> +/* 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"
>>  
>>   typedef enum {
>>      VIR_NETDEV_RX_FILTER_MODE_NONE = 0,
>> @@ -145,6 +151,21 @@ struct _virNetDevCoalesce {
>>       uint32_t rate_sample_interval;
>>   };
>>  
>> +typedef enum {
>> +    VIR_NET_DEV_GEN_NAME_VNET,
>> +    VIR_NET_DEV_GEN_NAME_MACVTAP,
>> +    VIR_NET_DEV_GEN_NAME_MACVLAN,
>> +    VIR_NET_DEV_GEN_NAME_LAST
>> +} virNetDevGenNameType;
>> +
>> +typedef struct _virNetDevGenName virNetDevGenName;
>> +typedef virNetDevGenName *virNetDevGenNamePtr;
>> +struct _virNetDevGenName {
>> +    int lastID;         /* not "unsigned" because callers use %d */
>> +    const char *prefix;
>> +    virMutex mutex;
>> +};
>> +
>>  
>>   int virNetDevSetupControl(const char *ifname,
>>                             virIfreq *ifr)
>> @@ -321,3 +342,7 @@ int virNetDevVFInterfaceStats(virPCIDeviceAddressPtr vfAddr,
>>   ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
>>  
>>   G_DEFINE_AUTOPTR_CLEANUP_FUNC(virNetDevRxFilter, virNetDevRxFilterFree);
>> +
>> +void virNetDevReserveName(const char *name);
>> +
>> +int virNetDevGenerateName(char **ifname, virNetDevGenNameType type);
>> diff --git a/src/util/virnetdevtap.c b/src/util/virnetdevtap.c
>> index 198607b5..9354cc10 100644
>> --- a/src/util/virnetdevtap.c
>> +++ b/src/util/virnetdevtap.c
>> @@ -76,11 +76,11 @@ virNetDevTapReserveName(const char *name)
>>       const char *idstr = NULL;
>>  
>>  
>> -    if (STRPREFIX(name, VIR_NET_GENERATED_TAP_PREFIX)) {
>> +    if (STRPREFIX(name, VIR_NET_GENERATED_VNET_PREFIX)) {
>>  
>>           VIR_INFO("marking device in use: '%s'", name);
>>  
>> -        idstr = name + strlen(VIR_NET_GENERATED_TAP_PREFIX);
>> +        idstr = name + strlen(VIR_NET_GENERATED_VNET_PREFIX);
>>  
>>           if (virStrToLong_ui(idstr, NULL, 10, &id) >= 0) {
>>               virMutexLock(&virNetDevTapCreateMutex);
>> @@ -200,7 +200,7 @@ static int
>>   virNetDevTapGenerateName(char **ifname)
>>   {
>>       int id;
>> -    double maxIDd = pow(10, IFNAMSIZ - 1 - strlen(VIR_NET_GENERATED_TAP_PREFIX));
>> +    double maxIDd = pow(10, IFNAMSIZ - 1 - strlen(VIR_NET_GENERATED_VNET_PREFIX));
>>       int maxID = INT_MAX;
>>       int attempts = 0;
>>  
>> @@ -227,7 +227,7 @@ virNetDevTapGenerateName(char **ifname)
>>  
>>       virReportError(VIR_ERR_INTERNAL_ERROR,
>>                      _("no unused %s names available"),
>> -                   VIR_NET_GENERATED_TAP_PREFIX);
>> +                   VIR_NET_GENERATED_VNET_PREFIX);
>>       return -1;
>>   }
>>  
>> @@ -270,7 +270,7 @@ int virNetDevTapCreate(char **ifname,
>>        * immediately re-using names that have just been released, which
>>        * can lead to race conditions).
>>        */
>> -    if (STREQ(*ifname, VIR_NET_GENERATED_TAP_PREFIX "%d") &&
>> +    if (STREQ(*ifname, VIR_NET_GENERATED_VNET_PREFIX "%d") &&
>>           virNetDevTapGenerateName(ifname) < 0) {
>>           goto cleanup;
>>       }
>
>




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux