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
>
>
> /**
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list