On 21.12.2015 18:17, Laine Stump wrote:
> virNetDevVPortProfileGetStatus() would log the following error:
>
> Could not find netlink response with expected parameters
>
> anytime a port profile DISASSOCIATE operation was done for 802.1Qbh,
> even though the disassociate had been successfully completely. Then,
> due to the fortunate coincidence of status having been initialized to
> 0 and then not changed when the "failure" was encountered, it would
> still return a status of 0 (PORT_VDP_RESPONSE_SUCCESS), so the caller
> would assume a successful operation.
>
> This would result in a spurious log message though, and would fill in
> LastErrorMessage, so that the API would return that error if it
> happened during cleanup from some other error. That, in turn, would
> lead to an incorrect supposition that the response to the port profile
> disassociate was the cause of the failure.
>
> During debugging, I noticed that the VF in question usually had *no
> uuid* associated with it (big surprise), so the solution is *not* to
> send the previous instanceId down.
>
> This patch fixes virNetDevVPortProfileGetStatus() to only check the
> VF's uuid in the status if it was given an instanceId to check
> against. Otherwise it only checks that the particular VF is present
> (it will be).
>
> This does cause a difference in behavior, so I want confirmation from
> Cisco and IBM that this behavior change is desired (or at least not
> *un*desired) - rather than returning with status unchanged (and thus
> always 0, aka PORT_VDP_RESPONSE_SUCCESS) when instanceId is NULL, it
> will actually get the IFLA_PORT_RESPONSE. This could lead to
> revelation of error conditions we were previously ignoring. Or
> not. Only experimentation will tell. Note that for 802.1Qbg instanceId
> is *always* NULL, and for 802.1Qbh, it is NULL for all DISASSOCIATE
> commands.
>
> --- src/util/virnetdevvportprofile.c | 26
> +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9
> deletions(-)
>
> diff --git a/src/util/virnetdevvportprofile.c b/src/util/virnetdevvportprofile.c
> index d0d4552..427c67b 100644
> --- a/src/util/virnetdevvportprofile.c
> +++ b/src/util/virnetdevvportprofile.c
> @@ -1,5 +1,5 @@
> /*
> - * Copyright (C) 2009-2014 Red Hat, Inc.
> + * Copyright (C) 2009-2015 Red Hat, Inc.
> *
> * This library is free software; you can redistribute it and/or
> * modify it under the terms of the GNU Lesser General Public
> @@ -544,14 +544,22 @@ virNetDevVPortProfileGetStatus(struct nlattr **tb, int32_t vf,
> goto cleanup;
> }
>
> - if (instanceId &&
> - tb_port[IFLA_PORT_INSTANCE_UUID] &&
> - !memcmp(instanceId,
> - (unsigned char *)
> - RTA_DATA(tb_port[IFLA_PORT_INSTANCE_UUID]),
> - VIR_UUID_BUFLEN) &&
> - tb_port[IFLA_PORT_VF] &&
> - vf == *(uint32_t *)RTA_DATA(tb_port[IFLA_PORT_VF])) {
> + /* This ensures that the given VF is present in the
> + * IFLA_VF_PORTS list, and that its uuid matches the
> + * instanceId (in case we've associated it). If no
> + * instanceId is sent from the caller, that means we've
> + * disassociated it from this instanceId, and the uuid
> + * will either be unset (if still not associated with
> + * anything) or will be set to a new and different uuid
> + */
> + if ((tb_port[IFLA_PORT_VF] &&
> + vf == *(uint32_t *)RTA_DATA(tb_port[IFLA_PORT_VF])) &&
> + (!instanceId ||
> + (tb_port[IFLA_PORT_INSTANCE_UUID] &&
> + !memcmp(instanceId,
> + (unsigned char *)
> + RTA_DATA(tb_port[IFLA_PORT_INSTANCE_UUID]),
> + VIR_UUID_BUFLEN)))) {
> found = true;
> break;
> }
>
I must admit that even though I understand C, I have no idea whether
this is correct or not. I mean, I've never played with such hardware so
I'll let Stefan review this one.
Michal
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list