[libvirt] [PATCH 2/2] Iface: disallow network tuning in session mode globally

[Michal Privoznik]

On 30.10.2014 16:14, Erik Skultety wrote:
> Patch 43b67f2e disallowed network tuning only with qemu driver, however
> this patch moved the check for root privileges into
> virNetDevBandwidthSetInternal wrapper function, so the call should now
> fail in all possible cases. The reason to create another wrapper is that
> we do execute a test suite with user privileges during compilation.
> Tests fail unless the wrapper is used.
> ---
>   src/libvirt_private.syms          |  4 ++++
>   src/util/virnetdevbandwidth.c     | 31 ++++++++++++++++++++++++-------
>   src/util/virnetdevbandwidthpriv.h | 36 ++++++++++++++++++++++++++++++++++++
>   tests/virnetdevbandwidthtest.c    |  5 +++--
>   4 files changed, 67 insertions(+), 9 deletions(-)
>   create mode 100644 src/util/virnetdevbandwidthpriv.h
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 9d5c814..ab3d6d0 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1647,6 +1647,10 @@ virNetDevBandwidthUnplug;
>   virNetDevBandwidthUpdateRate;
>
>
> +# util/virnetdevbandwidthpriv.h
> +virNetDevBandwidthSetInternal;
> +
> +
>   # util/virnetdevbridge.h
>   virNetDevBridgeAddPort;
>   virNetDevBridgeCreate;
> diff --git a/src/util/virnetdevbandwidth.c b/src/util/virnetdevbandwidth.c
> index 5fa231a..1fc6fdf 100644
> --- a/src/util/virnetdevbandwidth.c
> +++ b/src/util/virnetdevbandwidth.c
> @@ -21,8 +21,9 @@
>    */
>
>   #include <config.h>
> -
> -#include "virnetdevbandwidth.h"
> +#include <unistd.h>
> +#define __VIR_BANDWIDTH_PRIV_H_ALLOW__
> +#include "virnetdevbandwidthpriv.h"
>   #include "vircommand.h"
>   #include "viralloc.h"
>   #include "virerror.h"
> @@ -41,9 +42,8 @@ virNetDevBandwidthFree(virNetDevBandwidthPtr def)
>       VIR_FREE(def);
>   }
>
> -
>   /**
> - * virNetDevBandwidthSet:
> + * virNetDevBandwidthSetInternal:
>    * @ifname: on which interface
>    * @bandwidth: rates to set (may be NULL)
>    * @hierarchical_class: whether to create hierarchical class
> @@ -58,9 +58,9 @@ virNetDevBandwidthFree(virNetDevBandwidthPtr def)
>    * Return 0 on success, -1 otherwise.
>    */
>   int
> -virNetDevBandwidthSet(const char *ifname,
> -                      virNetDevBandwidthPtr bandwidth,
> -                      bool hierarchical_class)
> +virNetDevBandwidthSetInternal(const char *ifname,
> +                              virNetDevBandwidthPtr bandwidth,
> +                              bool hierarchical_class)
>   {
>       int ret = -1;
>       virCommandPtr cmd = NULL;
> @@ -242,6 +242,23 @@ virNetDevBandwidthSet(const char *ifname,
>       return ret;
>   }
>
> +int
> +virNetDevBandwidthSet(const char *ifname,
> +                      virNetDevBandwidthPtr bandwidth,
> +                      bool hierarchical_class)
> +{
> +    if (geteuid() != 0) {
> +        virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> +                       _("Network bandwidth tuning is not available"
> +                         " in session mode"));
> +        return -1;
> +    } else {
> +        return virNetDevBandwidthSetInternal(ifname,
> +                                             bandwidth,
> +                                             hierarchical_class);
> +    }
> +}
> +
>   /**
>    * virNetDevBandwidthClear:
>    * @ifname: on which interface
> diff --git a/src/util/virnetdevbandwidthpriv.h b/src/util/virnetdevbandwidthpriv.h
> new file mode 100644
> index 0000000..9c3a0fa
> --- /dev/null
> +++ b/src/util/virnetdevbandwidthpriv.h
> @@ -0,0 +1,36 @@
> +/*
> + * virnetdevbandwidthpriv.h: Functions for testing virNetDevBandwidth APIs
> + *
> + * Copyright (C) 2014 Red Hat, Inc.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library.  If not, see
> + * <http://www.gnu.org/licenses/>.
> + *
> + */
> +
> +#ifndef __VIR_BANDWIDTH_PRIV_H_ALLOW__
> +# error "virnetdevbandwidthpriv.h may only be included by virnetdevbandwidth.c or test suites"
> +#endif
> +
> +#ifndef __VIR_NETDEVBANDWIDTH_PRIV_H__
> +# define __VIR_NETDEVBANDWIDTH_PRIV_H__
> +
> +# include "virnetdevbandwidth.h"
> +
> +int virNetDevBandwidthSetInternal(const char *ifname,
> +                                  virNetDevBandwidthPtr bandwidth,
> +                                  bool hierarchical_class)
> +    ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK;
> +
> +#endif /* __VIR_NETDEVBANDWIDTH_PRIV_H__ */
> diff --git a/tests/virnetdevbandwidthtest.c b/tests/virnetdevbandwidthtest.c
> index 384991e..a93d0ae 100644
> --- a/tests/virnetdevbandwidthtest.c
> +++ b/tests/virnetdevbandwidthtest.c
> @@ -23,7 +23,8 @@
>   #include "testutils.h"
>   #define __VIR_COMMAND_PRIV_H_ALLOW__
>   #include "vircommandpriv.h"
> -#include "virnetdevbandwidth.h"
> +#define __VIR_BANDWIDTH_PRIV_H_ALLOW__
> +#include "virnetdevbandwidthpriv.h"
>   #include "netdev_bandwidth_conf.c"
>
>   #define VIR_FROM_THIS VIR_FROM_NONE
> @@ -79,7 +80,7 @@ testVirNetDevBandwidthSet(const void *data)
>
>       virCommandSetDryRun(&buf, NULL, NULL);
>
> -    if (virNetDevBandwidthSet(iface, band, info->hierarchical_class) < 0)
> +    if (virNetDevBandwidthSetInternal(iface, band, info->hierarchical_class) < 0)
>           goto cleanup;
>
>       if (!(actual_cmd = virBufferContentAndReset(&buf))) {
>

So IIUC, you are creating virnetdevbandwidthpriv.h just to satisfy the 
test? It's a bit overkill to me. How about:

1) checking EUID in virNetDevBandwidthSet() directly (without any 
virNetDevBandwidthSetInternal),
2) creating a mock for virnetdevbandwidthtest.c for geteuid() that will 
always return zero,
3) skipping the test on non-Linux platforms (they don't support mocking).

Michal