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

[Stefan Berger]

On Fri, 2010-05-28 at 23:17 +0200, Jim Meyering wrote:
> 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.

Already there...

> 
> 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".

Fixed the two related to this patch. The storage one didn't appear.

> 
> 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)
> 

I'll go for this one.

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

Ok. Did that. v12 with an addition of my own coming shortly. Phew.

Thanks.

   Stefan