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;
> }
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list