On 02/17/2013 08: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().

Before I noticed that this was all just code movement, I looked at the incoming code and found some issues. Since this patch is code movement, ACK to it anyway, but I think the issues I point out should be taken care of in a separate patch. (Oh, but you should remove the extra declaration of udevIfaceGetIfaceDefBridge , and combine its ATTRIBUTEs with the definition.)

--- src/interface/interface_backend_udev.c | 161 +++++++++++++++++++-------------- 1 file changed, 92 insertions(+), 69 deletions(-)

diff --git a/src/interface/interface_backend_udev.c b/src/interface/interface_backend_udev.c index 92c35d9..abd9895 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,101 @@ udevIfaceFreeIfaceDef(virInterfaceDef *ifacedef) VIR_FREE(ifacedef); }

+static int +udevIfaceGetIfaceDefBridge(struct udev *udev, + struct udev_device *dev, + const char *name, + virInterfaceDef *ifacedef) +ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) + ATTRIBUTE_NONNULL(4) ATTRIBUTE_RETURN_CHECK; + +static int +udevIfaceGetIfaceDefBridge(struct udev *udev, + struct udev_device *dev, + const char *name, + virInterfaceDef *ifacedef)

Instead of a separate declaration, why not just embed the ATTRIBUTE macros in the definition: static int ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2_ ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4) ATTRIBUTE_RETURN_CHECK udevIfaceGetIfaceDefBridge(struct udev *udev, .....)

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

Is it possible that the above udev function could ever legitimately return NULL? If so, you could potentially be treating a "no forward_delay value given" return as an OOM error.

+ if (!ifacedef->data.bridge.delay) { + virReportOOMError(); + goto cleanup; + } + + stp_str = udev_device_get_sysattr_value(dev, "bridge/stp_state");

Likewise, could that ever return NULL in anything other than an error condition?

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

I doubt it would ever make any difference in practice, but stp should always be set to 0, 1, or -1. If it was set to some other non-0 value, it wouldn't be included in the output of the format function.

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

This is going to end up including all of the guest tap devices, isn't it? That could make it difficult for virt-manager to determine which physdev to list in its dropdown menu. Perhaps there's some non-yucky way to eliminate libvirt's tap devices? (the danger here would be that you could possibly eliminate some *other* tap device that *was* supposed to be listed, for example the tap used for a VPN or something).

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

You should check for NULL return here log an error if it happens.

+ 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 +714,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(); - 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(); + if (udevIfaceGetIfaceDefBridge(udev, dev, name, ifacedef)) 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 +727,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);

On 02/17/2013 08: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 | 81 ++++++++++++++++++++++------------ 1 file changed, 54 insertions(+), 27 deletions(-)

diff --git a/src/interface/interface_backend_udev.c b/src/interface/interface_backend_udev.c index abd9895..c144978 100644 --- a/src/interface/interface_backend_udev.c +++ b/src/interface/interface_backend_udev.c @@ -627,6 +627,59 @@ cleanup: return -1; }

+static int +udevIfaceGetIfaceDefVlan(struct udev *udev ATTRIBUTE_UNUSED, + struct udev_device *dev ATTRIBUTE_UNUSED, + const char *name, + virInterfaceDef *ifacedef) + ATTRIBUTE_NONNULL(1) + ATTRIBUTE_NONNULL(2) + ATTRIBUTE_NONNULL(3) + ATTRIBUTE_NONNULL(4) + ATTRIBUTE_RETURN_CHECK;

Same comment applies here as in bridge case, about combining the forward declaration with the definition, and putting the ATTRIBUTES directly in the definition. ACK with that change.

+static int +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) { @@ -685,34 +738,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(); - 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); + if (udevIfaceGetIfaceDefVlan(udev, dev, name, ifacedef)) 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)) goto cleanup;

On 02/17/2013 08:56 PM, Doug Goldstein wrote:

Refactored the interface device type identification to make it more clear about the operations. --- src/interface/interface_backend_udev.c | 43 ++++++++++++++++++++++++---------- 1 file changed, 30 insertions(+), 13 deletions(-)

diff --git a/src/interface/interface_backend_udev.c b/src/interface/interface_backend_udev.c index c144978..73494a6 100644 --- a/src/interface/interface_backend_udev.c +++ b/src/interface/interface_backend_udev.c @@ -561,9 +561,6 @@ udevIfaceGetIfaceDefBridge(struct udev *udev, 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")); @@ -665,9 +662,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; @@ -688,6 +682,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) { @@ -697,7 +692,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) { @@ -734,18 +728,41 @@ 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 */ + /* 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, 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; + }

So on kernels prior to 3.7, the devtype would still be, e.g. "ethernet", but the "." in the name would just imply that it's a vlan device? ACK if my understanding is correct.

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

On 02/17/2013 08:56 PM, Doug Goldstein wrote:

The udev backend now supports bond interfaces. --- The result of an iface-dumpxml bond0 is as follows:

<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> --- src/interface/interface_backend_udev.c | 195 +++++++++++++++++++++++++++++++++ 1 file changed, 195 insertions(+)

diff --git a/src/interface/interface_backend_udev.c b/src/interface/interface_backend_udev.c index 73494a6..bba02d1 100644 --- a/src/interface/interface_backend_udev.c +++ b/src/interface/interface_backend_udev.c @@ -494,6 +494,22 @@ 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) +{ + if (STRPREFIX(entry->d_name, "slave_")) + return 1;

Urg. That *feels* kind of ugly, but if that's what it takes, then that's what it takes...

+ + return 0; +} + +/** * Helper function for finding bridge members using scandir() * * @param entry - directory entry passed by scandir() @@ -522,6 +538,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++) { @@ -541,6 +565,168 @@ udevIfaceFreeIfaceDef(virInterfaceDef *ifacedef) }

static int +udevIfaceGetIfaceDefBond(struct udev *udev ATTRIBUTE_UNUSED, + struct udev_device *dev ATTRIBUTE_UNUSED, + const char *name, + virInterfaceDef *ifacedef) + ATTRIBUTE_NONNULL(1) + ATTRIBUTE_NONNULL(2) + ATTRIBUTE_NONNULL(3) + ATTRIBUTE_NONNULL(4) + ATTRIBUTE_RETURN_CHECK; +static int +udevIfaceGetIfaceDefBond(struct udev *udev ATTRIBUTE_UNUSED, + 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 (virStrToLong_i(tmp_str, NULL, 10, &tmp_int) < 0) {

How does virStrToLong_i handle a NULL string? (Is it possible that udev_device_get_sysattr_value() could ever return NULL, e.g. if the value isn't set?)

+ 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 (virStrToLong_i(tmp_str, NULL, 10, &tmp_int) < 0) {

Same comment as above.

+ 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 (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 (virStrToLong_i(tmp_str, NULL, 10, &tmp_int) < 0) {

Same comment as above.

+ 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 (virStrToLong_i(strchr(tmp_str, ' ') + 1, NULL, 10, &tmp_int) < 0) {

Ooh. Are you guaranteed that udev_device_get_sysattr_value will return non-NULL, and that it will *always* contain a space? If not, the above line could segfault (especially devious because of the "+1" - even a function that checked for NULL and special-cased it would fail that check and try to dereference 0x1. I think you need to 1) check from tmp_str != NULL, 2) do the strchr() separately and check for NULL, then call virStrToLong_i().

+ 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 (virStrToLong_i(strchr(tmp_str, ' ') + 1, NULL, 10, &tmp_int) < 0) {

Same comment as above.

+ 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 (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; + + ifacedef->data.bond.target = + strdup(udev_device_get_sysattr_value(dev, "bonding/arp_ip_target"));

Same question here as before - is it possible that the above udev function vould return NULL in some case other than an error? If so, you'll need to call that in a separate step from strdup().

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

Need to check for NULL return here and appropriately cleanup if encountered.

+ 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 udevIfaceGetIfaceDefBridge(struct udev *udev, struct udev_device *dev, const char *name, @@ -752,6 +938,11 @@ udevIfaceGetIfaceDef(struct udev *udev, const char *name) 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; + } +

As with the "special case" check for vlans on kernels older than 3.7, this clause is checked even if ifacedef->type was already set to something else. Both this, and the special vlan case that looks for a "." in the device name, should be in an "else if()" of the original if clause.

switch (ifacedef->type) { case VIR_INTERFACE_TYPE_VLAN: if (udevIfaceGetIfaceDefVlan(udev, dev, name, ifacedef)) @@ -761,6 +952,10 @@ udevIfaceGetIfaceDef(struct udev *udev, const char *name) if (udevIfaceGetIfaceDefBridge(udev, dev, name, ifacedef)) goto cleanup; break; + case VIR_INTERFACE_TYPE_BOND: + if (udevIfaceGetIfaceDefBond(udev, dev, name, ifacedef)) + goto cleanup; + break;

Shoot! I didn't notice before that you're doing if(function()) here rather than the more standard "if (function() < 0)". All of the cases in this switch should check for < 0 rather than just non-0 to fit with libvirt coding practices (otherwise I find myself thinking that the function returns a string :-) That nit retroactively applies to the first 3 patches.

case VIR_INTERFACE_TYPE_ETHERNET: break; }