[libvirt] [PATCH] libxl: unref objects in error paths
by Jim Fehlig
libxlMakeNic opens a virConnect object and takes a reference on a
virNetwork object, but doesn't drop the references on all error
paths. Rework the function to follow the standard libvirt pattern
of using a local 'ret' variable to hold the function return value,
performing all cleanup and returning 'ret' at a 'cleanup' label.
Signed-off-by: Jim Fehlig <jfehlig(a)suse.com>
---
I noticed these leaks while trying to rework the code to use a
common virConnect object. That task has turned into quite an
effort [1], but in the meantime the leaks in the current code
should be plugged.
[1] https://www.redhat.com/archives/libvir-list/2016-February/msg01188.html
src/libxl/libxl_conf.c | 57 ++++++++++++++++++++++++--------------------------
1 file changed, 27 insertions(+), 30 deletions(-)
diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index b20df29..763f4c5 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -1286,7 +1286,11 @@ libxlMakeNic(virDomainDefPtr def,
{
bool ioemu_nic = def->os.type == VIR_DOMAIN_OSTYPE_HVM;
virDomainNetType actual_type = virDomainNetGetActualType(l_nic);
+ char *brname = NULL;
+ virNetworkPtr network = NULL;
+ virConnectPtr conn = NULL;
virNetDevBandwidthPtr actual_bw;
+ int ret = -1;
/* TODO: Where is mtu stored?
*
@@ -1312,64 +1316,50 @@ libxlMakeNic(virDomainDefPtr def,
if (l_nic->model) {
if (VIR_STRDUP(x_nic->model, l_nic->model) < 0)
- return -1;
+ goto cleanup;
if (STREQ(l_nic->model, "netfront"))
x_nic->nictype = LIBXL_NIC_TYPE_VIF;
}
if (VIR_STRDUP(x_nic->ifname, l_nic->ifname) < 0)
- return -1;
+ goto cleanup;
switch (actual_type) {
case VIR_DOMAIN_NET_TYPE_BRIDGE:
if (VIR_STRDUP(x_nic->bridge,
virDomainNetGetActualBridgeName(l_nic)) < 0)
- return -1;
+ goto cleanup;
/* fallthrough */
case VIR_DOMAIN_NET_TYPE_ETHERNET:
if (VIR_STRDUP(x_nic->script, l_nic->script) < 0)
- return -1;
+ goto cleanup;
if (l_nic->nips > 0) {
x_nic->ip = virSocketAddrFormat(&l_nic->ips[0]->address);
if (!x_nic->ip)
- return -1;
+ goto cleanup;
}
break;
case VIR_DOMAIN_NET_TYPE_NETWORK:
{
- bool fail = false;
- char *brname = NULL;
- virNetworkPtr network;
- virConnectPtr conn;
-
if (!(conn = virConnectOpen("xen:///system")))
- return -1;
+ goto cleanup;
if (!(network =
virNetworkLookupByName(conn, l_nic->data.network.name))) {
- virObjectUnref(conn);
- return -1;
+ goto cleanup;
}
if (l_nic->nips > 0) {
x_nic->ip = virSocketAddrFormat(&l_nic->ips[0]->address);
if (!x_nic->ip)
- return -1;
+ goto cleanup;
}
- if ((brname = virNetworkGetBridgeName(network))) {
- if (VIR_STRDUP(x_nic->bridge, brname) < 0)
- fail = true;
- } else {
- fail = true;
- }
-
- VIR_FREE(brname);
+ if (!(brname = virNetworkGetBridgeName(network)))
+ goto cleanup;
- virObjectUnref(network);
- virObjectUnref(conn);
- if (fail)
- return -1;
+ if (VIR_STRDUP(x_nic->bridge, brname) < 0)
+ goto cleanup;
break;
}
case VIR_DOMAIN_NET_TYPE_VHOSTUSER:
@@ -1385,18 +1375,18 @@ libxlMakeNic(virDomainDefPtr def,
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("unsupported interface type %s"),
virDomainNetTypeToString(l_nic->type));
- return -1;
+ goto cleanup;
}
if (l_nic->domain_name) {
#ifdef LIBXL_HAVE_DEVICE_BACKEND_DOMNAME
if (VIR_STRDUP(x_nic->backend_domname, l_nic->domain_name) < 0)
- return -1;
+ goto cleanup;
#else
virReportError(VIR_ERR_XML_DETAIL, "%s",
_("this version of libxenlight does not "
"support backend domain name"));
- return -1;
+ goto cleanup;
#endif
}
@@ -1438,7 +1428,14 @@ libxlMakeNic(virDomainDefPtr def,
x_nic->rate_interval_usecs = 50000UL;
}
- return 0;
+ ret = 0;
+
+ cleanup:
+ VIR_FREE(brname);
+ virObjectUnref(network);
+ virObjectUnref(conn);
+
+ return ret;
}
static int
--
2.6.1
8 years, 10 months
[libvirt] [PATCH v3 0/3] Add capability for text based polkit authentication for virsh
by John Ferlan
v2: http://www.redhat.com/archives/libvir-list/2016-February/msg00618.html
Adjustments since v2:
Patch 1 - Change not only the message (as requested), but also use a
different and new error code (VIR_ERR_AUTH_UNAVAILABLE).
Patch 2 - Added a check for "if (!isatty(STDIN_FILENO))"
Used a new parameter for pkttyagent call - "--notify-fd", which
is documented as "To get notified when the authentication agent
has been registered either listen to the Changed D-Bus signal
or use --notify-fd to pass the number of a file descriptor that
has been passed to the program. This file descriptor will then
be closed when the authentication agent has been successfully
registered. Followed the systemd mechanism. If it's felt that
a timeout of -1 is too dangerous, I'd be fine with changing it.
Patch 3 - Since we now can determine our failure based on err->code, use
the new VIR_ERR_AUTH_UNAVAILABLE to attempt the AgentCreate.
Also, since virPolkitAgentCreate now will wait for the agent
to start before returning, the agentstart counter is removed.
John Ferlan (3):
polkit: Adjust message when authentication agent isn't found
util: Introduce API's for Polkit text authentication
virsh: Add support for text based polkit authentication
include/libvirt/virterror.h | 3 +-
src/libvirt_private.syms | 2 ++
src/util/virerror.c | 8 ++++-
src/util/virpolkit.c | 85 ++++++++++++++++++++++++++++++++++++++++++---
src/util/virpolkit.h | 5 +++
tests/virpolkittest.c | 8 +++--
tools/virsh.c | 38 +++++++++++++++++---
tools/virsh.h | 2 ++
8 files changed, 138 insertions(+), 13 deletions(-)
--
2.5.0
8 years, 10 months
[libvirt] [PATCH 1/2] netdev: Use virNetDevIsVirtualFunction() properly
by Andrea Bolognani
virNetDevIsVirtualFunction() returns 1 if the interface is a
virtual function, 0 if it isn't and -1 on error. This means that,
despite the name suggesting otherwise, using it as a predicate is
not correct.
Fix two callers that were doing so adding an explicit check on
the return value.
---
src/util/virnetdev.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index ea95552..9e39c26 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -2531,7 +2531,7 @@ virNetDevReplaceNetConfig(const char *linkdev, int vf,
int ret = -1;
char *pfdevname = NULL;
- if (vf == -1 && virNetDevIsVirtualFunction(linkdev)) {
+ if (vf == -1 && virNetDevIsVirtualFunction(linkdev) == 1) {
/* If this really *is* a VF and the caller just didn't know
* it, we should set the MAC address via PF+vf# instead of
* setting directly via VF, because the latter will be
@@ -2571,7 +2571,7 @@ virNetDevRestoreNetConfig(const char *linkdev, int vf, const char *stateDir)
char *pfdevname = NULL;
const char *vfdevname = NULL;
- if (vf == -1 && virNetDevIsVirtualFunction(linkdev)) {
+ if (vf == -1 && virNetDevIsVirtualFunction(linkdev) == 1) {
/* If this really *is* a VF and the caller just didn't know
* it, we should set the MAC address via PF+vf# instead of
* setting directly via VF, because the latter will be
--
2.5.0
8 years, 10 months
[libvirt] [PATCH 2/2] hostdev: Remove temporary variable when checking for VF
by Andrea Bolognani
The virHostdevIsVirtualFunction() was called exactly twice, and in
both cases the return value was saved to a temporary variable before
being checked. This would be okay if it improved readability, but in
this case is pretty pointless.
Get rid of the temporary variable and check the return value
directly; while at it, change the check from '<= 0' to '!= 1' to
align it with the way other similar *IsVirtualFunction() functions
are used thorough the code.
---
src/util/virhostdev.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c
index 9ae217d..2db0da0 100644
--- a/src/util/virhostdev.c
+++ b/src/util/virhostdev.c
@@ -416,10 +416,8 @@ virHostdevNetConfigReplace(virDomainHostdevDefPtr hostdev,
int vf = -1;
int vlanid = -1;
bool port_profile_associate = true;
- int isvf;
- isvf = virHostdevIsVirtualFunction(hostdev);
- if (isvf <= 0) {
+ if (virHostdevIsVirtualFunction(hostdev) != 1) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("Interface type hostdev is currently supported on"
" SR-IOV Virtual Functions only"));
@@ -489,7 +487,6 @@ virHostdevNetConfigRestore(virDomainHostdevDefPtr hostdev,
int ret = -1;
int vf = -1;
bool port_profile_associate = false;
- int isvf;
/* This is only needed for PCI devices that have been defined
* using <interface type='hostdev'>. For all others, it is a NOP.
@@ -497,8 +494,7 @@ virHostdevNetConfigRestore(virDomainHostdevDefPtr hostdev,
if (!virHostdevIsPCINetDevice(hostdev))
return 0;
- isvf = virHostdevIsVirtualFunction(hostdev);
- if (isvf <= 0) {
+ if (virHostdevIsVirtualFunction(hostdev) != 1) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("Interface type hostdev is currently supported on"
" SR-IOV Virtual Functions only"));
--
2.5.0
8 years, 10 months
[libvirt] [python PATCH 0/4] formatting and (un)signed improvements
by Pavel Hrdina
Pavel Hrdina (4):
libvirt-override: all flags should be defined as unsigned int
libvirt-override: fix PyArg_ParseTuple for unsigned int
libvirt-override: fix PyArg_ParseTuple for unsigned long long
libvirt-override: fix PyArg_ParseTuple for size_t
libvirt-override.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
--
2.7.1
8 years, 10 months
[libvirt] [PATCH 0/3] More Coverity changes
by John Ferlan
All the issues here are found by a later version of Coverity than
the one used by our internal testing. Two were false positives and
one just an ordering issue with the virCheckFlags in the zfs backend.
The future coverity really has a problem with strtok_r usage after a
VIR_STRDUP which only checks < 0 because it's perfectly fine for the
"source" to be NULL and a < 0 check won't fail. The reason why this is
a problem is the target is then NULL and used for the strtok_r call.
Code inspection finds the cases we have are all false positives; however,
rather than making the <= or sa_assert() type check to validate that -
I chose to use the virStringSplitCount in order to split and inspect
the input string. It was much cleaner at least on the inspection front.
The storage changes related to checking (ret == -1) resulted in a
RESOURCE_LEAK false positive. As it turns out, the only reason why
-1 was being check was to determine whether or not the VIR_APPEND_ELEMENT
insertion failed. If that succeeds, then 'vol' is cleared anyway, so
truly we only need to attempt the free if we have a new vol. The
virStorageVolDefFree will quietly return if vol == NULL.
John Ferlan (3):
openvz: Use virStringSplitCount instead of strtok_r
zfs: Resolve RESOURCE_LEAK
storage: No need to check ret after VIR_APPEND_ELEMENT
src/openvz/openvz_conf.c | 32 +++++++++++---------------------
src/storage/storage_backend_logical.c | 2 +-
src/storage/storage_backend_zfs.c | 6 ++++--
3 files changed, 16 insertions(+), 24 deletions(-)
--
2.5.0
8 years, 10 months
[libvirt] [PATCH v4 0/2] notify about reverting to a snapshot
by Dmitry Andreev
Reverting to a snapshot may change domain configuration
but there will be no events about that in some cases.
Lack of the event become a problem for virt-manager
https://bugzilla.redhat.com/show_bug.cgi?id=1081148
This patch-set introduces new event and emits it in
qemuDomainRevertToSnapshot.
v4: rebase
v3:
[2/2] event is emited only in the case when stopped domain
is reverted to a stopped domain.
Dmitry Andreev (2):
Introduce new VIR_DOMAIN_EVENT_DEFINED_FROM_SNAPSHOT sub-event
qemu: emit 'defined' event after reverted to snapshot without state
changes
examples/object-events/event-test.c | 2 ++
include/libvirt/libvirt-domain.h | 1 +
src/qemu/qemu_driver.c | 8 +++++++-
tools/virsh-domain.c | 3 ++-
4 files changed, 12 insertions(+), 2 deletions(-)
--
1.8.3.1
8 years, 10 months
[libvirt] [PATCH] Revert "Error out on missing machine type in machine configs"
by Ján Tomko
Revert commit 55e6d8cd9eac7eb2aaa4d221585e9402cf7269d5.
It unconditionally required a machine type for all machine types
even though qemu is the only emulator using them.
Reverting this re-introduces the crash when someone fiddles with
libvirt's machine configs with /etc/, but fixes persistent domains
for other drivers.
Breaks https://bugzilla.redhat.com/show_bug.cgi?id=1267256 again.
---
A proper fix would be too invasive for the freeze, we would either
* have to revert commit f1a89a8 which relaxed the requirement
to execute the emulator
* or (partially) revert commits a8b628e and 3d92a00 which reintroduced
them to post parse callbacks and keep track of which parser
requires the machine type in virDomainDefParserConfig.
src/conf/domain_conf.c | 6 ------
1 file changed, 6 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 3b15cb4..79758d4 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -14851,12 +14851,6 @@ virDomainDefParseXML(xmlDocPtr xml,
goto error;
}
VIR_FREE(capsdata);
- } else {
- if (!def->os.machine) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("Missing machine type"));
- goto error;
- }
}
/* Extract domain name */
--
2.4.10
8 years, 10 months
[libvirt] [PATCH] libxl: implement virDomainInterfaceStats
by Joao Martins
Introduce support for domainInterfaceStats API call for querying
network interface statistics. Consequently it also enables the use of
`virsh domifstat <dom> <interface name>` command plus seeing the
interfaces names instead of "-" when doing `virsh domiflist <dom>`.
After successful guest creation we fill the network interfaces names
based on domain, device id and append suffix if it's emulated in the
following form: vif<domid>.<devid>[-emu]. We extract the network
interfaces info from the libxl_domain_config object in
libxlDomainCreateIfaceNames() to generate ifname. On domain cleanup we
also clear ifname, in case it was set by libvirt (i.e. being prefixed
with "vif"). We also skip these two steps in case the name of the
interface was manually inserted by the adminstrator. Since the
introdution of netprefix (commit a040ba9), ifnames with a registered
prefix will be freed on virDomain{Obj,Def}Format*, thus eliminating
the migration issues observed with the reverted commit d2e5538 whereas
source and destination would have the same ifname.
For getting the interface statistics we resort to virNetInterfaceStats
and let libvirt handle the platform specific nits. Note that the
latter is not yet supported in FreeBSD.
Signed-off-by: Joao Martins <joao.m.martins(a)oracle.com>
---
Changes since d2e5538:
- Check net->ifname beforehand, preventing a crash on the migration
failure path.
- Use LIBXL_GENERATED_PREFIX_XEN as opposed to hardcoding "vif"
- Bump from 1.3.0 to 1.3.2
- Change commit message to mention the migration issue and netprefix.
---
src/libxl/libxl_domain.c | 39 ++++++++++++++++++++++++++++++++++++
src/libxl/libxl_driver.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 91 insertions(+)
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
index 50f7eed..9d0da68 100644
--- a/src/libxl/libxl_domain.c
+++ b/src/libxl/libxl_domain.c
@@ -762,6 +762,18 @@ libxlDomainCleanup(libxlDriverPrivatePtr driver,
}
}
+ if ((vm->def->nnets)) {
+ size_t i;
+
+ for (i = 0; i < vm->def->nnets; i++) {
+ virDomainNetDefPtr net = vm->def->nets[i];
+
+ if (net->ifname &&
+ STRPREFIX(net->ifname, LIBXL_GENERATED_PREFIX_XEN))
+ VIR_FREE(net->ifname);
+ }
+ }
+
if (virAsprintf(&file, "%s/%s.xml", cfg->stateDir, vm->def->name) > 0) {
if (unlink(file) < 0 && errno != ENOENT && errno != ENOTDIR)
VIR_DEBUG("Failed to remove domain XML for %s", vm->def->name);
@@ -929,6 +941,32 @@ libxlConsoleCallback(libxl_ctx *ctx, libxl_event *ev, void *for_callback)
libxl_event_free(ctx, ev);
}
+/*
+ * Create interface names for the network devices in parameter def.
+ * Names are created with the pattern 'vif<domid>.<devid><suffix>'.
+ * devid is extracted from the network devices in the d_config
+ * parameter. User-provided interface names are skipped.
+ */
+static void
+libxlDomainCreateIfaceNames(virDomainDefPtr def, libxl_domain_config *d_config)
+{
+ size_t i;
+
+ for (i = 0; i < def->nnets && i < d_config->num_nics; i++) {
+ virDomainNetDefPtr net = def->nets[i];
+ libxl_device_nic *x_nic = &d_config->nics[i];
+ const char *suffix =
+ x_nic->nictype != LIBXL_NIC_TYPE_VIF ? "-emu" : "";
+
+ if (net->ifname)
+ continue;
+
+ ignore_value(virAsprintf(&net->ifname,
+ LIBXL_GENERATED_PREFIX_XEN "%d.%d%s",
+ def->id, x_nic->devid, suffix));
+ }
+}
+
/*
* Start a domain through libxenlight.
@@ -1073,6 +1111,7 @@ libxlDomainStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm,
if (libxl_evenable_domain_death(cfg->ctx, vm->def->id, 0, &priv->deathW))
goto cleanup_dom;
+ libxlDomainCreateIfaceNames(vm->def, &d_config);
if ((dom_xml = virDomainDefFormat(vm->def, cfg->caps, 0)) == NULL)
goto cleanup_dom;
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index 404016e..5479441 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -58,6 +58,7 @@
#include "virhostdev.h"
#include "network/bridge_driver.h"
#include "locking/domain_lock.h"
+#include "virstats.h"
#define VIR_FROM_THIS VIR_FROM_LIBXL
@@ -4662,6 +4663,56 @@ libxlDomainIsUpdated(virDomainPtr dom)
}
static int
+libxlDomainInterfaceStats(virDomainPtr dom,
+ const char *path,
+ virDomainInterfaceStatsPtr stats)
+{
+ libxlDriverPrivatePtr driver = dom->conn->privateData;
+ virDomainObjPtr vm;
+ ssize_t i;
+ int ret = -1;
+
+ if (!(vm = libxlDomObjFromDomain(dom)))
+ goto cleanup;
+
+ if (virDomainInterfaceStatsEnsureACL(dom->conn, vm->def) < 0)
+ goto cleanup;
+
+ if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_QUERY) < 0)
+ goto cleanup;
+
+ if (!virDomainObjIsActive(vm)) {
+ virReportError(VIR_ERR_OPERATION_INVALID,
+ "%s", _("domain is not running"));
+ goto endjob;
+ }
+
+ /* Check the path is one of the domain's network interfaces. */
+ for (i = 0; i < vm->def->nnets; i++) {
+ if (vm->def->nets[i]->ifname &&
+ STREQ(vm->def->nets[i]->ifname, path)) {
+ ret = 0;
+ break;
+ }
+ }
+
+ if (ret == 0)
+ ret = virNetInterfaceStats(path, stats);
+ else
+ virReportError(VIR_ERR_INVALID_ARG,
+ _("'%s' is not a known interface"), path);
+
+ endjob:
+ if (!libxlDomainObjEndJob(driver, vm))
+ vm = NULL;
+
+ cleanup:
+ if (vm)
+ virObjectUnlock(vm);
+ return ret;
+}
+
+static int
libxlDomainGetTotalCPUStats(libxlDriverPrivatePtr driver,
virDomainObjPtr vm,
virTypedParameterPtr params,
@@ -5539,6 +5590,7 @@ static virHypervisorDriver libxlHypervisorDriver = {
.domainGetJobStats = libxlDomainGetJobStats, /* 1.3.1 */
.domainMemoryStats = libxlDomainMemoryStats, /* 1.3.0 */
.domainGetCPUStats = libxlDomainGetCPUStats, /* 1.3.0 */
+ .domainInterfaceStats = libxlDomainInterfaceStats, /* 1.3.2 */
.connectDomainEventRegister = libxlConnectDomainEventRegister, /* 0.9.0 */
.connectDomainEventDeregister = libxlConnectDomainEventDeregister, /* 0.9.0 */
.domainManagedSave = libxlDomainManagedSave, /* 0.9.2 */
--
2.1.4
8 years, 10 months
[libvirt] [PATCH] Error out on missing machine type in machine configs
by Ján Tomko
Commit f1a89a8 allowed parsing configs from /etc/libvirt
without validating the emulator capabilities.
Check for the presence of os->type.machine even if the
VIR_DOMAIN_DEF_PARSE_SKIP_OSTYPE_CHECKS flag is set,
otherwise the daemon can crash on carelessly crafted input
in the config directory.
https://bugzilla.redhat.com/show_bug.cgi?id=1267256
---
src/conf/domain_conf.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 67415fa..5d3fed0 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -14841,6 +14841,12 @@ virDomainDefParseXML(xmlDocPtr xml,
goto error;
}
VIR_FREE(capsdata);
+ } else {
+ if (!def->os.machine) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Missing machine type"));
+ goto error;
+ }
}
/* Extract domain name */
--
2.4.10
8 years, 10 months
