Re: [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]); }