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