Stefan Berger wrote:
This is now hopefully the final version of this patch that adds support for configuring 802.1Qbg and 802.1Qbh switches. The 802.1Qbh part has been successfully tested with real hardware. The 802.1Qbg part has only been tested with a (dummy) server that 'behaves' similarly to how we expect lldpad to 'behave'.
V11: - determining pid of lldpad daemon by reading it from /var/run/libvirt.pid (hardcode as is hardcode alson in lldpad sources) - merging netlink send code for kernel target and user space target (lldpad) using one function nlComm() to send the messages - adding a select() after the sending and before the reading of the netlink response in case lldpad doesn't respond and so we don't hang - when reading the port status, in case of 802.1Qbg, no status may be received while things are 'in progress' and only at the end a status will be there. - when reading the port status, use the given instanceId and vf to pick the right IFLA_VF_PORT among those nested under IFLA_VF_PORTS.
Hi Stefan, There are a few nits, but nothing serious. If this will be pushed in your name, you should add your name/email to the AUTHORS file. There are two cpp-indentation nits: cppi: src/storage/storage_backend.h: line 96: not properly indented cppi: src/util/macvtap.c: line 75: not properly indented And one unmarked diagnostic: src/util/macvtap.c-766- "error parsing pid of lldpad"); Those three things were exposed by running "make syntax-check". Further, in this code, +static uint32_t +getLldpadPid(void) { + int fd; + uint32_t pid = 0; + + fd = open(LLDPAD_PID_FILE, O_RDONLY); + if (fd >= 0) { + char buffer[10]; + char *endptr; + + if (saferead(fd, buffer, sizeof(buffer)) <= sizeof(buffer)) { + unsigned int res; + + if (virStrToLong_ui(buffer, &endptr, 10, &res)) + macvtapError(VIR_ERR_INTERNAL_ERROR, "%s", + "error parsing pid of lldpad"); + else + pid = res; by passing a non-NULL &endptr (and not checking it after the call), you're declaring that any non-numeric suffix on that PID is valid. It would be better not to do that: it'd be better to ensure that anything following it is acceptable, e.g. via endptr == NULL || c_isspace(endptr) Or, if you can guarantee how it's written, simply require that it be followed by a newline: endptr && *endptr == '\n' Stylistic: In this function, +static int +doPortProfileOpCommon(bool nltarget_kernel, you add this loop: + while (--repeats >= 0) { + rc = link_dump(nltarget_kernel, NULL, ifindex, tb, &recvbuf); + if (rc) + goto err_exit; + rc = getPortProfileStatus(tb, vf, instanceId, nltarget_kernel, + is8021Qbg, &status); + if (rc == 0) { + if (status == PORT_PROFILE_RESPONSE_SUCCESS || + status == PORT_VDP_RESPONSE_SUCCESS) { + break; + } else if (status == PORT_PROFILE_RESPONSE_INPROGRESS) { + // keep trying... + } else { + virReportSystemError(EINVAL, + _("error %d during port-profile setlink on " + "interface %s (%d)"), + status, ifname, ifindex); + rc = 1; + break; + } + } else + goto err_exit; + + usleep(STATUS_POLL_INTERVL_USEC); + + VIR_FREE(recvbuf); + } However, I find it simpler to read if you put the single-stmt-goto-in-else-clause first, and then un-indent what is currently the body of that if block: + if (rc != 0) + goto err_exit; + + if (status == PORT_PROFILE_RESPONSE_SUCCESS || + status == PORT_VDP_RESPONSE_SUCCESS) { + break; + } else if (status == PORT_PROFILE_RESPONSE_INPROGRESS) { + // keep trying... + } else { + virReportSystemError(EINVAL, + _("error %d during port-profile setlink on " + "interface %s (%d)"), + status, ifname, ifindex); + rc = 1; + break; + }