1:56 p.m.
Refactor code, clean up error handling, and finally add bond support. The
last patch optionally supports a patch I submitted to the Linux kernel
which should go in for 3.9 (it was just accepted for net-next).
After this patch when you have a bond device you'll get the following:
$ ./tools/virsh iface-dumpxml br0
<interface type='bridge' name='br0'>
<mtu size='1500'/>
<bridge stp='on' delay='1499'>
<interface type='bond' name='bond0'>
<mtu size='1500'/>
<bond mode='balance-rr'>
<interface type='ethernet' name='eth2'>
<mac address='d0:67:e5:fa:88:95'/>
<mtu size='1500'/>
</interface>
<interface type='ethernet' name='eth3'>
<mac address='d0:67:e5:fa:88:95'/>
<mtu size='1500'/>
</interface>
</bond>
</interface>
<!-- incorrectly including guest tap devices, but was an issue
prior and will be fixed in a later series -->
</bridge>
</interface>
Doug Goldstein (6):
interface: Refactor udev bridge to helper func
interface: udev bridge code error handling updates
interface: Refactor interface vlan to helper func
interface: Improve udev backend device type id
interface: add bond support to udev backend
interface: dev type support for bond interfaces
src/interface/interface_backend_udev.c | 546 +++++++++++++++++++++++++++------
1 file changed, 451 insertions(+), 95 deletions(-)
--
1.7.12.4
1:56 p.m.
New subject: [libvirt] [PATCHv2 1/6] interface: Refactor udev bridge to helper func
Mechanical move to break up udevIfaceGetIfaceDef() into different
helpers for each of the interface types to hopefully make the code
easier to follow. This moves the bridge code to
udevIfaceGetIfaceDefBridge().
---
src/interface/interface_backend_udev.c | 155 ++++++++++++++++++---------------
1 file changed, 86 insertions(+), 69 deletions(-)
diff --git a/src/interface/interface_backend_udev.c
b/src/interface/interface_backend_udev.c
index 2c41bde..780a472 100644
--- a/src/interface/interface_backend_udev.c
+++ b/src/interface/interface_backend_udev.c
@@ -41,6 +41,8 @@ typedef enum {
VIR_UDEV_IFACE_ALL
} virUdevStatus ;
+static virInterfaceDef *udevIfaceGetIfaceDef(struct udev *udev, const char *name);
+
static const char *
virUdevStatusString(virUdevStatus status)
{
@@ -492,14 +494,14 @@ err:
}
/**
- * Helper function for our use of scandir()
+ * Helper function for finding bridge members using scandir()
*
* @param entry - directory entry passed by scandir()
*
* @return 1 if we want to add it to scandir's list, 0 if not.
*/
static int
-udevIfaceScanDirFilter(const struct dirent *entry)
+udevIfaceBridgeScanDirFilter(const struct dirent *entry)
{
if (STREQ(entry->d_name, ".") || STREQ(entry->d_name,
".."))
return 0;
@@ -538,18 +540,95 @@ udevIfaceFreeIfaceDef(virInterfaceDef *ifacedef)
VIR_FREE(ifacedef);
}
+static int
+ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
+ATTRIBUTE_NONNULL(4) ATTRIBUTE_RETURN_CHECK
+udevIfaceGetIfaceDefBridge(struct udev *udev,
+ struct udev_device *dev,
+ const char *name,
+ virInterfaceDef *ifacedef)
+{
+ struct dirent **member_list = NULL;
+ int member_count = 0;
+ char *member_path;
+ const char *stp_str;
+ int stp;
+ int i;
+
+ /* Set our type to Bridge */
+ ifacedef->type = VIR_INTERFACE_TYPE_BRIDGE;
+
+ /* Bridge specifics */
+ ifacedef->data.bridge.delay =
+ strdup(udev_device_get_sysattr_value(dev, "bridge/forward_delay"));
+ if (!ifacedef->data.bridge.delay) {
+ virReportOOMError();
+ goto cleanup;
+ }
+
+ stp_str = udev_device_get_sysattr_value(dev, "bridge/stp_state");
+ if (virStrToLong_i(stp_str, NULL, 10, &stp) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not parse STP state '%s'"), stp_str);
+ goto cleanup;
+ }
+ ifacedef->data.bridge.stp = stp;
+
+ /* Members of the bridge */
+ if (virAsprintf(&member_path, "%s/%s",
+ udev_device_get_syspath(dev), "brif") < 0) {
+ virReportOOMError();
+ goto cleanup;
+ }
+
+ /* Get each member of the bridge */
+ member_count = scandir(member_path, &member_list,
+ udevIfaceBridgeScanDirFilter, alphasort);
+
+ /* Don't need the path anymore */
+ VIR_FREE(member_path);
+
+ if (member_count < 0) {
+ virReportSystemError(errno,
+ _("Could not get members of bridge '%s'"),
+ name);
+ goto cleanup;
+ }
+
+ /* Allocate our list of member devices */
+ if (VIR_ALLOC_N(ifacedef->data.bridge.itf, member_count) < 0) {
+ virReportOOMError();
+ goto cleanup;
+ }
+ ifacedef->data.bridge.nbItf = member_count;
+
+ for (i = 0; i < member_count; i++) {
+ ifacedef->data.bridge.itf[i] =
+ udevIfaceGetIfaceDef(udev, member_list[i]->d_name);
+ VIR_FREE(member_list[i]);
+ }
+
+ VIR_FREE(member_list);
+
+ return 0;
+
+cleanup:
+ for (i = 0; i < member_count; i++) {
+ VIR_FREE(member_list[i]);
+ }
+ VIR_FREE(member_list);
+
+ return -1;
+}
static virInterfaceDef * ATTRIBUTE_NONNULL(1)
-udevIfaceGetIfaceDef(struct udev *udev, char *name)
+udevIfaceGetIfaceDef(struct udev *udev, const char *name)
{
struct udev_device *dev = NULL;
virInterfaceDef *ifacedef;
unsigned int mtu;
const char *mtu_str;
char *vlan_parent_dev = NULL;
- struct dirent **member_list = NULL;
- int member_count = 0;
- int i;
/* Allocate our interface definition structure */
if (VIR_ALLOC(ifacedef) < 0) {
@@ -629,66 +708,8 @@ udevIfaceGetIfaceDef(struct udev *udev, char *name)
ifacedef->data.vlan.tag = vid;
ifacedef->data.vlan.devname = vlan_parent_dev;
} else if (STREQ_NULLABLE(udev_device_get_devtype(dev), "bridge")) {
- /* Check if its a bridge device */
- char *member_path;
- const char *stp_str;
- int stp;
-
- /* Set our type to Bridge */
- ifacedef->type = VIR_INTERFACE_TYPE_BRIDGE;
-
- /* Bridge specifics */
- ifacedef->data.bridge.delay =
- strdup(udev_device_get_sysattr_value(dev,
"bridge/forward_delay"));
- if (!ifacedef->data.bridge.delay) {
- virReportOOMError();
- goto cleanup;
- }
-
- stp_str = udev_device_get_sysattr_value(dev, "bridge/stp_state");
- if (virStrToLong_i(stp_str, NULL, 10, &stp) < 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Could not parse STP state '%s'"),
stp_str);
- goto cleanup;
- }
- ifacedef->data.bridge.stp = stp;
-
- /* Members of the bridge */
- if (virAsprintf(&member_path, "%s/%s",
- udev_device_get_syspath(dev), "brif") < 0) {
- virReportOOMError();
+ if (udevIfaceGetIfaceDefBridge(udev, dev, name, ifacedef) < 0)
goto cleanup;
- }
-
- /* Get each member of the bridge */
- member_count = scandir(member_path, &member_list,
- udevIfaceScanDirFilter, alphasort);
-
- /* Don't need the path anymore */
- VIR_FREE(member_path);
-
- if (member_count < 0) {
- virReportSystemError(errno,
- _("Could not get members of bridge
'%s'"),
- name);
- goto cleanup;
- }
-
- /* Allocate our list of member devices */
- if (VIR_ALLOC_N(ifacedef->data.bridge.itf, member_count) < 0) {
- virReportOOMError();
- goto cleanup;
- }
- ifacedef->data.bridge.nbItf = member_count;
-
- for (i = 0; i < member_count; i++) {
- ifacedef->data.bridge.itf[i] =
- udevIfaceGetIfaceDef(udev, member_list[i]->d_name);
- VIR_FREE(member_list[i]);
- }
-
- VIR_FREE(member_list);
-
} else {
/* Set our type to ethernet */
ifacedef->type = VIR_INTERFACE_TYPE_ETHERNET;
@@ -700,10 +721,6 @@ udevIfaceGetIfaceDef(struct udev *udev, char *name)
cleanup:
udev_device_unref(dev);
- for (i = 0; i < member_count; i++) {
- VIR_FREE(member_list[i]);
- }
- VIR_FREE(member_list);
udevIfaceFreeIfaceDef(ifacedef);
--
1.7.12.4
2:57 p.m.
New subject: [libvirt] [PATCHv2 1/6] interface: Refactor udev bridge to helper func
On 02/20/2013 02:56 PM, Doug Goldstein wrote:
Mechanical move to break up udevIfaceGetIfaceDef() into different
helpers for each of the interface types to hopefully make the code
easier to follow. This moves the bridge code to
udevIfaceGetIfaceDefBridge().
---
src/interface/interface_backend_udev.c | 155 ++++++++++++++++++---------------
1 file changed, 86 insertions(+), 69 deletions(-)
diff --git a/src/interface/interface_backend_udev.c
b/src/interface/interface_backend_udev.c
index 2c41bde..780a472 100644
--- a/src/interface/interface_backend_udev.c
+++ b/src/interface/interface_backend_udev.c
@@ -41,6 +41,8 @@ typedef enum {
VIR_UDEV_IFACE_ALL
} virUdevStatus ;
+static virInterfaceDef *udevIfaceGetIfaceDef(struct udev *udev, const char *name);
+
static const char *
virUdevStatusString(virUdevStatus status)
{
@@ -492,14 +494,14 @@ err:
}
/**
- * Helper function for our use of scandir()
+ * Helper function for finding bridge members using scandir()
*
* @param entry - directory entry passed by scandir()
*
* @return 1 if we want to add it to scandir's list, 0 if not.
*/
static int
-udevIfaceScanDirFilter(const struct dirent *entry)
+udevIfaceBridgeScanDirFilter(const struct dirent *entry)
{
if (STREQ(entry->d_name, ".") || STREQ(entry->d_name,
".."))
return 0;
@@ -538,18 +540,95 @@ udevIfaceFreeIfaceDef(virInterfaceDef *ifacedef)
VIR_FREE(ifacedef);
}
+static int
+ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
+ATTRIBUTE_NONNULL(4) ATTRIBUTE_RETURN_CHECK
+udevIfaceGetIfaceDefBridge(struct udev *udev,
+ struct udev_device *dev,
+ const char *name,
+ virInterfaceDef *ifacedef)
+{
+ struct dirent **member_list = NULL;
+ int member_count = 0;
+ char *member_path;
+ const char *stp_str;
+ int stp;
+ int i;
+
+ /* Set our type to Bridge */
+ ifacedef->type = VIR_INTERFACE_TYPE_BRIDGE;
+
+ /* Bridge specifics */
+ ifacedef->data.bridge.delay =
+ strdup(udev_device_get_sysattr_value(dev, "bridge/forward_delay"));
Okay, so I guess this udev function never returns NULL (unless there is
an out of memory error). And I'll assume that for the others I asked
about too.
+ if (!ifacedef->data.bridge.delay) {
+ virReportOOMError();
+ goto cleanup;
+ }
+
+ stp_str = udev_device_get_sysattr_value(dev, "bridge/stp_state");
+ if (virStrToLong_i(stp_str, NULL, 10, &stp) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not parse STP state '%s'"), stp_str);
+ goto cleanup;
+ }
+ ifacedef->data.bridge.stp = stp;
You certainly trust udev to return sane values more than I do, but I
guess maybe I'm just paranoid :-P
+
+ /* Members of the bridge */
+ if (virAsprintf(&member_path, "%s/%s",
+ udev_device_get_syspath(dev), "brif") < 0) {
+ virReportOOMError();
+ goto cleanup;
+ }
+
+ /* Get each member of the bridge */
+ member_count = scandir(member_path, &member_list,
+ udevIfaceBridgeScanDirFilter, alphasort);
As we discussed before, this will include all the guest tap devices, but
for now that's acceptable.
+
+ /* Don't need the path anymore */
+ VIR_FREE(member_path);
+
+ if (member_count < 0) {
+ virReportSystemError(errno,
+ _("Could not get members of bridge '%s'"),
+ name);
+ goto cleanup;
+ }
+
+ /* Allocate our list of member devices */
+ if (VIR_ALLOC_N(ifacedef->data.bridge.itf, member_count) < 0) {
+ virReportOOMError();
+ goto cleanup;
+ }
+ ifacedef->data.bridge.nbItf = member_count;
+
+ for (i = 0; i < member_count; i++) {
+ ifacedef->data.bridge.itf[i] =
+ udevIfaceGetIfaceDef(udev, member_list[i]->d_name);
Again you're being very trusting.
+ VIR_FREE(member_list[i]);
+ }
+
+ VIR_FREE(member_list);
+
+ return 0;
+
+cleanup:
+ for (i = 0; i < member_count; i++) {
+ VIR_FREE(member_list[i]);
+ }
+ VIR_FREE(member_list);
+
+ return -1;
+}
static virInterfaceDef * ATTRIBUTE_NONNULL(1)
-udevIfaceGetIfaceDef(struct udev *udev, char *name)
+udevIfaceGetIfaceDef(struct udev *udev, const char *name)
{
struct udev_device *dev = NULL;
virInterfaceDef *ifacedef;
unsigned int mtu;
const char *mtu_str;
char *vlan_parent_dev = NULL;
- struct dirent **member_list = NULL;
- int member_count = 0;
- int i;
/* Allocate our interface definition structure */
if (VIR_ALLOC(ifacedef) < 0) {
@@ -629,66 +708,8 @@ udevIfaceGetIfaceDef(struct udev *udev, char *name)
ifacedef->data.vlan.tag = vid;
ifacedef->data.vlan.devname = vlan_parent_dev;
} else if (STREQ_NULLABLE(udev_device_get_devtype(dev), "bridge")) {
- /* Check if its a bridge device */
- char *member_path;
- const char *stp_str;
- int stp;
-
- /* Set our type to Bridge */
- ifacedef->type = VIR_INTERFACE_TYPE_BRIDGE;
-
- /* Bridge specifics */
- ifacedef->data.bridge.delay =
- strdup(udev_device_get_sysattr_value(dev,
"bridge/forward_delay"));
- if (!ifacedef->data.bridge.delay) {
- virReportOOMError();
- goto cleanup;
- }
-
- stp_str = udev_device_get_sysattr_value(dev, "bridge/stp_state");
- if (virStrToLong_i(stp_str, NULL, 10, &stp) < 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Could not parse STP state '%s'"),
stp_str);
- goto cleanup;
- }
- ifacedef->data.bridge.stp = stp;
-
- /* Members of the bridge */
- if (virAsprintf(&member_path, "%s/%s",
- udev_device_get_syspath(dev), "brif") < 0) {
- virReportOOMError();
+ if (udevIfaceGetIfaceDefBridge(udev, dev, name, ifacedef) < 0)
goto cleanup;
- }
-
- /* Get each member of the bridge */
- member_count = scandir(member_path, &member_list,
- udevIfaceScanDirFilter, alphasort);
-
- /* Don't need the path anymore */
- VIR_FREE(member_path);
-
- if (member_count < 0) {
- virReportSystemError(errno,
- _("Could not get members of bridge
'%s'"),
- name);
- goto cleanup;
- }
-
- /* Allocate our list of member devices */
- if (VIR_ALLOC_N(ifacedef->data.bridge.itf, member_count) < 0) {
- virReportOOMError();
- goto cleanup;
- }
- ifacedef->data.bridge.nbItf = member_count;
-
- for (i = 0; i < member_count; i++) {
- ifacedef->data.bridge.itf[i] =
- udevIfaceGetIfaceDef(udev, member_list[i]->d_name);
- VIR_FREE(member_list[i]);
- }
-
- VIR_FREE(member_list);
-
} else {
/* Set our type to ethernet */
ifacedef->type = VIR_INTERFACE_TYPE_ETHERNET;
@@ -700,10 +721,6 @@ udevIfaceGetIfaceDef(struct udev *udev, char *name)
cleanup:
udev_device_unref(dev);
- for (i = 0; i < member_count; i++) {
- VIR_FREE(member_list[i]);
- }
- VIR_FREE(member_list);
udevIfaceFreeIfaceDef(ifacedef);
ACK.
1:56 p.m.
New subject: [libvirt] [PATCHv2 2/6] interface: udev bridge code error handling updates
Based on feedback from Laine Stump, improve a number of the error
handling cases to report the issue to the user instead of not generating
data or giving vague errors. Added the bridge device name to every error
message as well to make it clear which bridge failed.
---
src/interface/interface_backend_udev.c | 61 ++++++++++++++++++++++++++--------
1 file changed, 47 insertions(+), 14 deletions(-)
diff --git a/src/interface/interface_backend_udev.c
b/src/interface/interface_backend_udev.c
index 780a472..f5b44ea 100644
--- a/src/interface/interface_backend_udev.c
+++ b/src/interface/interface_backend_udev.c
@@ -551,34 +551,60 @@ udevIfaceGetIfaceDefBridge(struct udev *udev,
struct dirent **member_list = NULL;
int member_count = 0;
char *member_path;
- const char *stp_str;
+ const char *tmp_str;
int stp;
int i;
/* Set our type to Bridge */
ifacedef->type = VIR_INTERFACE_TYPE_BRIDGE;
- /* Bridge specifics */
- ifacedef->data.bridge.delay =
- strdup(udev_device_get_sysattr_value(dev, "bridge/forward_delay"));
+ /* Retrieve the forward delay */
+ tmp_str = udev_device_get_sysattr_value(dev, "bridge/forward_delay");
+ if (!tmp_str) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not retrieve 'bridge/forward_delay' for
'%s'"), name);
+ goto err;
+ }
+
+ ifacedef->data.bridge.delay = strdup(tmp_str);
if (!ifacedef->data.bridge.delay) {
virReportOOMError();
- goto cleanup;
+ goto err;
}
- stp_str = udev_device_get_sysattr_value(dev, "bridge/stp_state");
- if (virStrToLong_i(stp_str, NULL, 10, &stp) < 0) {
+ /* Retrieve Spanning Tree State. Valid values = -1, 0, 1 */
+ tmp_str = udev_device_get_sysattr_value(dev, "bridge/stp_state");
+ if (!tmp_str) {
virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Could not parse STP state '%s'"), stp_str);
- goto cleanup;
+ _("Could not retrieve 'bridge/stp_state' for
'%s'"), name);
+ goto err;
+ }
+
+ if (virStrToLong_i(tmp_str, NULL, 10, &stp) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not parse 'bridge/stp_state' '%s' for
'%s'"),
+ tmp_str, name);
+ goto err;
+ }
+
+ switch (stp) {
+ case -1:
+ case 0:
+ case 1:
+ ifacedef->data.bridge.stp = stp;
+ break;
+ default:
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Invalid STP state value %d received for '%s'. Must be "
+ "-1, 0, or 1."), stp, name);
+ goto err;
}
- ifacedef->data.bridge.stp = stp;
/* Members of the bridge */
if (virAsprintf(&member_path, "%s/%s",
udev_device_get_syspath(dev), "brif") < 0) {
virReportOOMError();
- goto cleanup;
+ goto err;
}
/* Get each member of the bridge */
@@ -592,19 +618,26 @@ udevIfaceGetIfaceDefBridge(struct udev *udev,
virReportSystemError(errno,
_("Could not get members of bridge '%s'"),
name);
- goto cleanup;
+ goto err;
}
/* Allocate our list of member devices */
if (VIR_ALLOC_N(ifacedef->data.bridge.itf, member_count) < 0) {
virReportOOMError();
- goto cleanup;
+ goto err;
}
ifacedef->data.bridge.nbItf = member_count;
+ /* Get the interface defintions for each member of the bridge */
for (i = 0; i < member_count; i++) {
ifacedef->data.bridge.itf[i] =
udevIfaceGetIfaceDef(udev, member_list[i]->d_name);
+ if (!ifacedef->data.bridge.itf[i]) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not get interface information for '%s', which is
"
+ "a member of bridge '%s'"),
member_list[i]->d_name, name);
+ goto err;
+ }
VIR_FREE(member_list[i]);
}
@@ -612,7 +645,7 @@ udevIfaceGetIfaceDefBridge(struct udev *udev,
return 0;
-cleanup:
+err:
for (i = 0; i < member_count; i++) {
VIR_FREE(member_list[i]);
}
--
1.7.12.4
2:59 p.m.
New subject: [libvirt] [PATCHv2 2/6] interface: udev bridge code error handling updates
Ah, *now* I understand. You did the extra error checking in a separate
patch!
On 02/20/2013 02:56 PM, Doug Goldstein wrote:
Based on feedback from Laine Stump, improve a number of the error
handling cases to report the issue to the user instead of not generating
data or giving vague errors. Added the bridge device name to every error
message as well to make it clear which bridge failed.
---
src/interface/interface_backend_udev.c | 61 ++++++++++++++++++++++++++--------
1 file changed, 47 insertions(+), 14 deletions(-)
diff --git a/src/interface/interface_backend_udev.c
b/src/interface/interface_backend_udev.c
index 780a472..f5b44ea 100644
--- a/src/interface/interface_backend_udev.c
+++ b/src/interface/interface_backend_udev.c
@@ -551,34 +551,60 @@ udevIfaceGetIfaceDefBridge(struct udev *udev,
struct dirent **member_list = NULL;
int member_count = 0;
char *member_path;
- const char *stp_str;
+ const char *tmp_str;
int stp;
int i;
/* Set our type to Bridge */
ifacedef->type = VIR_INTERFACE_TYPE_BRIDGE;
- /* Bridge specifics */
- ifacedef->data.bridge.delay =
- strdup(udev_device_get_sysattr_value(dev, "bridge/forward_delay"));
+ /* Retrieve the forward delay */
+ tmp_str = udev_device_get_sysattr_value(dev, "bridge/forward_delay");
+ if (!tmp_str) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not retrieve 'bridge/forward_delay' for
'%s'"), name);
+ goto err;
Please name this label "error" instead of "err" to fit with the rest
of
libvirt.
+ }
+
+ ifacedef->data.bridge.delay = strdup(tmp_str);
if (!ifacedef->data.bridge.delay) {
virReportOOMError();
- goto cleanup;
+ goto err;
}
- stp_str = udev_device_get_sysattr_value(dev, "bridge/stp_state");
- if (virStrToLong_i(stp_str, NULL, 10, &stp) < 0) {
+ /* Retrieve Spanning Tree State. Valid values = -1, 0, 1 */
+ tmp_str = udev_device_get_sysattr_value(dev, "bridge/stp_state");
+ if (!tmp_str) {
virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Could not parse STP state '%s'"), stp_str);
- goto cleanup;
+ _("Could not retrieve 'bridge/stp_state' for
'%s'"), name);
+ goto err;
+ }
+
+ if (virStrToLong_i(tmp_str, NULL, 10, &stp) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not parse 'bridge/stp_state' '%s' for
'%s'"),
+ tmp_str, name);
+ goto err;
+ }
+
+ switch (stp) {
+ case -1:
+ case 0:
+ case 1:
+ ifacedef->data.bridge.stp = stp;
+ break;
+ default:
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Invalid STP state value %d received for '%s'. Must be
"
+ "-1, 0, or 1."), stp, name);
+ goto err;
}
- ifacedef->data.bridge.stp = stp;
/* Members of the bridge */
if (virAsprintf(&member_path, "%s/%s",
udev_device_get_syspath(dev), "brif") < 0) {
virReportOOMError();
- goto cleanup;
+ goto err;
}
/* Get each member of the bridge */
@@ -592,19 +618,26 @@ udevIfaceGetIfaceDefBridge(struct udev *udev,
virReportSystemError(errno,
_("Could not get members of bridge '%s'"),
name);
- goto cleanup;
+ goto err;
}
/* Allocate our list of member devices */
if (VIR_ALLOC_N(ifacedef->data.bridge.itf, member_count) < 0) {
virReportOOMError();
- goto cleanup;
+ goto err;
}
ifacedef->data.bridge.nbItf = member_count;
+ /* Get the interface defintions for each member of the bridge */
for (i = 0; i < member_count; i++) {
ifacedef->data.bridge.itf[i] =
udevIfaceGetIfaceDef(udev, member_list[i]->d_name);
+ if (!ifacedef->data.bridge.itf[i]) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not get interface information for '%s', which is
"
+ "a member of bridge '%s'"),
member_list[i]->d_name, name);
+ goto err;
+ }
VIR_FREE(member_list[i]);
}
@@ -612,7 +645,7 @@ udevIfaceGetIfaceDefBridge(struct udev *udev,
return 0;
-cleanup:
+err:
for (i = 0; i < member_count; i++) {
VIR_FREE(member_list[i]);
}
ACK with "err" changed to "error".
1:56 p.m.
New subject: [libvirt] [PATCHv2 3/6] interface: Refactor interface vlan to helper func
Mechanical move to break up udevIfaceGetIfaceDef() into different
helpers for each of the interface types to hopefully make the code
easier to follow. This moves the vlan code to
udevIfaceGetIfaceDefVlan().
---
src/interface/interface_backend_udev.c | 73 +++++++++++++++++++++-------------
1 file changed, 46 insertions(+), 27 deletions(-)
diff --git a/src/interface/interface_backend_udev.c
b/src/interface/interface_backend_udev.c
index f5b44ea..e5ab1a7 100644
--- a/src/interface/interface_backend_udev.c
+++ b/src/interface/interface_backend_udev.c
@@ -654,6 +654,51 @@ err:
return -1;
}
+static int
+ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
+ATTRIBUTE_NONNULL(4) ATTRIBUTE_RETURN_CHECK
+udevIfaceGetIfaceDefVlan(struct udev *udev ATTRIBUTE_UNUSED,
+ struct udev_device *dev ATTRIBUTE_UNUSED,
+ const char *name,
+ virInterfaceDef *ifacedef)
+{
+ char *vid;
+ char *vlan_parent_dev = NULL;
+
+ vlan_parent_dev = strdup(name);
+ if (!vlan_parent_dev) {
+ virReportOOMError();
+ goto cleanup;
+ }
+
+ /* Find the DEVICE.VID again */
+ vid = strrchr(vlan_parent_dev, '.');
+ if (!vid) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("failed to find the VID for the VLAN device '%s'"),
+ name);
+ goto cleanup;
+ }
+
+ /* Replace the dot with a NULL so we can have the device and VID */
+ vid[0] = '\0';
+ vid++;
+
+ /* Set our type to VLAN */
+ ifacedef->type = VIR_INTERFACE_TYPE_VLAN;
+
+ /* Set the VLAN specifics */
+ ifacedef->data.vlan.tag = vid;
+ ifacedef->data.vlan.devname = vlan_parent_dev;
+
+ return 0;
+
+cleanup:
+ VIR_FREE(vlan_parent_dev);
+
+ return -1;
+}
+
static virInterfaceDef * ATTRIBUTE_NONNULL(1)
udevIfaceGetIfaceDef(struct udev *udev, const char *name)
{
@@ -712,34 +757,8 @@ udevIfaceGetIfaceDef(struct udev *udev, const char *name)
* other devices */
vlan_parent_dev = strrchr(name, '.');
if (vlan_parent_dev) {
- /* Found the VLAN dot */
- char *vid;
-
- vlan_parent_dev = strdup(name);
- if (!vlan_parent_dev) {
- virReportOOMError();
+ if (udevIfaceGetIfaceDefVlan(udev, dev, name, ifacedef))
goto cleanup;
- }
-
- /* Find the DEVICE.VID separator again */
- vid = strrchr(vlan_parent_dev, '.');
- if (!vid) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("failed to find the VID for the VLAN device
'%s'"),
- name);
- goto cleanup;
- }
-
- /* Replace the dot with a NULL so we can have the device and VID */
- vid[0] = '\0';
- vid++;
-
- /* Set our type to VLAN */
- ifacedef->type = VIR_INTERFACE_TYPE_VLAN;
-
- /* Set the VLAN specifics */
- ifacedef->data.vlan.tag = vid;
- ifacedef->data.vlan.devname = vlan_parent_dev;
} else if (STREQ_NULLABLE(udev_device_get_devtype(dev), "bridge")) {
if (udevIfaceGetIfaceDefBridge(udev, dev, name, ifacedef) < 0)
goto cleanup;
--
1.7.12.4
3:03 p.m.
New subject: [libvirt] [PATCHv2 3/6] interface: Refactor interface vlan to helper func
On 02/20/2013 02:56 PM, Doug Goldstein wrote:
Mechanical move to break up udevIfaceGetIfaceDef() into different
helpers for each of the interface types to hopefully make the code
easier to follow. This moves the vlan code to
udevIfaceGetIfaceDefVlan().
---
src/interface/interface_backend_udev.c | 73 +++++++++++++++++++++-------------
1 file changed, 46 insertions(+), 27 deletions(-)
diff --git a/src/interface/interface_backend_udev.c
b/src/interface/interface_backend_udev.c
index f5b44ea..e5ab1a7 100644
--- a/src/interface/interface_backend_udev.c
+++ b/src/interface/interface_backend_udev.c
@@ -654,6 +654,51 @@ err:
return -1;
}
+static int
+ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
+ATTRIBUTE_NONNULL(4) ATTRIBUTE_RETURN_CHECK
+udevIfaceGetIfaceDefVlan(struct udev *udev ATTRIBUTE_UNUSED,
+ struct udev_device *dev ATTRIBUTE_UNUSED,
+ const char *name,
+ virInterfaceDef *ifacedef)
+{
+ char *vid;
+ char *vlan_parent_dev = NULL;
+
+ vlan_parent_dev = strdup(name);
+ if (!vlan_parent_dev) {
+ virReportOOMError();
+ goto cleanup;
+ }
+
+ /* Find the DEVICE.VID again */
+ vid = strrchr(vlan_parent_dev, '.');
+ if (!vid) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("failed to find the VID for the VLAN device '%s'"),
+ name);
+ goto cleanup;
+ }
+
+ /* Replace the dot with a NULL so we can have the device and VID */
+ vid[0] = '\0';
+ vid++;
+
+ /* Set our type to VLAN */
+ ifacedef->type = VIR_INTERFACE_TYPE_VLAN;
+
+ /* Set the VLAN specifics */
+ ifacedef->data.vlan.tag = vid;
+ ifacedef->data.vlan.devname = vlan_parent_dev;
+
+ return 0;
+
+cleanup:
+ VIR_FREE(vlan_parent_dev);
+
+ return -1;
+}
+
static virInterfaceDef * ATTRIBUTE_NONNULL(1)
udevIfaceGetIfaceDef(struct udev *udev, const char *name)
{
@@ -712,34 +757,8 @@ udevIfaceGetIfaceDef(struct udev *udev, const char *name)
* other devices */
vlan_parent_dev = strrchr(name, '.');
if (vlan_parent_dev) {
- /* Found the VLAN dot */
- char *vid;
-
- vlan_parent_dev = strdup(name);
- if (!vlan_parent_dev) {
- virReportOOMError();
+ if (udevIfaceGetIfaceDefVlan(udev, dev, name, ifacedef))
goto cleanup;
- }
-
- /* Find the DEVICE.VID separator again */
- vid = strrchr(vlan_parent_dev, '.');
- if (!vid) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("failed to find the VID for the VLAN device
'%s'"),
- name);
- goto cleanup;
- }
-
- /* Replace the dot with a NULL so we can have the device and VID */
- vid[0] = '\0';
- vid++;
-
- /* Set our type to VLAN */
- ifacedef->type = VIR_INTERFACE_TYPE_VLAN;
-
- /* Set the VLAN specifics */
- ifacedef->data.vlan.tag = vid;
- ifacedef->data.vlan.devname = vlan_parent_dev;
} else if (STREQ_NULLABLE(udev_device_get_devtype(dev), "bridge")) {
if (udevIfaceGetIfaceDefBridge(udev, dev, name, ifacedef) < 0)
goto cleanup;
ACK.
1:56 p.m.
New subject: [libvirt] [PATCHv2 4/6] interface: Improve udev backend device type id
Refactored the interface device type identification to make it more
clear about the operations. Add support for udev devtype to detect
VLANs on Linux 3.7 and newer. Move VLAN detection based on device
name to fallback case.
---
src/interface/interface_backend_udev.c | 46 +++++++++++++++++++++++++---------
1 file changed, 34 insertions(+), 12 deletions(-)
diff --git a/src/interface/interface_backend_udev.c
b/src/interface/interface_backend_udev.c
index e5ab1a7..db4e7d1 100644
--- a/src/interface/interface_backend_udev.c
+++ b/src/interface/interface_backend_udev.c
@@ -684,9 +684,6 @@ udevIfaceGetIfaceDefVlan(struct udev *udev ATTRIBUTE_UNUSED,
vid[0] = '\0';
vid++;
- /* Set our type to VLAN */
- ifacedef->type = VIR_INTERFACE_TYPE_VLAN;
-
/* Set the VLAN specifics */
ifacedef->data.vlan.tag = vid;
ifacedef->data.vlan.devname = vlan_parent_dev;
@@ -707,6 +704,7 @@ udevIfaceGetIfaceDef(struct udev *udev, const char *name)
unsigned int mtu;
const char *mtu_str;
char *vlan_parent_dev = NULL;
+ const char *devtype;
/* Allocate our interface definition structure */
if (VIR_ALLOC(ifacedef) < 0) {
@@ -716,7 +714,6 @@ udevIfaceGetIfaceDef(struct udev *udev, const char *name)
/* Clear our structure and set safe defaults */
ifacedef->startmode = VIR_INTERFACE_START_UNSPECIFIED;
- ifacedef->type = VIR_INTERFACE_TYPE_ETHERNET;
ifacedef->name = strdup(name);
if (!ifacedef->name) {
@@ -753,18 +750,43 @@ udevIfaceGetIfaceDef(struct udev *udev, const char *name)
ifacedef->nprotos = 0;
ifacedef->protos = NULL;
- /* Check if its a VLAN since we can have a VLAN of any of the
- * other devices */
- vlan_parent_dev = strrchr(name, '.');
- if (vlan_parent_dev) {
+ /* Check the type of device we are working with based on the devtype */
+ devtype = udev_device_get_devtype(dev);
+
+ /* Set our type to ethernet as the default case */
+ ifacedef->type = VIR_INTERFACE_TYPE_ETHERNET;
+
+ if (STREQ_NULLABLE(devtype, "vlan")) {
+ /* This only works on modern kernels (3.7 and newer)
+ * e949b09b71d975a82f13ac88ce4ad338fed213da
+ */
+ ifacedef->type = VIR_INTERFACE_TYPE_VLAN;
+ } else if (STREQ_NULLABLE(devtype, "bridge")) {
+ ifacedef->type = VIR_INTERFACE_TYPE_BRIDGE;
+ }
+
+ /* Fallback checks if the devtype check didn't work. */
+ if (ifacedef->type == VIR_INTERFACE_TYPE_ETHERNET) {
+ /* First check if its a VLAN based on the name containing a dot,
+ * to prevent false positives
+ */
+ vlan_parent_dev = strrchr(name, '.');
+ if (vlan_parent_dev) {
+ ifacedef->type = VIR_INTERFACE_TYPE_VLAN;
+ }
+ }
+
+ switch (ifacedef->type) {
+ case VIR_INTERFACE_TYPE_VLAN:
if (udevIfaceGetIfaceDefVlan(udev, dev, name, ifacedef))
goto cleanup;
- } else if (STREQ_NULLABLE(udev_device_get_devtype(dev), "bridge")) {
+ break;
+ case VIR_INTERFACE_TYPE_BRIDGE:
if (udevIfaceGetIfaceDefBridge(udev, dev, name, ifacedef) < 0)
goto cleanup;
- } else {
- /* Set our type to ethernet */
- ifacedef->type = VIR_INTERFACE_TYPE_ETHERNET;
+ break;
+ case VIR_INTERFACE_TYPE_ETHERNET:
+ break;
}
udev_device_unref(dev);
--
1.7.12.4
3:05 p.m.
New subject: [libvirt] [PATCHv2 4/6] interface: Improve udev backend device type id
On 02/20/2013 02:56 PM, Doug Goldstein wrote:
Refactored the interface device type identification to make it more
clear about the operations. Add support for udev devtype to detect
VLANs on Linux 3.7 and newer. Move VLAN detection based on device
name to fallback case.
---
src/interface/interface_backend_udev.c | 46 +++++++++++++++++++++++++---------
1 file changed, 34 insertions(+), 12 deletions(-)
diff --git a/src/interface/interface_backend_udev.c
b/src/interface/interface_backend_udev.c
index e5ab1a7..db4e7d1 100644
--- a/src/interface/interface_backend_udev.c
+++ b/src/interface/interface_backend_udev.c
@@ -684,9 +684,6 @@ udevIfaceGetIfaceDefVlan(struct udev *udev ATTRIBUTE_UNUSED,
vid[0] = '\0';
vid++;
- /* Set our type to VLAN */
- ifacedef->type = VIR_INTERFACE_TYPE_VLAN;
-
/* Set the VLAN specifics */
ifacedef->data.vlan.tag = vid;
ifacedef->data.vlan.devname = vlan_parent_dev;
@@ -707,6 +704,7 @@ udevIfaceGetIfaceDef(struct udev *udev, const char *name)
unsigned int mtu;
const char *mtu_str;
char *vlan_parent_dev = NULL;
+ const char *devtype;
/* Allocate our interface definition structure */
if (VIR_ALLOC(ifacedef) < 0) {
@@ -716,7 +714,6 @@ udevIfaceGetIfaceDef(struct udev *udev, const char *name)
/* Clear our structure and set safe defaults */
ifacedef->startmode = VIR_INTERFACE_START_UNSPECIFIED;
- ifacedef->type = VIR_INTERFACE_TYPE_ETHERNET;
ifacedef->name = strdup(name);
if (!ifacedef->name) {
@@ -753,18 +750,43 @@ udevIfaceGetIfaceDef(struct udev *udev, const char *name)
ifacedef->nprotos = 0;
ifacedef->protos = NULL;
- /* Check if its a VLAN since we can have a VLAN of any of the
- * other devices */
- vlan_parent_dev = strrchr(name, '.');
- if (vlan_parent_dev) {
+ /* Check the type of device we are working with based on the devtype */
+ devtype = udev_device_get_devtype(dev);
+
+ /* Set our type to ethernet as the default case */
+ ifacedef->type = VIR_INTERFACE_TYPE_ETHERNET;
+
+ if (STREQ_NULLABLE(devtype, "vlan")) {
+ /* This only works on modern kernels (3.7 and newer)
+ * e949b09b71d975a82f13ac88ce4ad338fed213da
+ */
+ ifacedef->type = VIR_INTERFACE_TYPE_VLAN;
+ } else if (STREQ_NULLABLE(devtype, "bridge")) {
+ ifacedef->type = VIR_INTERFACE_TYPE_BRIDGE;
+ }
+
+ /* Fallback checks if the devtype check didn't work. */
+ if (ifacedef->type == VIR_INTERFACE_TYPE_ETHERNET) {
+ /* First check if its a VLAN based on the name containing a dot,
+ * to prevent false positives
+ */
+ vlan_parent_dev = strrchr(name, '.');
+ if (vlan_parent_dev) {
+ ifacedef->type = VIR_INTERFACE_TYPE_VLAN;
+ }
+ }
+
+ switch (ifacedef->type) {
+ case VIR_INTERFACE_TYPE_VLAN:
if (udevIfaceGetIfaceDefVlan(udev, dev, name, ifacedef))
goto cleanup;
- } else if (STREQ_NULLABLE(udev_device_get_devtype(dev), "bridge")) {
+ break;
+ case VIR_INTERFACE_TYPE_BRIDGE:
if (udevIfaceGetIfaceDefBridge(udev, dev, name, ifacedef) < 0)
goto cleanup;
- } else {
- /* Set our type to ethernet */
- ifacedef->type = VIR_INTERFACE_TYPE_ETHERNET;
+ break;
+ case VIR_INTERFACE_TYPE_ETHERNET:
+ break;
}
udev_device_unref(dev);
ACK.
1:56 p.m.
New subject: [libvirt] [PATCHv2 5/6] interface: add bond support to udev backend
The udev backend now supports bond interfaces.
---
src/interface/interface_backend_udev.c | 264 ++++++++++++++++++++++++++++++++-
1 file changed, 263 insertions(+), 1 deletion(-)
diff --git a/src/interface/interface_backend_udev.c
b/src/interface/interface_backend_udev.c
index db4e7d1..9f1570c 100644
--- a/src/interface/interface_backend_udev.c
+++ b/src/interface/interface_backend_udev.c
@@ -494,6 +494,27 @@ err:
}
/**
+ * Helper function for finding bond slaves using scandir()
+ *
+ * @param entry - directory entry passed by scandir()
+ *
+ * @return 1 if we want to add it to scandir's list, 0 if not.
+ */
+static int
+udevIfaceBondScanDirFilter(const struct dirent *entry)
+{
+ /* This is ugly so if anyone has a better suggestion, please improve
+ * this. Unfortunately the kernel stores everything in the top level
+ * interface sysfs entry and references the slaves as slave_eth0 for
+ * example.
+ */
+ if (STRPREFIX(entry->d_name, "slave_"))
+ return 1;
+
+ return 0;
+}
+
+/**
* Helper function for finding bridge members using scandir()
*
* @param entry - directory entry passed by scandir()
@@ -522,6 +543,14 @@ udevIfaceFreeIfaceDef(virInterfaceDef *ifacedef)
if (!ifacedef)
return;
+ if (ifacedef->type == VIR_INTERFACE_TYPE_BOND) {
+ VIR_FREE(ifacedef->data.bond.target);
+ for (i = 0; i < ifacedef->data.bond.nbItf; i++) {
+ udevIfaceFreeIfaceDef(ifacedef->data.bond.itf[i]);
+ }
+ VIR_FREE(ifacedef->data.bond.itf);
+ }
+
if (ifacedef->type == VIR_INTERFACE_TYPE_BRIDGE) {
VIR_FREE(ifacedef->data.bridge.delay);
for (i = 0; i < ifacedef->data.bridge.nbItf; i++) {
@@ -543,6 +572,230 @@ udevIfaceFreeIfaceDef(virInterfaceDef *ifacedef)
static int
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
ATTRIBUTE_NONNULL(4) ATTRIBUTE_RETURN_CHECK
+udevIfaceGetIfaceDefBond(struct udev *udev,
+ struct udev_device *dev,
+ const char *name,
+ virInterfaceDef *ifacedef)
+{
+ struct dirent **slave_list = NULL;
+ int slave_count = 0;
+ int i;
+ const char *tmp_str;
+ int tmp_int;
+
+ /* Initial defaults */
+ ifacedef->data.bond.target = NULL;
+ ifacedef->data.bond.nbItf = 0;
+ ifacedef->data.bond.itf = NULL;
+
+ /* Set the bond specifics */
+ tmp_str = udev_device_get_sysattr_value(dev, "bonding/downdelay");
+ if (!tmp_str) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not retrieve 'bonding/downdelay' for
'%s'"), name);
+ goto cleanup;
+ }
+ if (virStrToLong_i(tmp_str, NULL, 10, &tmp_int) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not parse 'bonding/downdelay' '%s' for
'%s'"),
+ tmp_str, name);
+ goto cleanup;
+ }
+ ifacedef->data.bond.downdelay = tmp_int;
+
+ tmp_str = udev_device_get_sysattr_value(dev, "bonding/updelay");
+ if (!tmp_str) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not retrieve 'bonding/updelay' for
'%s'"), name);
+ goto cleanup;
+ }
+ if (virStrToLong_i(tmp_str, NULL, 10, &tmp_int) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not parse 'bonding/updelay' '%s' for
'%s'"),
+ tmp_str, name);
+ goto cleanup;
+ }
+ ifacedef->data.bond.updelay = tmp_int;
+
+ tmp_str = udev_device_get_sysattr_value(dev, "bonding/miimon");
+ if (!tmp_str) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not retrieve 'bonding/miimon' for
'%s'"), name);
+ goto cleanup;
+ }
+ if (virStrToLong_i(tmp_str, NULL, 10, &tmp_int) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not parse 'bonding/miimon' '%s' for
'%s'"),
+ tmp_str, name);
+ goto cleanup;
+ }
+ ifacedef->data.bond.frequency = tmp_int;
+
+ tmp_str = udev_device_get_sysattr_value(dev, "bonding/arp_interval");
+ if (!tmp_str) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not retrieve 'bonding/arp_interval' for
'%s'"), name);
+ goto cleanup;
+ }
+ if (virStrToLong_i(tmp_str, NULL, 10, &tmp_int) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not parse 'bonding/arp_interval' '%s' for
'%s'"),
+ tmp_str, name);
+ goto cleanup;
+ }
+ ifacedef->data.bond.interval = tmp_int;
+
+ /* bonding/mode is in the format: "balance-rr 0" so we find the
+ * space and increment the pointer to get the number and convert
+ * it to an interger. libvirt uses 1 through 7 while the raw
+ * number is 0 through 6 so increment it by 1.
+ */
+ tmp_str = udev_device_get_sysattr_value(dev, "bonding/mode");
+ if (!tmp_str) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not retrieve 'bonding/mode' for
'%s'"), name);
+ goto cleanup;
+ }
+ tmp_str = strchr(tmp_str, ' ');
+ if (!tmp_str) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Invalid format for 'bonding/mode' for
'%s'"), name);
+ goto cleanup;
+ }
+ if (strlen(tmp_str) < 2) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Unable to find correct value in 'bonding/mode' for
'%s'"),
+ name);
+ goto cleanup;
+ }
+ if (virStrToLong_i(tmp_str + 1, NULL, 10, &tmp_int) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not parse 'bonding/mode' '%s' for
'%s'"),
+ tmp_str, name);
+ goto cleanup;
+ }
+ ifacedef->data.bond.mode = tmp_int + 1;
+
+ /* bonding/arp_validate is in the format: "none 0" so we find the
+ * space and increment the pointer to get the number and convert
+ * it to an interger.
+ */
+ tmp_str = udev_device_get_sysattr_value(dev, "bonding/arp_validate");
+ if (!tmp_str) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not retrieve 'bonding/arp_validate' for
'%s'"), name);
+ goto cleanup;
+ }
+ tmp_str = strchr(tmp_str, ' ');
+ if (!tmp_str) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Invalid format for 'bonding/arp_validate' for
'%s'"), name);
+ goto cleanup;
+ }
+ if (strlen(tmp_str) < 2) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Unable to find correct value in 'bonding/arp_validate'
"
+ "for '%s'"), name);
+ goto cleanup;
+ }
+ if (virStrToLong_i(tmp_str + 1, NULL, 10, &tmp_int) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not parse 'bonding/arp_validate' '%s' for
'%s'"),
+ tmp_str, name);
+ goto cleanup;
+ }
+ ifacedef->data.bond.validate = tmp_int;
+
+ /* bonding/use_carrier is 0 or 1 and libvirt stores it as 1 or 2. */
+ tmp_str = udev_device_get_sysattr_value(dev, "bonding/use_carrier");
+ if (!tmp_str) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not retrieve 'bonding/use_carrier' for
'%s'"), name);
+ goto cleanup;
+ }
+ if (virStrToLong_i(tmp_str, NULL, 10, &tmp_int) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not parse 'bonding/use_carrier' '%s' for
'%s'"),
+ tmp_str, name);
+ goto cleanup;
+ }
+ ifacedef->data.bond.carrier = tmp_int + 1;
+
+ /* MII or ARP Monitoring is based on arp_interval and miimon.
+ * if arp_interval > 0 then ARP monitoring is in play, if
+ * miimon > 0 then MII monitoring is in play.
+ */
+ if (ifacedef->data.bond.interval > 0)
+ ifacedef->data.bond.monit = VIR_INTERFACE_BOND_MONIT_ARP;
+ else if (ifacedef->data.bond.frequency > 0)
+ ifacedef->data.bond.monit = VIR_INTERFACE_BOND_MONIT_MII;
+ else
+ ifacedef->data.bond.monit = VIR_INTERFACE_BOND_MONIT_NONE;
+
+ tmp_str = udev_device_get_sysattr_value(dev, "bonding/arp_ip_target");
+ if (!tmp_str) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not retrieve 'bonding/arp_ip_target' for
'%s'"), name);
+ goto cleanup;
+ }
+ ifacedef->data.bond.target = strdup(tmp_str);
+ if (!ifacedef->data.bond.target) {
+ virReportOOMError();
+ goto cleanup;
+ }
+
+ /* Slaves of the bond */
+ /* Get each slave in the bond */
+ slave_count = scandir(udev_device_get_syspath(dev), &slave_list,
+ udevIfaceBondScanDirFilter, alphasort);
+
+ if (slave_count < 0) {
+ virReportSystemError(errno,
+ _("Could not get slaves of bond '%s'"), name);
+ goto cleanup;
+ }
+
+ /* Allocate our list of slave devices */
+ if (VIR_ALLOC_N(ifacedef->data.bond.itf, slave_count) < 0) {
+ virReportOOMError();
+ goto cleanup;
+ }
+ ifacedef->data.bond.nbItf = slave_count;
+
+ for (i = 0; i < slave_count; i++) {
+ /* Names are slave_interface. e.g. slave_eth0
+ * so we use the part after the _
+ */
+ tmp_str = strchr(slave_list[i]->d_name, '_');
+ tmp_str++;
+
+ ifacedef->data.bond.itf[i] =
+ udevIfaceGetIfaceDef(udev, tmp_str);
+ if (!ifacedef->data.bond.itf[i]) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not get interface information for '%s', which is
"
+ "a enslaved in bond '%s'"), slave_list[i]->d_name,
name);
+ goto cleanup;
+ }
+ VIR_FREE(slave_list[i]);
+ }
+
+ VIR_FREE(slave_list);
+
+ return 0;
+
+cleanup:
+ for (i = 0; i < slave_count; i++) {
+ VIR_FREE(slave_list[i]);
+ }
+ VIR_FREE(slave_list);
+
+ return -1;
+}
+
+static int
+ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
+ATTRIBUTE_NONNULL(4) ATTRIBUTE_RETURN_CHECK
udevIfaceGetIfaceDefBridge(struct udev *udev,
struct udev_device *dev,
const char *name,
@@ -774,17 +1027,26 @@ udevIfaceGetIfaceDef(struct udev *udev, const char *name)
if (vlan_parent_dev) {
ifacedef->type = VIR_INTERFACE_TYPE_VLAN;
}
+
+ /* Fallback check to see if this is a bond device */
+ if (udev_device_get_sysattr_value(dev, "bonding/mode")) {
+ ifacedef->type = VIR_INTERFACE_TYPE_BOND;
+ }
}
switch (ifacedef->type) {
case VIR_INTERFACE_TYPE_VLAN:
- if (udevIfaceGetIfaceDefVlan(udev, dev, name, ifacedef))
+ if (udevIfaceGetIfaceDefVlan(udev, dev, name, ifacedef) < 0)
goto cleanup;
break;
case VIR_INTERFACE_TYPE_BRIDGE:
if (udevIfaceGetIfaceDefBridge(udev, dev, name, ifacedef) < 0)
goto cleanup;
break;
+ case VIR_INTERFACE_TYPE_BOND:
+ if (udevIfaceGetIfaceDefBond(udev, dev, name, ifacedef) < 0)
+ goto cleanup;
+ break;
case VIR_INTERFACE_TYPE_ETHERNET:
break;
}
--
1.7.12.4
3:07 p.m.
New subject: [libvirt] [PATCHv2 5/6] interface: add bond support to udev backend
On 02/20/2013 02:56 PM, Doug Goldstein wrote:
The udev backend now supports bond interfaces.
---
src/interface/interface_backend_udev.c | 264 ++++++++++++++++++++++++++++++++-
1 file changed, 263 insertions(+), 1 deletion(-)
diff --git a/src/interface/interface_backend_udev.c
b/src/interface/interface_backend_udev.c
index db4e7d1..9f1570c 100644
--- a/src/interface/interface_backend_udev.c
+++ b/src/interface/interface_backend_udev.c
@@ -494,6 +494,27 @@ err:
}
/**
+ * Helper function for finding bond slaves using scandir()
+ *
+ * @param entry - directory entry passed by scandir()
+ *
+ * @return 1 if we want to add it to scandir's list, 0 if not.
+ */
+static int
+udevIfaceBondScanDirFilter(const struct dirent *entry)
+{
+ /* This is ugly so if anyone has a better suggestion, please improve
+ * this. Unfortunately the kernel stores everything in the top level
+ * interface sysfs entry and references the slaves as slave_eth0 for
+ * example.
+ */
+ if (STRPREFIX(entry->d_name, "slave_"))
+ return 1;
+
+ return 0;
+}
+
+/**
* Helper function for finding bridge members using scandir()
*
* @param entry - directory entry passed by scandir()
@@ -522,6 +543,14 @@ udevIfaceFreeIfaceDef(virInterfaceDef *ifacedef)
if (!ifacedef)
return;
+ if (ifacedef->type == VIR_INTERFACE_TYPE_BOND) {
+ VIR_FREE(ifacedef->data.bond.target);
+ for (i = 0; i < ifacedef->data.bond.nbItf; i++) {
+ udevIfaceFreeIfaceDef(ifacedef->data.bond.itf[i]);
+ }
+ VIR_FREE(ifacedef->data.bond.itf);
+ }
+
if (ifacedef->type == VIR_INTERFACE_TYPE_BRIDGE) {
VIR_FREE(ifacedef->data.bridge.delay);
for (i = 0; i < ifacedef->data.bridge.nbItf; i++) {
@@ -543,6 +572,230 @@ udevIfaceFreeIfaceDef(virInterfaceDef *ifacedef)
static int
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
ATTRIBUTE_NONNULL(4) ATTRIBUTE_RETURN_CHECK
+udevIfaceGetIfaceDefBond(struct udev *udev,
+ struct udev_device *dev,
+ const char *name,
+ virInterfaceDef *ifacedef)
+{
+ struct dirent **slave_list = NULL;
+ int slave_count = 0;
+ int i;
+ const char *tmp_str;
+ int tmp_int;
+
+ /* Initial defaults */
+ ifacedef->data.bond.target = NULL;
+ ifacedef->data.bond.nbItf = 0;
+ ifacedef->data.bond.itf = NULL;
+
+ /* Set the bond specifics */
+ tmp_str = udev_device_get_sysattr_value(dev, "bonding/downdelay");
+ if (!tmp_str) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not retrieve 'bonding/downdelay' for
'%s'"), name);
+ goto cleanup;
+ }
+ if (virStrToLong_i(tmp_str, NULL, 10, &tmp_int) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not parse 'bonding/downdelay' '%s' for
'%s'"),
+ tmp_str, name);
+ goto cleanup;
+ }
+ ifacedef->data.bond.downdelay = tmp_int;
+
+ tmp_str = udev_device_get_sysattr_value(dev, "bonding/updelay");
+ if (!tmp_str) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not retrieve 'bonding/updelay' for
'%s'"), name);
+ goto cleanup;
+ }
+ if (virStrToLong_i(tmp_str, NULL, 10, &tmp_int) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not parse 'bonding/updelay' '%s' for
'%s'"),
+ tmp_str, name);
+ goto cleanup;
+ }
+ ifacedef->data.bond.updelay = tmp_int;
+
+ tmp_str = udev_device_get_sysattr_value(dev, "bonding/miimon");
+ if (!tmp_str) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not retrieve 'bonding/miimon' for
'%s'"), name);
+ goto cleanup;
+ }
+ if (virStrToLong_i(tmp_str, NULL, 10, &tmp_int) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not parse 'bonding/miimon' '%s' for
'%s'"),
+ tmp_str, name);
+ goto cleanup;
+ }
+ ifacedef->data.bond.frequency = tmp_int;
+
+ tmp_str = udev_device_get_sysattr_value(dev, "bonding/arp_interval");
+ if (!tmp_str) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not retrieve 'bonding/arp_interval' for
'%s'"), name);
+ goto cleanup;
+ }
+ if (virStrToLong_i(tmp_str, NULL, 10, &tmp_int) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not parse 'bonding/arp_interval' '%s' for
'%s'"),
+ tmp_str, name);
+ goto cleanup;
+ }
+ ifacedef->data.bond.interval = tmp_int;
+
+ /* bonding/mode is in the format: "balance-rr 0" so we find the
+ * space and increment the pointer to get the number and convert
+ * it to an interger. libvirt uses 1 through 7 while the raw
+ * number is 0 through 6 so increment it by 1.
+ */
+ tmp_str = udev_device_get_sysattr_value(dev, "bonding/mode");
+ if (!tmp_str) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not retrieve 'bonding/mode' for
'%s'"), name);
+ goto cleanup;
+ }
+ tmp_str = strchr(tmp_str, ' ');
+ if (!tmp_str) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Invalid format for 'bonding/mode' for
'%s'"), name);
+ goto cleanup;
+ }
+ if (strlen(tmp_str) < 2) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Unable to find correct value in 'bonding/mode' for
'%s'"),
+ name);
+ goto cleanup;
+ }
+ if (virStrToLong_i(tmp_str + 1, NULL, 10, &tmp_int) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not parse 'bonding/mode' '%s' for
'%s'"),
+ tmp_str, name);
+ goto cleanup;
+ }
+ ifacedef->data.bond.mode = tmp_int + 1;
+
+ /* bonding/arp_validate is in the format: "none 0" so we find the
+ * space and increment the pointer to get the number and convert
+ * it to an interger.
+ */
+ tmp_str = udev_device_get_sysattr_value(dev, "bonding/arp_validate");
+ if (!tmp_str) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not retrieve 'bonding/arp_validate' for
'%s'"), name);
+ goto cleanup;
+ }
+ tmp_str = strchr(tmp_str, ' ');
+ if (!tmp_str) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Invalid format for 'bonding/arp_validate' for
'%s'"), name);
+ goto cleanup;
+ }
+ if (strlen(tmp_str) < 2) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Unable to find correct value in 'bonding/arp_validate'
"
+ "for '%s'"), name);
+ goto cleanup;
+ }
+ if (virStrToLong_i(tmp_str + 1, NULL, 10, &tmp_int) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not parse 'bonding/arp_validate' '%s' for
'%s'"),
+ tmp_str, name);
+ goto cleanup;
+ }
+ ifacedef->data.bond.validate = tmp_int;
+
+ /* bonding/use_carrier is 0 or 1 and libvirt stores it as 1 or 2. */
+ tmp_str = udev_device_get_sysattr_value(dev, "bonding/use_carrier");
+ if (!tmp_str) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not retrieve 'bonding/use_carrier' for
'%s'"), name);
+ goto cleanup;
+ }
+ if (virStrToLong_i(tmp_str, NULL, 10, &tmp_int) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not parse 'bonding/use_carrier' '%s' for
'%s'"),
+ tmp_str, name);
+ goto cleanup;
+ }
+ ifacedef->data.bond.carrier = tmp_int + 1;
+
+ /* MII or ARP Monitoring is based on arp_interval and miimon.
+ * if arp_interval > 0 then ARP monitoring is in play, if
+ * miimon > 0 then MII monitoring is in play.
+ */
+ if (ifacedef->data.bond.interval > 0)
+ ifacedef->data.bond.monit = VIR_INTERFACE_BOND_MONIT_ARP;
+ else if (ifacedef->data.bond.frequency > 0)
+ ifacedef->data.bond.monit = VIR_INTERFACE_BOND_MONIT_MII;
+ else
+ ifacedef->data.bond.monit = VIR_INTERFACE_BOND_MONIT_NONE;
+
+ tmp_str = udev_device_get_sysattr_value(dev, "bonding/arp_ip_target");
+ if (!tmp_str) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not retrieve 'bonding/arp_ip_target' for
'%s'"), name);
+ goto cleanup;
+ }
+ ifacedef->data.bond.target = strdup(tmp_str);
+ if (!ifacedef->data.bond.target) {
+ virReportOOMError();
+ goto cleanup;
+ }
+
+ /* Slaves of the bond */
+ /* Get each slave in the bond */
+ slave_count = scandir(udev_device_get_syspath(dev), &slave_list,
+ udevIfaceBondScanDirFilter, alphasort);
+
+ if (slave_count < 0) {
+ virReportSystemError(errno,
+ _("Could not get slaves of bond '%s'"), name);
+ goto cleanup;
+ }
+
+ /* Allocate our list of slave devices */
+ if (VIR_ALLOC_N(ifacedef->data.bond.itf, slave_count) < 0) {
+ virReportOOMError();
+ goto cleanup;
+ }
+ ifacedef->data.bond.nbItf = slave_count;
+
+ for (i = 0; i < slave_count; i++) {
+ /* Names are slave_interface. e.g. slave_eth0
+ * so we use the part after the _
+ */
+ tmp_str = strchr(slave_list[i]->d_name, '_');
+ tmp_str++;
+
+ ifacedef->data.bond.itf[i] =
+ udevIfaceGetIfaceDef(udev, tmp_str);
+ if (!ifacedef->data.bond.itf[i]) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not get interface information for '%s', which is
"
+ "a enslaved in bond '%s'"),
slave_list[i]->d_name, name);
+ goto cleanup;
+ }
+ VIR_FREE(slave_list[i]);
+ }
+
+ VIR_FREE(slave_list);
+
+ return 0;
+
+cleanup:
+ for (i = 0; i < slave_count; i++) {
+ VIR_FREE(slave_list[i]);
+ }
+ VIR_FREE(slave_list);
+
+ return -1;
+}
+
+static int
+ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
+ATTRIBUTE_NONNULL(4) ATTRIBUTE_RETURN_CHECK
udevIfaceGetIfaceDefBridge(struct udev *udev,
struct udev_device *dev,
const char *name,
@@ -774,17 +1027,26 @@ udevIfaceGetIfaceDef(struct udev *udev, const char *name)
if (vlan_parent_dev) {
ifacedef->type = VIR_INTERFACE_TYPE_VLAN;
}
+
+ /* Fallback check to see if this is a bond device */
+ if (udev_device_get_sysattr_value(dev, "bonding/mode")) {
+ ifacedef->type = VIR_INTERFACE_TYPE_BOND;
+ }
}
switch (ifacedef->type) {
case VIR_INTERFACE_TYPE_VLAN:
- if (udevIfaceGetIfaceDefVlan(udev, dev, name, ifacedef))
+ if (udevIfaceGetIfaceDefVlan(udev, dev, name, ifacedef) < 0)
goto cleanup;
break;
case VIR_INTERFACE_TYPE_BRIDGE:
if (udevIfaceGetIfaceDefBridge(udev, dev, name, ifacedef) < 0)
goto cleanup;
break;
+ case VIR_INTERFACE_TYPE_BOND:
+ if (udevIfaceGetIfaceDefBond(udev, dev, name, ifacedef) < 0)
+ goto cleanup;
+ break;
case VIR_INTERFACE_TYPE_ETHERNET:
break;
}
ACK.
1:56 p.m.
New subject: [libvirt] [PATCHv2 6/6] interface: dev type support for bond interfaces
Patch has been accepted into net-next's 3.9 queue to correctly expose
bond interfaces with the 'bond' devtype.
---
I'd consider this patch optional until the fix lands in Linus' tree.
---
src/interface/interface_backend_udev.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/src/interface/interface_backend_udev.c
b/src/interface/interface_backend_udev.c
index 9f1570c..70c4508 100644
--- a/src/interface/interface_backend_udev.c
+++ b/src/interface/interface_backend_udev.c
@@ -1016,6 +1016,9 @@ udevIfaceGetIfaceDef(struct udev *udev, const char *name)
ifacedef->type = VIR_INTERFACE_TYPE_VLAN;
} else if (STREQ_NULLABLE(devtype, "bridge")) {
ifacedef->type = VIR_INTERFACE_TYPE_BRIDGE;
+ } else if (STREQ_NULLABLE(devtype, "bond")) {
+ /* This only works on modern kernels (3.9 and newer) */
+ ifacedef->type = VIR_INTERFACE_TYPE_BOND;
}
/* Fallback checks if the devtype check didn't work. */
--
1.7.12.4
2:43 p.m.
New subject: [libvirt] [PATCHv2 6/6] interface: dev type support for bond interfaces
On 02/20/2013 02:56 PM, Doug Goldstein wrote:
Patch has been accepted into net-next's 3.9 queue to correctly
expose
bond interfaces with the 'bond' devtype.
---
I'd consider this patch optional until the fix lands in Linus' tree.
Optional but innocuous. ACK.
---
src/interface/interface_backend_udev.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/src/interface/interface_backend_udev.c
b/src/interface/interface_backend_udev.c
index 9f1570c..70c4508 100644
--- a/src/interface/interface_backend_udev.c
+++ b/src/interface/interface_backend_udev.c
@@ -1016,6 +1016,9 @@ udevIfaceGetIfaceDef(struct udev *udev, const char *name)
ifacedef->type = VIR_INTERFACE_TYPE_VLAN;
} else if (STREQ_NULLABLE(devtype, "bridge")) {
ifacedef->type = VIR_INTERFACE_TYPE_BRIDGE;
+ } else if (STREQ_NULLABLE(devtype, "bond")) {
+ /* This only works on modern kernels (3.9 and newer) */
+ ifacedef->type = VIR_INTERFACE_TYPE_BOND;
}
/* Fallback checks if the devtype check didn't work. */
6:40 p.m.
On Wed, Feb 20, 2013 at 1:56 PM, Doug Goldstein <cardoe(a)cardoe.com> wrote:
Refactor code, clean up error handling, and finally add bond support.
The
last patch optionally supports a patch I submitted to the Linux kernel
which should go in for 3.9 (it was just accepted for net-next).
After this patch when you have a bond device you'll get the following:
$ ./tools/virsh iface-dumpxml br0
<interface type='bridge' name='br0'>
<mtu size='1500'/>
<bridge stp='on' delay='1499'>
<interface type='bond' name='bond0'>
<mtu size='1500'/>
<bond mode='balance-rr'>
<interface type='ethernet' name='eth2'>
<mac address='d0:67:e5:fa:88:95'/>
<mtu size='1500'/>
</interface>
<interface type='ethernet' name='eth3'>
<mac address='d0:67:e5:fa:88:95'/>
<mtu size='1500'/>
</interface>
</bond>
</interface>
<!-- incorrectly including guest tap devices, but was an issue
prior and will be fixed in a later series -->
</bridge>
</interface>
Doug Goldstein (6):
interface: Refactor udev bridge to helper func
interface: udev bridge code error handling updates
interface: Refactor interface vlan to helper func
interface: Improve udev backend device type id
interface: add bond support to udev backend
interface: dev type support for bond interfaces
src/interface/interface_backend_udev.c | 546 +++++++++++++++++++++++++++------
1 file changed, 451 insertions(+), 95 deletions(-)
--
1.7.12.4
Applied this series after review and feedback from Laine. Thanks.
--
Doug Goldstein
4291
days inactive
4293
days old
13 comments
2 participants
participants (2)
-
Doug Goldstein -
Laine Stump