Re: [libvirt] [PATCH v13] [REPOST] add 802.1Qbh and 802.1Qbg handling
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.