> -----Original Message----- > From: libvir-list-bounces@xxxxxxxxxx [mailto:libvir-list-bounces@xxxxxxxxxx] On > Behalf Of Michal Privoznik > Sent: Monday, December 21, 2015 11:56 PM > To: Laine Stump; libvir-list@xxxxxxxxxx > Cc: stefanb@xxxxxxxxxx > Subject: Re: [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 ACK I have not tested the fix above, but both the analysis and the fix seem correct to me. Thanks /Chris -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list