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.