Re: [PATCH 06/10] network: Bridge - Add support for a DHCP Relay Agent

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]