[libvirt] [PATCH v13] [REPOST] add 802.1Qbh and 802.1Qbg handling

[Jim Meyering]

Stefan Berger wrote:
> Formatting errors in the previous posting (and another problem).
>
> 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'.
>
> V13:
>  - Merging Scott's v13-pre1 patch
>  - Fixing endptr related bug while using virStrToLong_ui() pointed out
>    by Jim Meyering
...
> +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) == 0
> +                && (endptr == NULL || c_isspace(*endptr))
> +                && res != 0) {
> +                pid = res;
> +            } else {
> +                macvtapError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                             _("error parsing pid of lldpad"));
> +            }
> +        }
> +    } else {
> +        virReportSystemError(errno,
> +                             _("Error opening file %s"), LLDPAD_PID_FILE);
> +    }
> +
> +    if (fd >= 0)
> +        close(fd);
> +
> +    return pid;
> +}

Hi Stefan,

Sorry, but my suggestion was incomplete.  *endptr == '\0' indicates
a valid conversion, too.  That's what would happen if there's no byte
at all (not even a trailing newline) after the PID.  In addition, when
virStrToLong_ui returns 0, endptr cannot be NULL, so there was no need
for that test.  This handles it:

                && (*endptr == '\0' || c_isspace(*endptr))

With that, ACK.

A minor improvement: move the declarations of "buffer" and "endptr"
down into the if (saferead(... block alongside "res", since that's
the only scope in which they're used.

The rest looked fine, too -- but I don't know enough
to spot 802.1Qbx protocol bugs.

Jim

P.S. There are probably enough PID-reading blocks of
code like this that it'd be worth factoring this into
a function in util.c.