On 01/20/2013 11:22 AM, Roman Bogorodskiy wrote:
--- src/util/virnetdevtap.c | 112 ++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 109 insertions(+), 3 deletions(-)
diff --git a/src/util/virnetdevtap.c b/src/util/virnetdevtap.c index a884de1..1b02e1f 100644 --- a/src/util/virnetdevtap.c +++ b/src/util/virnetdevtap.c @@ -103,7 +103,7 @@ virNetDevProbeVnetHdr(int tapfd) #endif
-#ifdef TUNSETIFF +#if defined(TUNSETIFF) /** * virNetDevTapCreate: * @ifname: the interface name @@ -230,7 +230,113 @@ cleanup: VIR_FORCE_CLOSE(fd); return ret; } -#else /* ! TUNSETIFF */ +#elif defined(__FreeBSD__) +int virNetDevTapCreate(char **ifname, + int *tapfd, + unsigned int flags ATTRIBUTE_UNUSED) +{ + int s; + struct ifreq ifr; + int ret = -1; + char *newifname = NULL; + + /* As FreeBSD determines interface type by name, + * we have to create 'tap' interface first and + * then rename it to 'vnet' + */ + if ((s = virNetDevSetupControl("tap", &ifr)) < 0) + return -1; + + if (ioctl(s, SIOCIFCREATE2, &ifr) < 0) { + virReportSystemError(errno, "%s", + _("Unable to create tap device")); + goto cleanup; + }
Trying to figure the relationship between this socket and the file (eg, tapfd) created below).
+ + /* In case we were given exact interface name (e.g. 'vnetN'), + * we just rename to it. If we have format string like + * 'vnet%d', we need to find the first available name that + * matches this pattern + */ + if (strstr(*ifname, "%d") == NULL) { + newifname = strdup(*ifname); + } else { + int i; + for (i = 0; i <= 255; i++) { + virBuffer newname = VIR_BUFFER_INITIALIZER; + virBufferAsprintf(&newname, *ifname, i); + + if (virBufferError(&newname)) { + virBufferFreeAndReset(&newname); + virReportOOMError(); + goto cleanup; + } + + if (virNetDevExists(virBufferCurrentContent(&newname)) == 0) { + newifname = strdup(virBufferContentAndReset(&newname)); + break; + } + } + } + + VIR_FREE(*ifname); + + if (newifname == NULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to generate new name for interface %s"), + ifr.ifr_name); + goto cleanup; + }
My comments from v1 still apply above. Since the "else" part of the above check is just replacing *ifname, then that's the only time we need to mess with *ifname. That is use "!=" and just replace *ifname at the end of the for loop as long as we generated one. The error message here would be wrong if you went through the existing "if" condition - it failed to strdup(). Thus you have: if (strstr(*ifname, "%d") != NULL) { int i; for (i = 0; i <= 255; i++) { virBuffer newname = VIR_BUFFER_INITIALIZER; virBufferAsprintf(&newname, *ifname, i); if (virBufferError(&newname)) { virBufferFreeAndReset(&newname); virReportOOMError(); goto cleanup; } if (virNetDevExists(virBufferCurrentContent(&newname)) == 0) { newifname = strdup(virBufferContentAndReset(&newname)); break; } } if (newifname) { VIR_FREE(*ifname); *ifname = newifname; } else { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to generate new name for interface %s"), ifr.ifr_name); goto cleanup; } } Also, I missed this the first time around, is there an existing constant for 255? Or is it/will it be dynamic? Beyond that I have some "thoughts" around the use of 255. First, it could be a time consuming loop - there's a lot of open, ioctl/check, close going on (and that doesn't include all the printf & Buffer mgmt code). Second, what are the chances some day someone wants 1024 tap devices. This loop is then outdated and even worse performance wise. It's too bad there isn't a way to just request the next available device and let the driver handle things rather than a linear algorithm which will cause performance problems some day. Yes, I've been down this road before.
+ + if (virNetDevSetName(ifr.ifr_name, newifname) == -1) {
Assuming the above change, then 's/newifname/*ifname/'
+ goto cleanup; + } + + *ifname = newifname;
Removing the above too. The way the code is written now, if you goto cleanup from the SetName, then newifname is leaked.
+ + if (tapfd) { + char *dev_path = NULL; + if (virAsprintf(&dev_path, "/dev/%s", *ifname) < 0) { + virReportOOMError(); + goto cleanup; + } + + *tapfd = open(dev_path, O_RDWR); + + VIR_FREE(dev_path); + } + + ret = 0; +cleanup: + if (ret < 0) + VIR_FORCE_CLOSE(s); + + return ret; +}
On success, we leave here with 's' and 'tapfd' being opened, right? Since the TapDelete will open another 's', should the "if (ret < 0)" above be removed and we always close 's'? If tapfd == NULL, then what happens? Shouldn't the device path still be opened & closed (like the "existing" code)? As it stands you return zero, but haven't necessarily opened the new dev_path. I'm trying to learn the relationship between the two. What happens when/if *tapfd == -1 because open() failed? and ret = 0?
+ +int virNetDevTapDelete(const char *ifname) +{ + int s; + struct ifreq ifr; + int ret = -1; + + if ((s = virNetDevSetupControl(ifname, &ifr)) < 0) + return -1; + + if (ioctl(s, SIOCIFDESTROY, &ifr) < 0) { + virReportSystemError(errno, + _("Unable to remove tap device %s"), + ifname); + goto cleanup; + } + + ret = 0; +cleanup: + VIR_FORCE_CLOSE(s); + return ret; +} + +#else /* !defined(__FreeBSD__) */ int virNetDevTapCreate(char **ifname ATTRIBUTE_UNUSED, int *tapfd ATTRIBUTE_UNUSED, unsigned int flags ATTRIBUTE_UNUSED) @@ -245,7 +351,7 @@ int virNetDevTapDelete(const char *ifname ATTRIBUTE_UNUSED) _("Unable to delete TAP devices on this platform")); return -1; } -#endif /* ! TUNSETIFF */ +#endif
/**