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.

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

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.

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.

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.

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. */