Re: [libvirt] [PATCH v11] add 802.1Qbh and 802.1Qbg handling

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; + }