On 02/27/2013 07:57 PM, TJ wrote:
> From: TJ <linux@xxxxxx>
>
> 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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list