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