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