Re: [libvirt] [PATCH 2/4] FreeBSD: implement virNetDevTapCreate() and virNetDevTapDelete().

2013-01-15 Thread Eric Blake 
On 01/13/2013 05:51 AM, Roman Bogorodskiy wrote:


 Why not use virNetDevSetupControl()?
 
 Unfortunately, it's declared static, so we cannot re-use it here.

But there's nothing wrong with changing it to not be static, adding it
to src/libvirt_private.syms, and using it, instead of copying its body.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 2/4] FreeBSD: implement virNetDevTapCreate() and virNetDevTapDelete().

2013-01-13 Thread Roman Bogorodskiy 
  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

Re: [libvirt] [PATCH 2/4] FreeBSD: implement virNetDevTapCreate() and virNetDevTapDelete().

2013-01-07 Thread John Ferlan 
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?

 +
 +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.

 +
 +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));
 +

[libvirt] [PATCH 2/4] FreeBSD: implement virNetDevTapCreate() and virNetDevTapDelete().

2013-01-04 Thread Roman Bogorodskiy 
---
 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 */
+#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;
+
+}
+
+if (ioctl(s, SIOCIFCREATE2, ifr)  0) {
+virReportSystemError(errno,
+ _(Unable to create tap device %s),
+ NULLSTR(*ifname));
+goto cleanup;
+}
+
+char *newifname = NULL;
+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;
+goto cleanup;
+}
+
+if (virNetDevExists(virBufferCurrentContent(newname)) == 0) {
+newifname = strdup(virBufferContentAndReset(newname));
+break;
+}
+}
+}
+
+if (newifname == NULL) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _(Failed to generate new name for interface %s),
+   ifr.ifr_name);
+ret = -1;
+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);
+ret = -1;
+goto cleanup;
+}
+
+*ifname = newifname;
+
+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;
+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;
+}
+
+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
 
 
 /**
-- 
1.8.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list