Re: [libvirt] [PATCHv2] util: keep/use a bitmap of in-use macvtap devices
On 01/25/2016 07:16 AM, Pavel Hrdina wrote:
On Fri, Jan 22, 2016 at 12:52:27PM -0500, Laine Stump wrote:
> This patch creates two bitmaps, one for macvlan devicenames and one
s/devicenames/device names/
[...]
Done.
> diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c
[...]
> +/**
> + * virNetDevMacVLanReserveName:
> + *
> + * @name: already-known name of device
> + * @quietFail: don't log an error if this name is already in-use
> + *
> + * Extract the device type and id from a macvtap/macvlan device name
> + * and mark the appropriate position as in-use in the appropriate
> + * bitmap.
> + *
> + * returns 0 on success, -1 on failure, -2 if the name doesn't fit the
auto-pattern
Long line, would be nice to wrap it. And the return 0 isn't true, because
virNetDevMacVLanReserveID() returns ID on success.
The danger of documenting the function when it's initially written,
rather than doing it as an afterthought after everything is wrapped up :-).
> + */
> +int
> +virNetDevMacVLanReserveName(const char *name, bool quietFail)
> +{
> + unsigned int id;
> + unsigned int flags = 0;
> + const char *idstr = NULL;
> +
> + if (virNetDevMacVLanInitialize() < 0)
> + return -1;
> +
> + if (STRPREFIX(name, MACVTAP_NAME_PREFIX)) {
> + idstr = name + strlen(MACVTAP_NAME_PREFIX);
> + flags |= VIR_NETDEV_MACVLAN_CREATE_WITH_TAP;
> + } else if (STRPREFIX(name, MACVLAN_NAME_PREFIX)) {
> + idstr = name + strlen(MACVLAN_NAME_PREFIX);
> + } else {
> + return -2;
> + }
> +
> + if (virStrToLong_ui(idstr, NULL, 10, &id) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("couldn't get id value from macvtap device name
%s"),
> + name);
> + return -1;
> + }
> + return virNetDevMacVLanReserveID(id, flags, quietFail, false);
> +}
[...]
> 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;
Maybe use the MACV(TAP|LAN)_NAME_PREFIX instead of the strings for *type.
Yeah, I wonder why it wasn't like that from the beginning (and for that
matter why they had a separate char* for prefix and type even though
they were identical. Maybe they were anticipating the day when libvirt
might change the names it uses for the devices? Dunno. Anyway, I've
changed it as you suggest.
> const char *pattern = (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) ?
> MACVTAP_NAME_PATTERN : MACVLAN_NAME_PATTERN;
> - int c, rc;
> + int rc, reservedID = -1;
> char ifname[IFNAMSIZ];
> int retries, do_retry = 0;
> uint32_t macvtapMode;
> - const char *cr_ifname = NULL;
> + const char *ifnameCreated = NULL;
> int ret;
> int vf = -1;
> bool vnet_hdr = flags & VIR_NETDEV_MACVLAN_VNET_HDR;
[...]
> + retries = MACVLAN_MAX_ID + 1;
> + while (!ifnameCreated && retries) {
> virMutexLock(&virNetDevMacVLanCreateMutex);
> - for (c = 0; c < 8192; c++) {
> - snprintf(ifname, sizeof(ifname), pattern, c);
> - if ((ret = virNetDevExists(ifname)) < 0) {
> - virMutexUnlock(&virNetDevMacVLanCreateMutex);
> + reservedID = virNetDevMacVLanReserveID(reservedID, flags, false, true);
> + if (reservedID < 0) {
> + virMutexUnlock(&virNetDevMacVLanCreateMutex);
> + return -1;
> + }
> + snprintf(ifname, sizeof(ifname), pattern, reservedID);
Since you're changing this, snprintf returns -1 in case of error.
Well, the man page says -1 is returned by the *printf functions "if an
output error is encountered", which would include an I/O error (moot for
snprintf() since it doesn't do any I/O) or a bad format string (but
we're calling it with a literal format string that only includes a
single %d, so it is known to be good). Checking the returned length is
pointless too, since by definition the longest possible result of
macvtap%d where the %d is guaranteed between 0 and 8192 is
"macvtap8192", which is 12 characters - safely below the limit of
IFNAMSIZ (16).
ACK with the issues fixed.