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

[Jim Meyering]

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