Re: [PATCH v2 2/2] util: Add phys_port_name support on virPCIGetNetName

On 1/21/21 7:15 AM, Moshe Levi wrote: This change disappears once the incorrect chunks are removed from patch 1. Likewise. Likewise (side note: I was spelunking the code trying to figure out when this idx is actually set and used, and it seems like the answer might be "never". I should probably look into cleaning it up) When I started using g_autofree and explicitly called VIR_FREE() on a g_autofree pointer so I could re-use it, I was told this is a bad practice and to avoid it. We can easily avoid it in this case by declaring thisPhysPortName inside the else clause where it is used (just before the line above us ^^^ that calls virNetDevGetPhysPortName()) - that way it will be autofreed each time through the loop. All this stuff about setting firstEntryName is pointless in the else clause of "if (physPortID)": we will either get to the bottom of the while() loop and return without using firstEntryName, or we will continue back to the top of the loop until we run out of entries, drop out of the loop, and get to:   if (!physPortID)       return 0; Since we've already established that physPortID == NULL, that's going to take us out of this function before we can use firstEntryName. The else clause here is what will be taken for any sort of hardware that has only a single netdev listed in the "net" directory of the PCI device (so pretty much everything except a new generation Mellanox card, right?). I would have thought that in this case we should treat it as below (just increment i and continue) (BTW, if one of the netdevs in a PCI device's "net" directory has a phys_port_name, is it *always* the case that all the other netdevs listed in that directory also have a phys_port_name? (ie if the PF has a phys_port_name, will the VF representors always have a phys_port_name, and vice versa?) I'm assuming the answer is "yes". After thinking about this a bit, I've come up with the attached diff (based on your patches - you can just add it to your local directory with "git am -3 0001-blah.patch"). Does that look right to you? If it does (and it works!) then I can squash it in and push. If you're not quite happy with it, then you can send another iteration. I promise I'll be much faster about responding now!! (be sure to Cc me so I notice it though, and maybe even ping me in IRC.) There should be no space between # and define here. I'm trying to remember if it's the cppi package that finds that, or something else. (that's also in my diff)