[libvirt] [PATCHv2 2/6] interface: udev bridge code error handling updates

[Laine Stump]

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