Re: [libvirt] [PATCH v3 1/2] util: Add helper APIs to get/verify VF Representor name

Thanks again John for giving detailed review and feedback for v3 patchset. I have tried incorporating the suggestions in v4. v4: https://www.redhat.com/archives/libvir-list/2018-June/msg01807.html -Jai ________________________________ From: John Ferlan <jferlan(a)redhat.com&gt; Sent: 13 April 2018 00:38 To: Jai Singh Rana; libvir-list(a)redhat.com Cc: Rana, JaiSingh Subject: Re: [libvirt] [PATCH v3 1/2] util: Add helper APIs to get/verify VF Representor name On 04/04/2018 12:29 PM, Jai Singh Rana wrote: You are missing the S-o-b: The someone new policy is that: Contributors to libvirt projects must assert that they are in compliance with the Developer Certificate of Origin 1.1. This is achieved by adding a "Signed-off-by" line containing the contributor's name and e-mail to every commit message. The presence of this line attests that the contributor has read the above lined DCO and agrees with its statements. https://developercertificate.org/ You probably don't have cppi installed, but if you did it would have pointed out a few more syntax-check issues... cppi: src/util/virnetdevhostdev.c: line 41: not properly indented IOW: "# define" FWIW: Easiest way to test that your compilation works on "linux" and something else is to simply change this line to something else that wouldn't be defined and that would show if your #else logic works... A < 0 is an error A == 0 is empty file so be careful in just cut-copy-paste as this will fail for some unknown reason if the length of what's read == 0. This should also go after the virAsprintf for pf_switch_id_file And you don't really the { } for a one line goto cleanup although it passed syntax-check because of the multi-line virFileReadAddQuiet Your comments though make it seem like you want to check if the file exists first before reading (e.g. if (!virFileExists())). Then if it doesn't exist, return. You may also want to alter your returns to be -1 error, 0 nothing gathered, and 1 something gathered. Don't forget to document that too. Looking at patch 2 which is where virNetdevHostdevGetVFRIfName (the caller to this method) is called - if the returned name is NULL, then the data would be ignored anyway. So the 0 and 1 return could be useful. s/a match/a match/ Also, entire comment is incorrectly indented. Again, should we first to a if (!virFileExists()) continue; type operation? So if <= we're falling through to the next step, which I don't think you want. This one doesn't follow your pattern to add a VIR_FREE(pf_subsystem_device_port_name_file) beforehand within your while loop... Should be unnecessary since virFree will do that for you Again more thought around !virFileExists() Again the pattern of VIR_FREE(vf_num_str); beforehand > + if (virAsprintf(&vf_num_str, "%d", vf) < 0) > + goto cleanup; > + > + /* phys_port_name may contain just VF number or string in format > + * as pf'X'vf'Y' or vf'Y', where X and Y are PF and VF numbers. > + * As at this point, we are already with correct PF, just need > + * to verify VF number now. > + */ > + > + /* vf_rep_ifname read from file may contain new line,replace with '\0' > + for string comparison below */ > + i = strlen(vf_rep_ifname); > + if (c_isspace(vf_rep_ifname[i-1])) { > + vf_rep_ifname[i-1] = '\0'; > + i -= 1; > + while (c_isdigit(vf_rep_ifname[i-1])) > + i -= 1; > + > + if ((ret = STREQ((vf_rep_ifname + i), vf_num_str))) { This changes ret < == > 0... don't believe that's what you really want. For the above last hunk... All if this because you're trying to determine if the passed "vf" number is equal to the number portion of the vf_rep_ifname? I believe virSkipSpaces and virParseNumber will help you be a whole lot cleaner! Considering my comment above and changing the return values means this could change as well. At the very least actually checking < 0 for failure... To some degree since the caller only cares if the return is NULL or not and doesn't error when it's not, then all/any error generated will be lost. So that means would could be a valid reason to cause a failure would be lost and the data just ignored. At the very least a virResetLastError would clear away the error Before I read this comment after looking at the next patch and wondering what was different to virNetDevTapInterfaceStats, my first gut response follows. Now that I've dealt with that - rather than cut-n-paste-n-copy - you need to figure out a way to make use of the existing code. That means introducing an API that the __linux___ version of virNetDevTapInterfaceStats would call and this function would also call. Perhaps something along the lines of virNetDevGetProcNetDev that lives in src/util/virnetdev.c... One patch would extract it out and the next one would also use it for your hostdev path. FWIW: Still some of my feeings below could still apply to/for an improved processing of the file/data - especially w/r/t format of the file, headings, etc. Since "something" already exists though, we can live with it I suppose, even though it's perhaps not optimal Tehnically there's a header row or two that you're missing from what I see... Once you get beyond and verify the header, you may want to look into virStringSplit - it's pretty powerful and this is just ripe for all sorts of issues especially if the format of the output *ever* changes.... In fact I'd go as far to say that checking/learning which column heading goes with which data element might be very useful thing to add here especially if it ever differs between distros or ever has changed or could change. Check out using virFileReadAll and other API's that read /proc files. If you can "map" your expected headers into the format you're reading and ensure that no future change would alter what you've magically sscanf'd below, then that future proofs yourself a bit. You could possibly also create some magic that would pull only the ones you care about based on the headers provided. There's "inventive ways" to use ARRAY_CARDINALITY (see virNetClientFindDefaultSshKey) that will help you make sure that the header doesn't change which skews your results. In any case, the folowing at the very least should follow virHostCPUGetStatsLinux or virHostMemGetStatsLinux in order to make sure headers match sscanf'd columns. Wouldn't this be more an empty /proc/net/dev file? Is that a sign of a different problem? We've already covered the missing file above. static functions don't need to be in the #else Parameters here are different than .h file. This one's not static... Please place the order of functions in the #else section the same as in the #if section cppi: src/util/virnetdevhostdev.h: line 21: not properly indented cppi: src/util/virnetdevhostdev.h: line 22: not properly indented IOW: "# define" and "# include" Add a blank like for readability Same here... Also keep the same function order as the .c file - it's just easier that way to compare .h and .c... Why change format? s/b int virNetdevHostdev... John