[libvirt] [PATCH 2/2] util: add missing error log messages when failing to get netlink VFINFO

[Eric Blake]

On 12/20/2012 01:01 PM, Laine Stump wrote:
> This patch fixes the lack of error messages when libvirt fails to find
> VFINFO in a returned netlinke response message.
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=827519#c10 is an example
> of the error message that was previously logged when the
> IFLA_VFINFO_LIST object was missing from the netlink response. The
> reason for this failure is detailed in
> 
>    https://bugzilla.redhat.com/show_bug.cgi?id=889319
> 
> Even though that root problem has been fixed, the experience of
> finding the root cause shows us how important it is to properly log an
> error message in these cases. This patch *seems* to replace the entire
> function, but really most of the changes are due to moving code that
> was previously inside an if() statement out to the top level of the
> function (the original if() was reversed and made to log an error and
> return).
> ---
>  src/util/virnetdev.c | 78 +++++++++++++++++++++++++++-------------------------
>  1 file changed, 40 insertions(+), 38 deletions(-)

'git diff -b' to the rescue.  I'm reviewing this simpler version that
ignores whitespace:

> diff --git c/src/util/virnetdev.c w/src/util/virnetdev.c
> index 205fd93..f5f4562 100644
> --- c/src/util/virnetdev.c
> +++ w/src/util/virnetdev.c
> @@ -1465,10 +1465,7 @@ static int
>  virNetDevParseVfConfig(struct nlattr **tb, int32_t vf, virMacAddrPtr mac,
>                         int *vlanid)
>  {
> -    const char *msg = NULL;
>      int rc = -1;
> -
> -    if (tb[IFLA_VFINFO_LIST]) {
>      struct ifla_vf_mac *vf_mac;
>      struct ifla_vf_vlan *vf_vlan;
>      struct nlattr *tb_vf_info = {NULL, };
> @@ -1476,13 +1473,20 @@ virNetDevParseVfConfig(struct nlattr **tb, int32_t vf, virMacAddrPtr mac,
>      int found = 0;
>      int rem;
> 
> +    if (!tb[IFLA_VFINFO_LIST]) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("missing IFLA_VF_INFO in netlink response"));
> +        goto cleanup;
> +    }
> +
>      nla_for_each_nested(tb_vf_info, tb[IFLA_VFINFO_LIST], rem) {
>          if (nla_type(tb_vf_info) != IFLA_VF_INFO)
>              continue;
> 
>          if (nla_parse_nested(tb_vf, IFLA_VF_MAX, tb_vf_info,
>                               ifla_vf_policy)) {
> -                msg = _("error parsing IFLA_VF_INFO");
> +            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                           _("error parsing IFLA_VF_INFO"));
>              goto cleanup;

Yeah, reporting the error as soon as it's known instead of squirreling
away a non-NULL msg to report later is nicer.

>          }
> 
> @@ -1503,15 +1507,13 @@ virNetDevParseVfConfig(struct nlattr **tb, int32_t vf, virMacAddrPtr mac,
>          }
>          if (found) {
>              rc = 0;
> -                break;
> -            }
> +            goto cleanup;

Okay, so ignoring whitespace makes this hunk look a bit odd :)  But the
logic is correct.

>          }
>      }
> -
> +    virReportError(VIR_ERR_INTERNAL_ERROR,
> +                   _("couldn't find IFLA_VF_INFO for VF %d "
> +                     "in netlink response"), vf);
>  cleanup:
> -    if (msg)
> -        virReportError(VIR_ERR_INTERNAL_ERROR, "%s", msg);
> -
>      return rc;
>  }

Sometimes, when doing a patch like this, I will intentionally break it
into two commits, one that does the logic change but leaves indents
wrong, and the next to fix just indents, as it is easier to review that
way.  But no need to rework it, you have my:

ACK.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20121220/65f5b85e/attachment-0001.sig>