On 30.10.2014 16:14, Erik Skultety wrote:

Since there was a valid note to patch 43b67f2e about the best spot to check for bandwidth set call while having libvirt daemon run in session mode, this patch reverts previous changes dealing with bandwith (excluding NUMA!) in qemu_driver.c and qemu_command.c. There will be another patch in the series which introduces the fix itself. --- src/qemu/qemu_command.c | 11 ----------- src/qemu/qemu_driver.c | 6 ------ 2 files changed, 17 deletions(-)

Looking at the commit you are referring to, I spot other interesting things too. For instance, you are introducing @cfg variable to qemuDomainGetNumaParameters() but it's not used anywhere in the function (besides getting and unrefing driver's config) which is weird. So while you are at revert, you may revert that part too.

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 2e5af4f..be8e335 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7835,17 +7835,6 @@ qemuBuildCommandLine(virConnectPtr conn, _("CPU tuning is not available in session mode")); goto error; } - - virDomainNetDefPtr *nets = def->nets; - virNetDevBandwidthPtr bandwidth = NULL; - size_t nnets = def->nnets; - for (i = 0; i < nnets; i++) { - if ((bandwidth = virDomainNetGetActualBandwidth(nets[i])) != NULL) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("Network bandwidth tuning is not available in session mode")); - goto error; - } - } }

for (i = 0; i < def->ngraphics; ++i) { diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 373daab..631cb5f 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -10437,12 +10437,6 @@ qemuDomainSetInterfaceParameters(virDomainPtr dom, if (virDomainSetInterfaceParametersEnsureACL(dom->conn, vm->def, flags) < 0) goto cleanup;

- if (!cfg->privileged) { - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", - _("Network bandwidth tuning is not available in session mode")); - goto cleanup; - } - if (!(caps = virQEMUDriverGetCapabilities(driver, false))) goto cleanup;

I've always felt we should use a different approach. If we want to check the permissions we should do that as close to the actual place needing root permissions as possible. With the complexity of call tree in libvirt it's easy to forget about one possible path. I mean, I like the approach 2/2 more then this old one. Michal

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