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;
+ }
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list