On 15.10.2012 12:26, Benjamin Cama wrote:
> Enabling IP forwarding without the administrator's consent is bad. Only
> check for forwarding, do not enable it.
> ---
> src/network/bridge_driver.c | 56 +++++++++++++++++++++++++++++++++----------
> 1 files changed, 43 insertions(+), 13 deletions(-)
>
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index e1846ee..e3e8dc2 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -1853,19 +1853,51 @@ networkReloadIptablesRules(struct network_driver *driver)
> }
> }
>
> -/* Enable IP Forwarding. Return 0 for success, -1 for failure. */
> +#define SYSCTL_PATH "/proc/sys"
> +
> +/* Check for IP Forwarding. Return 0 if required family is enabled, -1 for failure. */
> static int
> -networkEnableIpForwarding(bool enableIPv4, bool enableIPv6)
> +networkCheckIpForwarding(bool checkIPv4, bool checkIPv6)
> {
> int ret = 0;
> - if (enableIPv4)
> - ret = virFileWriteStr("/proc/sys/net/ipv4/ip_forward", "1\n", 0);
> - if (enableIPv6 && ret == 0)
> - ret = virFileWriteStr("/proc/sys/net/ipv6/conf/all/forwarding", "1\n", 0);
> - return ret;
> -}
> + char *buf = NULL;
>
> -#define SYSCTL_PATH "/proc/sys"
> + if (checkIPv4) {
> + ret = virFileReadAll(SYSCTL_PATH "/net/ipv4/ip_forward", 2, &buf);
> + if (ret != 2)
> + goto error;
> + if (STRNEQ(buf, "1\n")) {
> + networkReportError(VIR_ERR_SYSTEM_ERROR,
> + _("IP forwarding is not enabled on your system"));
We dropped networkReportError and use virReportError instead. Moreover,
you miss "%s" there:
virReportError(VIR_ERR_SYSTEM_ERROR, "%s", _("blah"));
> + goto cleanup;
> + }
> + }
> + if (checkIPv6) {
> + if (ret > 0)
This check is useless since VIR_FREE(NULL) is safe.
> + VIR_FREE(buf);
> + ret = virFileReadAll(SYSCTL_PATH "/net/ipv6/conf/all/forwarding", 2, &buf);
> + if (ret != 2)
> + goto error;
> + if (STRNEQ(buf, "1\n")) {
> + networkReportError(VIR_ERR_SYSTEM_ERROR,
> + _("IPv6 forwarding is not enabled on your system"));
> + goto cleanup;
> + }
> + }
> +
> + VIR_FREE(buf);
> + return 0;
> +
> +error:
> + virReportSystemError(errno, "%s", _("cannot check for IP forwarding"));
> +cleanup:
> + if (ret > 0) {
> + VIR_FREE(buf);
> + return -1;
> + } else {
> + return ret;
> + }
> +}
We rather stick with flow:
static int
func(...)
{
int ret = -1;
... <commands> ...
if (cond) goto cleanup;
... <commands> ...
ret = 0;
cleanup:
return ret;
}
>
> static int
> networkSetIPv6Sysctls(virNetworkObjPtr network)
> @@ -2140,11 +2172,9 @@ networkStartNetworkVirtual(struct network_driver *driver,
> if (virNetDevSetOnline(network->def->bridge, 1) < 0)
> goto err2;
>
> - /* If forwardType != NONE, turn on global IP forwarding */
> + /* If forwardType != NONE, check for IP forwarding */
> if (network->def->forwardType != VIR_NETWORK_FORWARD_NONE &&
> - networkEnableIpForwarding(v4present, v6present) < 0) {
> - virReportSystemError(errno, "%s",
> - _("failed to enable IP forwarding"));
> + networkCheckIpForwarding(v4present, v6present) < 0) {
> goto err3;
> }
>
>
Well, I am not sure if we can do this. What would happen if some of our
users rely on this already? I mean, it's there since ages.
Michal
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list