Re: [libvirt] [PATCH 2/5] util: don't log error in virNetDevVPortProfileGetStatus if instanceId is NULL

-----Original Message----- From: libvir-list-bounces(a)redhat.com [mailto:libvir-list-bounces@redhat.com] On Behalf Of Michal Privoznik Sent: Monday, December 21, 2015 11:56 PM To: Laine Stump; libvir-list(a)redhat.com Cc: stefanb(a)us.ibm.com Subject: Re: [libvirt] [PATCH 2/5] util: don't log error in virNetDevVPortProfileGetStatus if instanceId is NULL 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