Re: [libvirt] [PATCH 06/10] network: Bridge - Add support for a DHCP Relay Agent
On 02/27/2013 07:57 PM, TJ wrote:
From: TJ <linux(a)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
Attachments:
- signature.asc (application/pgp-signature — 621 bytes)