John Ferlan wrote:
> On 01/04/2013 10:00 AM, Roman Bogorodskiy wrote:
> > ---
> > src/util/virnetdevtap.c | 140 ++++++++++++++++++++++++++++++++++++++++++++++--
> > 1 file changed, 137 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/util/virnetdevtap.c b/src/util/virnetdevtap.c
> > index a884de1..426d629 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,141 @@ cleanup:
> > VIR_FORCE_CLOSE(fd);
> > return ret;
> > }
> > -#else /* ! TUNSETIFF */
>
> Change the comment to add the !defined(TUNSETIFF)
>
> > +#elif defined(__FreeBSD__)
> > +int virNetDevTapCreate(char **ifname,
> > + int *tapfd,
> > + unsigned int flags ATTRIBUTE_UNUSED)
> > +{
> > + int s;
> > + struct ifreq ifr;
> > + int ret = -1;
> > +
> > + s = socket(AF_LOCAL, SOCK_DGRAM, 0);
> > + if (s < 0) {
> > + virReportSystemError(errno, "%s",
> > + _("Unable to create socket"));
> > + return -1;
> > + }
> > +
> > + memset(&ifr, 0, sizeof(ifr));
> > +
> > + if (virStrcpyStatic(ifr.ifr_name, "tap") == NULL) {
> > + virReportSystemError(ERANGE,
> > + _("Network interface name '%s' is too long"),
> > + *ifname);
> > + goto cleanup;
> > +
> > + }
>
> Confused here - you are copying "tap", but using "*ifname" in the error
> message. Why not use the virNetDevSetupControl() if in fact you are
> trying to use "*ifname"?
Please see a commit above.
> > +
> > + if (ioctl(s, SIOCIFCREATE2, &ifr) < 0) {
> > + virReportSystemError(errno,
> > + _("Unable to create tap device %s"),
> > + NULLSTR(*ifname));
> > + goto cleanup;
> > + }
> > +
> > + char *newifname = NULL;
>
> Put this with the other declarations.
>
> > + if (strstr(*ifname, "%d") == NULL) {
> > + newifname = *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();
> > + ret = - 1;
>
> Redundant with initialization
>
> > + goto cleanup;
> > + }
> > +
> > + if (virNetDevExists(virBufferCurrentContent(&newname)) == 0) {
> > + newifname = strdup(virBufferContentAndReset(&newname));
> > + break;
> > + }
>
> In this instance memory is allocated for newifname, when is it free()'d?
> Secondarily if not allocated, then the general message below may mask
> the reason. That is although you failed to generate a new name, it was
> because you were out of memory.
>
> > + }
> > + }
> > +
> > + if (newifname == NULL) {
> > + virReportError(VIR_ERR_INTERNAL_ERROR,
> > + _("Failed to generate new name for interface %s"),
> > + ifr.ifr_name);
> > + ret = -1;
>
> Redundant with initialization
>
> > + goto cleanup;
> > + }
> > +
> > + if (virNetDevSetName(ifr.ifr_name, newifname) == -1) {
> > + virReportError(VIR_ERR_INTERNAL_ERROR,
> > + _("Failed to rename interface %s to %s"),
> > + ifr.ifr_name, newifname);
>
> This message is redundant with the virNetDevSetName() function.
>
> > + ret = -1;
>
> Redundant with initialization
>
> > + goto cleanup;
> > + }
> > +
> > + *ifname = newifname;
>
> This is where a possible memory leak occurs if *ifname had been alloc'd
> before we'd be now losing it. I see the other virNetDevTapCreate() will
> VIR_FREE(*ifname) prior to strdup() attempt.
>
> Secondarily, it seems to me that if we go through the "if strstr() ==
> NULL" path, then we're not really changing the name, correct? If that's
> the case, then the only time you need to change the name is in the else
> condition.
>
> I think the answer probably goes back to the virStrcpyStatic using "tap"
> above for what the expectations are here.
Hi,
Sorry for the late reply, it's been a busy week.
The idea behind this code is the following:
FreeBSD determines a type of the interface we're creating by its name,
so it's not possible to create a TAP device with name "vnet" right away.
Here, if we were requested to create an interface 'vnet%d', we pass
"tap" interface name to create a TAP interface with the first available
name (e.g. if we already have tap0 and tap1, the new one will be tap2).
Then we need to find a "vnet%d" name we can use for an interface. I
wasn't able to find a better way but going from vnet0, then vnet1 etc.
The final step would be to rename the TAP interface we just created to
the new 'vnet' name.
In case fif we were passed a full interface name (e.g. 'vnet10') instead
if a pattern (e.g. 'vnet%d'), we don't need to find a proper name and
can just rename our tap interface right away.
I will need to add a comment for that piece in the code and state that
in commit message as well.
And you're correct regarding the leak, I'll take that into account.
> > +
> > + if (tapfd) {
> > + char *dev_prefix = "/dev/";
> > + char *dev_path;
> > + int dev_path_len = strlen(dev_prefix) + strlen(*ifname) + 1;
> > +
> > + if (VIR_ALLOC_N(dev_path, dev_path_len) < 0) {
> > + virReportOOMError();
> > + ret = -1;
>
> Redundant with initialization
>
> > + goto cleanup;
> > + }
> > + snprintf(dev_path, dev_path_len, "%s%s", dev_prefix, *ifname);
> > +
> > + *tapfd = open(dev_path, O_RDWR);
> > +
> > + VIR_FREE(dev_path);
> > + }
> > +
> > + ret = 0;
> > +cleanup:
> > + if (ret < 0)
> > + VIR_FORCE_CLOSE(s);
> > +
> > + return ret;
> > +}
> > +
> > +int virNetDevTapDelete(const char *ifname)
> > +{
> > + int s;
> > + struct ifreq ifr;
> > + int ret = -1;
> > +
> > + s = socket(AF_LOCAL, SOCK_DGRAM, 0);
> > + if (s < 0) {
> > + virReportSystemError(errno, "%s",
> > + _("Unable to create socket"));
> > + return -1;
> > + }
> > +
> > + memset(&ifr, 0, sizeof(ifr));
> > +
> > + if (virStrcpyStatic(ifr.ifr_name, ifname) == NULL) {
> > + virReportSystemError(ERANGE,
> > + _("Network interface name '%s' is too long"),
> > + ifname);
> > + goto cleanup;
> > + }
>
> Why not use virNetDevSetupControl()?
Unfortunately, it's declared static, so we cannot re-use it here.
Thanks for the review, I'm working on fixing issues you pointed out.
> > +
> > + 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
> > int virNetDevTapCreate(char **ifname ATTRIBUTE_UNUSED,
> > int *tapfd ATTRIBUTE_UNUSED,
> > unsigned int flags ATTRIBUTE_UNUSED)
> > @@ -245,7 +379,7 @@ int virNetDevTapDelete(const char *ifname ATTRIBUTE_UNUSED)
> > _("Unable to delete TAP devices on this platform"));
> > return -1;
> > }
> > -#endif /* ! TUNSETIFF */
> > +#endif
> >
>
> Add a /* defined(__FreeBSD__) */ to the #endif for clarity
> >
> > /**
> >
>
Roman Bogorodskiy
Attachment:
pgpy_Fg3rQO0T.pgp
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list