Re: [libvirt] [PATCH] nodedev: add switchdev to NIC capabilities

On 08/02/2017 06:00 AM, Edan David wrote: Not my area of expertise, but I'll point out some "basic" stuff... You really should have considered adding some of what you added to : https://www.redhat.com/archives/libvir-list/2017-August/msg00072.html into your commit message to describe what you're doing and why If only one patch, then the details/links or background can go after the following "---" in order to help the reviewer get some extra details. If you had multiple patches, then that'd be part of the cover letter Before sending patches, please be sure to run w/ "make check syntax-check" - this one fails syntax-check in a few places: preprocessor_indentation cppi: src/util/virnetdev.c: line 3128: not properly indented cppi: src/util/virnetdev.c: line 3135: not properly indented maint.mk: incorrect preprocessor indentation cfg.mk:684: recipe for target 'sc_preprocessor_indentation' failed make: *** [sc_preprocessor_indentation] Error 1 Spurious whitespace Only 2 lines between functions, not 3 This seems like a cut-n-paste from somewhere - it's not fully formed and certainly what it says to invoke isn't invoked prior to this call! use multiple lines for multiple args: e.g. virNetDevPutExtraHeader(struct nlmsghdr *nlh, size_t size) None of those NLMSG_* macros do the trick? uint32_t being returned to an int function. if (!(gmsgh = virNet*())) goto cleanup; Only one place to have the goto - just put the error message inline and goto cleanup; same here Is this the case where this isn't really supported? I wonder if perhaps this function should return -1 on failures, 0 on unsupported, and some value on supported. After Moshe Levi added the rdma and txudptnl code (commit id 'ac3ed2085') there was a bug that was reported by testing that I fixed via commit id '136f17efd'. There may have been something else too, but it's been too long to remember! I guess it comes down to the reason why this would be NULL when HAVE_DECL_DEVLINK is set and whether you really want things like "virsh nodedev-list --cap net" to fail in that case. uint32_t family_id; if ((family_id = virNetDevGetFamilyId(DEVLINK_GENL_NAME)) == 0) return -1 of course depends on whether you decide to use -1, 0, & value type returns, but you get the general gist. if ((is_vf = virNetDevIsVirtualFunction(ifname)) < 0) return -1 if (is_vf == 1 && virNetDevGetPhysicalFunction(ifname, &pfname) < 0) return -1; return -1; could just be "if (!(gsmsg = virNetDev*()))" Not my preferred style to have multiple goto's like this, but I see it's generally used in this manner for the various virnetdev*.c BTW: I think it looks better as something like: if ((nla_put() < 0) || (nla_put() < 0)) { virReportError(); goto cleanup; For this one, there's no need to have the additional goto as there's only one place that You need to add ATTRIBUTE_UNUSED after each argument like virNetDevGetEthtoolGFeatures Since this is called after virNetDevRDMAFeature it could have gone after the hunk that supports that - IOW: this next hunk. Not a requirement - it's one of those type A code flow things ;-) John