Re: [libvirt] [v5 PATCH] add 802.1Qbh handling for port-profiles based on Stefan's previous patches

Scott Feldman wrote: ... ... Hi Scott, The above introduces a syntax error. Fix it by inserting a "test" after the "||": if test "$with_macvtap" = "yes" || test "$with_virtualport" = "yes"; then ... ... The parameters, respbuflen and nl_groups make sense only when non-negative, so my reflex is to make their types reflect that. Any reason not to use an unsigned type in those cases? This "to_usecs" parameter is in the same boat, in that it is logically non-negative. I suggest using unsigned long long for it, since technically (with the two longs in struct timeval) a portable timeout can be as large as 2^31 * 1000^2 microseconds. Also, it'd be nice to rename it to "timeout_usecs", since "to_usecs" made me think of a flag that says whether to convert "to microseconds". This should be of type size_t. It is used as an accumulator, and we do add nbytes (of type ssize_t) to it, and (esp!) use it as an array index, so its type must be unsigned. Otherwise, overflow could be exploitable. At least "n" and "rfds" are used only in the while loop, so their declarations belong "down" there, too. Personal preference: I find that a name like "got_valid" is easier to read than a variablenamewithnoseparatorbetweenwords. Same for rcvoffset vs. rcv_offset, etc. That handles the case of an expired timeout. However, if select fails, it returns -1. You should diagnose that failure rather than going ahead and reading from "fd". I'm used to seeing this: if (err->error != -EOPNOTSUPP) { why do you do it the other way? Such differences are generally discouraged. The above is a dead store, since VIR_FREE has just done that. Likewise. Remove the dead store. Likewise. Remove the dead store. ... When I first read the "sizeof(i)" below I did a double take. I'd prefer a name that does not make me think of an integer index. Please remove the unary "-", since it's not used here. That "break;" should be indented. Same here. Indent. And here. Indent the "break". ... Can you use a symbolic constant instead of "64" ? Typo. s/beging/begin/ There is too much duplication in the above 6 clauses. Can you factor out a tiny helper function to make this more readable/maintainable? At the very least, combine the two "if...goto buffer_too_small" stmts into one: if (!rta || !nlAppend(nlm, sizeof(nlmsgbuf), rtattbuf, rta->rta_len)) goto buffer_too_small; > + > + /* end nesting of vport */ > + vfport->rta_len = (char *)nlm + nlm->nlmsg_len - (char *)vfport; > + > + if (vf != PORT_SELF_VF) { > + /* end nesting of vfports */ > + vfports->rta_len = (char *)nlm + nlm->nlmsg_len - (char *)vfports; > + } > + > + if (!multicast) { > + if (nlComm(nlm, &recvbuf, &recvbuflen) < 0) > + return -1; > + } else { > + if (nlCommWaitSuccess(nlm, RTMGRP_LINK, &recvbuf, &recvbuflen, > + 5 * MICROSEC_PER_SEC) < 0) > + return -1; > + } > + > + if (recvbuflen < NLMSG_LENGTH(0) || recvbuf == NULL) > + goto malformed_resp; > + > + resp = (struct nlmsghdr *)recvbuf; > + > + switch (resp->nlmsg_type) { > + case NLMSG_ERROR: > + err = (struct nlmsgerr *)NLMSG_DATA(resp); > + if (resp->nlmsg_len < NLMSG_LENGTH(sizeof(*err))) > + goto malformed_resp; > > + switch (-err->error) { Indent the break: case 0: break; Actually, since there's only one action here, just drop the switch in favor of a simple "if" stmt. (same with the other switch (-err->error) above) That makes the resulting code shorter by 4 lines per switch stmt. if (err->error) { virReportSystemError(-err->error, _("error during virtual port configuration of ifindex %d"), ifindex); rc = -1; } Indent. Indent. This can (hence should) be unsigned. This number is obviously related to the 125000-microsecond sleep below. Consider adding a comment saying up to how long we should wait for a result. What if getPortProfileStatus returns something other than 0? Currently, when that happens, this code just ignores it (failure?) sleeps and retries. Doesn't failure to get status deserve an error just as much as when we do get status that indicates a failure? Magic constant? How did you determine this number? How often are we expected to hit it? Please add a comment.