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