On 02/27/2013 07:57 PM, TJ wrote:
From: TJ <linux@iam.tj>
A DHCP relay daemon will be started that will forward all DHCP/BOOTP requests on the bridge network via the first declared forward interface. The relay is broadcast rather than routed so no IP address is needed on the bridge.
The XML <relay> element's "relay" property of the active DHCP stanza defaults to 'no'. Set it to 'yes' to enable the relay:
<ip ...> <dhcp relay='yes'/> </ip>
The relay will not be started if the "enable" property is 'no':
<ip ...> <dhcp enable='no' relay='yes'/> </ip>
At this point in the series, I'll defer to Laine for technical review. But I still have some style review:
+ + /* Use the first forwarding device to broadcast to the upstream DHCP server */ + if (network->def->forward.nifs > 0) { + virCommandAddArgList(cmd, "-b", network->def->forward.ifs[0].device.dev, NULL); + ret = 0;
No TABs in .c, ever. Run 'make syntax-check'.
+ } else
HACKING says that if you use {} on one side of 'else', you must use it on both sides.
+ virReportSystemError(VIR_ERR_INVALID_INTERFACE, + _("DHCP relay requires at least one network %s\n"), + "<forward ... dev='eth?'/> or <interface dev='eth?'/>");
Indentation is off, even when you get rid of that TAB. You have a mixed-language sentence - translators will get the first half, but can't translate the 'or' in the second string, and that looks gross. I would have written: virReportSystemError(VIR_ERR_INVALID_INTERFACE, "%s", _("DHCP relay requires at least one network " "<forward dev='eth?'/> or " "<interface dev='eth?'/>");
+static int +networkBuildDhcpRelayCommandLine(virNetworkObjPtr network, virCommandPtr *cmdout, + char *pidfile) +{ + virCommandPtr cmd = NULL; + int ret = -1; + + cmd = virCommandNew(DHCPRELAY); + if (networkBuildDhcpRelayArgv(network, pidfile, cmd) < 0) { + goto cleanup; + } + + if (cmdout) + *cmdout = cmd; + ret = 0; +cleanup: + if (ret < 0) + virCommandFree(cmd); + return ret;
Memory leak if ret == 0 but !*cmdout.
+} + +static int +networkStartDhcpRelayDaemon(struct network_driver *driver ATTRIBUTE_UNUSED, + virNetworkObjPtr network)
Indentation is off.
+{ + virCommandPtr cmd = NULL; + virNetworkIpDefPtr ipdef = NULL; + char *pidfile = NULL; + char *tmp = NULL; + int pid_len; + int ret = 0; + const char *dhcprelay = "dhcprelay_"; + + ipdef = networkGetActiveDhcp(network); + /* Prepare for DHCP relay agent */ + if (ipdef && ipdef->dhcp_enabled && ipdef->dhcp_relay) { + ret = -1;
Indentation is off.
+ + if (virFileMakePath(NETWORK_PID_DIR) < 0) { + virReportSystemError(errno, + _("cannot create directory %s"), + NETWORK_PID_DIR); + goto cleanup; + } + + pid_len = strlen(dhcprelay) + strlen(network->def->name); + if ( VIR_ALLOC_N(tmp, pid_len+1) >= 0) {
Spacing is off. Correct would be: if (VIR_ALLOC_N(tmp, pid_len + 1) >= 0) { But you shouldn't need to do VIR_ALLOC_N in the first place, since...
+ tmp = strcpy(tmp, dhcprelay); + tmp = strncat(tmp, network->def->name, pid_len);
...strncat() is generally the wrong interface. Use virAsprintf or virBuffer* instead.
+ if (!(pidfile = virPidFileBuildPath(NETWORK_PID_DIR, tmp))) { + virReportOOMError(); + goto cleanup; + } + } else { + virReportOOMError(); + goto cleanup; + }
This indentation mess with TAB is making this hard to review. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org