Re: [libvirt] [PATCH v4 3/6] util: Add helper APIs to get/verify VF Representor name

On 06/28/2018 09:22 AM, Jai Singh Rana wrote: This will certainly be where things get a bit more "unknown" to me... Still I think this patch can be split in half - with one patch being just the promotion of virHostdevNetDevice to external and the second adding the new module. Could you add a brief header here describing the inputs and what it returns. Perhaps of most interest is the values that @pfNetDevIdx can be (I see -1, 0, 1 being used). It also looks like @linkdev and @vf are returned as well as 0, -1 for success/failure. The @vf can be -1 and ret == 0 if the else condition is followed. Useful things to know for external consumers. (3) had better be NONNULL too! Far be it for me to be the license expert, but there's no copyright year here. I would have at least expected something, such as: * Copyright (C) 2018 XXX where for XXX I've seen people add a company name or their own name - there's lots of examples to choose from. Not my exact area of expertise over the "right value" though. I would put Red Hat Inc. there, but then again, that's where I work. Typically we prefer -1 on failure, 0 on success Still you seem to be having a 3rd condition where 0 could be we failed and we don't care and 1 could be we succeeded. The -1 could be left as a fatal error since currently there'd be no way to determine the difference between OOM from virAsprintf or failure from virFileReadAllQuiet. In both cases the caller should ignore/reset the last error rather than this lower down API. should be a fatal failure could be a graceful failure "that the driver" s/ignoring/ignore Seems like this could be a graceful failure Would be a fatal failure. I'll let you decide on the rest. Could be a goto cleanup too... because... ...if we complete the virDirRead loop without any match is that a bad thing? ^^^ This .... ... doesn't match this ^^^^ Asking from the point of not sure, but is "VFRIfName" redundant? That is could it just be "VFRName" or is there a delineation between a VFR Name and a VFR IFName that needs to be made. You may want to just call this "GetVFRInfo" leaving for the possibility of future expansion to more than just @ifname. Thus you pass the @ifname as a parameter and use 0 and -1 return values to indicate success or failure. This is what I meant earlier - you have no way to determine if the failure is fatal or not and resetting an OOM may not acceptible from this caller. I think the caller of this API should make that determination (something I validate later on). Why not just return vf_ifname directly later? It's already a VIR_STRDUP'd value - especially since you VIR_FREE it below. Same notes about the method name Again, you really don't know the reason for failure here. This is a Check method with a couple of ways to fail. Even the previous call could have some sort of failure that isn't reset. So prehaps the Reset should be in cleanup in this case. ret = STREQ(ifname, vf_ifname); Not sure I ever see the need for this to be specific... You're not validating that ifname is a VFRIfName, so the GetProcNetdevStats could just be called directly instead when it's needed later on. If by chance there could be multiple ways to obtain stats for one of these I could see use for the wrapper, but that doesn't happen in this patch series, so the direct call is better since the assumption is that @ifname is a vf_ifname (from the eventual caller). If any of the above in the #ifdef __linux__ do change, don't forget to modify these as well! Why the change here? IDC, but consisitently speaking it's different. Same here w/ copyright year You can follow/copy the .c header here and the other two: char * virNetdevHostdevGetVFRIfName(...) John