Re: Re: [PATCH 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-08 at 07:37, Laine Stump wrote:
>First - thanks for taking this on, and doing it the right way! (i.e.
>refactoring my two changes down into a common set of functions to be
>reused, rather than duplicating the code). I can't really say why I
>didn't go the extra bit when I made the changes to virnetdevtap.c and
>virnetdevmacvlan.c - I really should have cleaned it up as you've done
>in patches 1 & 2 here.
>
>On 12/4/20 2:01 AM, Shi Lei wrote:
>> Extract ReserveName/GenerateName from netdevtap and netdevmacvlan as
>> common helper functions. Along with them, export Lock/Unlock functions
>> to protect these processes.
>>
>> Signed-off-by: Shi Lei <shi_lei@xxxxxxxxxxxxxx>
>> ---
>>   src/libvirt_private.syms |   4 ++
>>   src/util/virnetdev.c     | 140 +++++++++++++++++++++++++++++++++++++++
>>   src/util/virnetdev.h     |  33 +++++++++
>>   3 files changed, 177 insertions(+)
>>
>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>> index 2f640ef1..be3148c9 100644
>> --- a/src/libvirt_private.syms
>> +++ b/src/libvirt_private.syms
>> @@ -2551,6 +2551,7 @@ virNetDevDelMulti;
>>   virNetDevExists;
>>   virNetDevFeatureTypeFromString;
>>   virNetDevFeatureTypeToString;
>> +virNetDevGenerateName;
>>   virNetDevGetFeatures;
>>   virNetDevGetIndex;
>>   virNetDevGetLinkInfo;
>> @@ -2572,8 +2573,10 @@ virNetDevGetVLanID;
>>   virNetDevIfStateTypeFromString;
>>   virNetDevIfStateTypeToString;
>>   virNetDevIsVirtualFunction;
>> +virNetDevLockGenName;
>>   virNetDevPFGetVF;
>>   virNetDevReadNetConfig;
>> +virNetDevReserveName;
>>   virNetDevRunEthernetScript;
>>   virNetDevRxFilterFree;
>>   virNetDevRxFilterModeTypeFromString;
>> @@ -2594,6 +2597,7 @@ virNetDevSetRcvMulti;
>>   virNetDevSetRootQDisc;
>>   virNetDevSetupControl;
>>   virNetDevSysfsFile;
>> +virNetDevUnlockGenName;
>>   virNetDevValidateConfig;
>>   virNetDevVFInterfaceStats;
>>  
>> diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
>> index 5104bbe7..5ff8e35f 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,22 @@ VIR_LOG_INIT("util.netdev");
>>       (FEATURE_WORD(blocks, index, field) & FEATURE_FIELD_FLAG(index))
>>   #endif
>>  
>> +VIR_ENUM_IMPL(virNetDevGenNameType,
>> +              VIR_NET_DEV_GEN_NAME_LAST,
>> +              "none",
>> +              "tap",
>> +              "macvtap",
>> +              "macvlan",
>> +);
>
>
>You've added VIR_ENUM_IMPL and VIR_ENUM_DECL, but haven't actually used
>the corresponding virNetDevGenNameTypeToString() and
>virNetDevGenNameTypeFromString() functions anywhere. Unless it will be
>used, I'd say it would be just as well to leave it out. (and if you do
>define it, probably use "vnet" for the tap device entry, since it's not
>really important that the device is a tap device, just that it's a
>network device whose name starts with "vnet").
> 

Okay.

>
>
>> +static virNetDevGenName
>> +virNetDevGenNames[VIR_NET_DEV_GEN_NAME_LAST] = {
>> +    {0, NULL, VIR_MUTEX_INITIALIZER},
>
>
>So you have this extra entry in the table at [0] because the first enum
>value is VIR_NET_DEV_GEN_NAME_NONE. Since that value would never be
>possible, I think you should just make the first value in the enum be
>VIR_NET_DEV_GEN_NAME_VNET (instead of ..._TAP), and then you don't need
>the unused entry in the table.
> 

Okay.

>
>> +    {-1, VIR_NET_GENERATED_TAP_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 +3533,126 @@ virNetDevSetRootQDisc(const char *ifname,
>>  
>>       return 0;
>>   }
>> +
>> +
>> +/**
>> + * virNetDevReserveName:
>> + * @name: name of an existing network device
>> + * @type: type of the network device
>> + * @locked: whether this process is locked by the internal mutex
>> + *
>> + * 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,
>> +                     virNetDevGenNameType type,
>> +                     bool locked)
>> +{
>> +    unsigned int id;
>> +    const char *idstr = NULL;
>> +
>> +    if (type && STRPREFIX(name, virNetDevGenNames[type].prefix)) {
>
>
>If you're going to worry about range-checking type, make sure that it's
>< ..._LAST (0 will be a valid value if you follow my recommendation
>above to remove ..._NONE) 

Okay.

>
>
>> +
>> +        VIR_INFO("marking device in use: '%s'", name);
>> +
>> +        idstr = name + strlen(virNetDevGenNames[type].prefix);
>> +
>> +        if (virStrToLong_ui(idstr, NULL, 10, &id) >= 0) {
>> +            if (locked)
>> +                virMutexLock(&virNetDevGenNames[type].mutex);
>> +
>> +            if (virNetDevGenNames[type].lastID < (int)id)
>> +                virNetDevGenNames[type].lastID = id;
>> +
>> +            if (locked)
>> +                virMutexUnlock(&virNetDevGenNames[type].mutex);
>> +        }
>> +    }
>> +}
>> +
>> +
>> +/**
>> + * virNetDevGenerateName:
>> + * @ifname: pointer to pointer to string containing template
>> + *
>> + * generate a new (currently unused) name for a new network device based
>> + * on the templace string in @ifname - replace %d with the reserved id,
>> + * 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.
>> + */
>> +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;
>> +
>> +    if (maxIDd <= (double)INT_MAX)
>> +        maxID = (int)maxIDd;
>> +
>> +    do {
>> +        g_autofree char *try = NULL;
>> +
>> +        id = ++virNetDevGenNames[type].lastID;
>> +
>> +        /* reset before overflow */
>> +        if (virNetDevGenNames[type].lastID >= maxID)
>> +            virNetDevGenNames[type].lastID = -1;
>> +
>> +        if (*ifname)
>> +            try = g_strdup_printf(*ifname, id);
>
>
>An interesting twist! I like the direction of this - instead of relying
>on the kernel to generate the names in the case of someone defining
>their device with, e.g. <target dev='myprefix%d'/>, you send it through
>our generator. Sending a user-provided string to printf as the format
>string makes me uneasy though. Also, what if ifname is an
>unparameterized name (e.g. "mytap"), and there is already a device named
>"mytap"? In that case, it will spin through the loop 10000 times trying
>the same device name, although we know from the start that it's not
>going to work.
>
>
>How about if, instead of blindly doing this, we check to see if the
>device name has a single "%" that is followed by "d"? If it's anything
>other than that (or NULL) then we just return with the string unchanged. 

Okay. In the meanwhile, I should modify the explanation of this functions.
>From now on, the *ifname can be a template, NULL or a user-defined name.

>
>
>> +        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;
>> +}
>> +
>> +
>> +/**
>> + * virNetDevLockGenName:
>> + * @type: type of the network device
>> + *
>> + * Lock the internal mutex for creation for this network device type.
>> + *
>> + * Returns nothing.
>> + */
>> +void
>> +virNetDevLockGenName(virNetDevGenNameType type)
>> +{
>> +    virMutexLock(&virNetDevGenNames[type].mutex);
>> +}
>> +
>> +
>> +/**
>> + * virNetDevUnlockGenName:
>> + * @type: type of the network device
>> + *
>> + * Unlock the internal mutex for creation for this network device type.
>> + *
>> + * Returns nothing.
>> + */
>> +void
>> +virNetDevUnlockGenName(virNetDevGenNameType type)
>> +{
>> +    virMutexUnlock(&virNetDevGenNames[type].mutex);
>> +}
>> diff --git a/src/util/virnetdev.h b/src/util/virnetdev.h
>> index 53e606c6..19f37b61 100644
>> --- a/src/util/virnetdev.h
>> +++ b/src/util/virnetdev.h
>> @@ -40,6 +40,12 @@ typedef void virIfreq;
>>    */
>>   #define VIR_NET_GENERATED_TAP_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,
>>      VIR_NETDEV_RX_FILTER_MODE_NORMAL,
>> @@ -145,6 +151,24 @@ struct _virNetDevCoalesce {
>>       uint32_t rate_sample_interval;
>>   };
>>  
>> +typedef enum {
>> +    VIR_NET_DEV_GEN_NAME_NONE = 0,
>
>
>See comment above about removing ..._NONE 

Okay.

>
>> +    VIR_NET_DEV_GEN_NAME_TAP,
>> +    VIR_NET_DEV_GEN_NAME_MACVTAP,
>> +    VIR_NET_DEV_GEN_NAME_MACVLAN,
>> +    VIR_NET_DEV_GEN_NAME_LAST
>> +} virNetDevGenNameType;
>> +
>> +VIR_ENUM_DECL(virNetDevGenNameType);
>
>
>Also about removing VIR_ENUM_DECL() (unless we end up with a reason for
>virNetDevGenNameType(To|From)String())
> 

Okay.

>
>> +
>> +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 +345,12 @@ int virNetDevVFInterfaceStats(virPCIDeviceAddressPtr vfAddr,
>>   ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
>>  
>>   G_DEFINE_AUTOPTR_CLEANUP_FUNC(virNetDevRxFilter, virNetDevRxFilterFree);
>> +
>> +void virNetDevReserveName(const char *name,
>> +                          virNetDevGenNameType type,
>> +                          bool locked);
>> +
>> +int virNetDevGenerateName(char **ifname, virNetDevGenNameType type);
>> +
>> +void virNetDevLockGenName(virNetDevGenNameType type);
>> +void virNetDevUnlockGenName(virNetDevGenNameType type);
>
>
>Otherwise this looks good!
>
>




[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