[libvirt PATCH] util: don't log error if SRIOV PF has no associated netdev
by Laine Stump
Some SRIOV PFs don't have a netdev associated with them (the spec
apparently doesn't require it). In most cases when libvirt is dealing
with an SRIOV VF, that VF must have a PF, and the PF *must* have an
associated netdev (the only way to set the MAC address of a VF is by
sending a netlink message to the netdev of that VF's PF). But there
are times when we don't need for the PF to have a netdev; in
particular, when we're just getting the Switchdev Features for a VF,
we don't need the PF netdev - the netdev of the VF (apparently) works
just as well.
Commit 6452e2f5 (libvirt 5.1.0) *kind of* made libvirt work around PFs
with no netdevs in this case - if virNetDevGetPhysicalFunction
returned an error when setting up to retrieve Switchdev feature info,
it would ignore the error, and then check if the PF netdev name was
NULL and, if so it would reset the error object and continue on rather
than returning early with a failure. The problem is that by the time
this special handling occured, the error message about missing netdev
had already been logged, which was harmless to proper operation, but
confused the user.
Fortunately there are only 2 users of virNetDevGetPhysicalFunction, so
it is easy to redefine it's API to state that a missing netdev name is
*not* an error - in that case it will still return success, but the
caller must be prepared for the PF netdev name to be NULL. After
making this change, we can modify the two callers to behave properly
with the new semantics (for one of the callers it *is* still an error,
so the error message is moved there, but for the other it is okay to
continue), and our spurious error messages are a thing of the past.
Resolves: https://bugzilla.redhat.com/1924616
Fixes: 6452e2f5e1837bd753ee465e6607ed3c4f62b815
Signed-off-by: Laine Stump <laine(a)redhat.com>
---
src/util/virnetdev.c | 31 +++++++++++++++----------------
1 file changed, 15 insertions(+), 16 deletions(-)
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index 2b4c8b6280..7b766234ec 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -1339,7 +1339,8 @@ virNetDevGetVirtualFunctionIndex(const char *pfname, const char *vfname,
*
* @ifname : name of the physical function interface name
* @pfname : Contains sriov physical function for interface ifname
- * upon successful return
+ * upon successful return (might be NULL if the PF has no
+ * associated netdev. This is *not* an error)
*
* Returns 0 on success, -1 on failure
*
@@ -1361,15 +1362,6 @@ virNetDevGetPhysicalFunction(const char *ifname, char **pfname)
return -1;
}
- if (!*pfname) {
- /* The SRIOV standard does not require VF netdevs to have
- * the netdev assigned to a PF. */
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("The PF device for VF %s has no network device name"),
- ifname);
- return -1;
- }
-
return 0;
}
@@ -1442,6 +1434,17 @@ virNetDevGetVirtualFunctionInfo(const char *vfname, char **pfname,
if (virNetDevGetPhysicalFunction(vfname, pfname) < 0)
return -1;
+ if (!*pfname) {
+ /* The SRIOV standard does not require VF netdevs to have the
+ * netdev assigned to a PF, but our method of retrieving
+ * VFINFO does.
+ */
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("The PF device for VF %s has no network device name, cannot get virtual function info"),
+ vfname);
+ return -1;
+ }
+
if (virNetDevGetVirtualFunctionIndex(*pfname, vfname, vf) < 0)
goto cleanup;
@@ -3204,12 +3207,8 @@ virNetDevSwitchdevFeature(const char *ifname,
if ((is_vf = virNetDevIsVirtualFunction(ifname)) < 0)
return ret;
- if (is_vf == 1) {
- /* Ignore error if PF does not have netdev assigned.
- * In that case pfname == NULL. */
- if (virNetDevGetPhysicalFunction(ifname, &pfname) < 0)
- virResetLastError();
- }
+ if (is_vf == 1 && virNetDevGetPhysicalFunction(ifname, &pfname) < 0)
+ return ret;
pci_device_ptr = pfname ? virNetDevGetPCIDevice(pfname) :
virNetDevGetPCIDevice(ifname);
--
2.29.2
4 years, 1 month
[PATCH] virnetdevbandwidth: Don't generate burst outside of boundaries
by Michal Privoznik
When generating TC rules for domain's outbound traffic, Libvirt
will use the 'average' as the default for 'burst' - it's been
this way since the feature introduction in v0.9.4-rc1~22. The
reason is that 'average' considers 'burst' for policing. However,
when parsing its command line TC uses an unsigned int (with
overflow detection) to store the 'burst' size. This means, that
the upper limit for the value is UINT_MAX, well UINT_MAX / 1024
because we are putting the value in KiB onto the command line.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1912210
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/util/virnetdevbandwidth.c | 12 +++++++++++-
tests/virnetdevbandwidthtest.c | 15 +++++++++++++++
2 files changed, 26 insertions(+), 1 deletion(-)
diff --git a/src/util/virnetdevbandwidth.c b/src/util/virnetdevbandwidth.c
index 612e8f985c..77a97176b0 100644
--- a/src/util/virnetdevbandwidth.c
+++ b/src/util/virnetdevbandwidth.c
@@ -358,7 +358,17 @@ virNetDevBandwidthSet(const char *ifname,
if (rx) {
average = g_strdup_printf("%llukbps", rx->average);
- burst = g_strdup_printf("%llukb", rx->burst ? rx->burst : rx->average);
+
+ if (rx->burst) {
+ burst = g_strdup_printf("%llukb", rx->burst);
+ } else {
+ /* Internally, tc uses uint to store burst size (in bytes).
+ * Therefore, the largest value we can set is UINT_MAX bytes.
+ * We're outputting the vale in KiB though. */
+ unsigned long long avg = MIN(rx->average, UINT_MAX / 1024);
+
+ burst = g_strdup_printf("%llukb", avg);
+ }
virCommandFree(cmd);
cmd = virCommandNew(TC);
diff --git a/tests/virnetdevbandwidthtest.c b/tests/virnetdevbandwidthtest.c
index 2e76af3d0c..7c236f50f3 100644
--- a/tests/virnetdevbandwidthtest.c
+++ b/tests/virnetdevbandwidthtest.c
@@ -156,6 +156,21 @@ mymain(void)
TC " filter add dev eth0 parent ffff: protocol all u32 match u32 0 0 "
"police rate 5kbps burst 7kb mtu 64kb drop flowid :1\n"));
+ DO_TEST_SET(("<bandwidth>"
+ " <inbound average='4294967295'/>"
+ " <outbound average='4294967295'/>"
+ "</bandwidth>"),
+ (TC " qdisc del dev eth0 root\n"
+ TC " qdisc del dev eth0 ingress\n"
+ TC " qdisc add dev eth0 root handle 1: htb default 1\n"
+ TC " class add dev eth0 parent 1: classid 1:1 htb rate 4294967295kbps quantum 366503875\n"
+ TC " qdisc add dev eth0 parent 1:1 handle 2: sfq perturb 10\n"
+ TC " filter add dev eth0 parent 1:0 protocol all prio 1 handle 1 fw flowid 1\n"
+ TC " qdisc add dev eth0 ingress\n"
+ TC " filter add dev eth0 parent ffff: protocol all u32 match "
+ "u32 0 0 police rate 4294967295kbps burst 4194303kb mtu 64kb "
+ "drop flowid :1\n"));
+
return ret;
}
--
2.26.2
4 years, 1 month
[PATCH] qemu: don't raise error upon interface update without <frames/> for <rx/> in coalesce
by Kristina Hanicova
With this, incomplete XML without <frames/> for <rx/> in coalesce
won't raise error as before. It will leave the coalesce parameter
empty, thanks to passing it as a parameter and return an integer
to indicate error state - previously it returned pointer (or NULL
for both error and incomplete XML).
The code went through some refactoring:
* change of a condition
* addition of a parameter
* change of order, that allowed removal of VIR_FREE
* removal of redundant labels and variables
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1535930
Signed-off-by: Kristina Hanicova <khanicov(a)redhat.com>
---
src/conf/domain_conf.c | 25 ++++++++++---------------
1 file changed, 10 insertions(+), 15 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index b731744f04..798594f520 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -7516,11 +7516,11 @@ virDomainNetIPInfoParseXML(const char *source,
}
-static virNetDevCoalescePtr
+static int
virDomainNetDefCoalesceParseXML(xmlNodePtr node,
- xmlXPathContextPtr ctxt)
+ xmlXPathContextPtr ctxt,
+ virNetDevCoalescePtr *coalesce)
{
- virNetDevCoalescePtr ret = NULL;
VIR_XPATH_NODE_AUTORESTORE(ctxt)
unsigned long long tmp = 0;
g_autofree char *str = NULL;
@@ -7529,15 +7529,13 @@ virDomainNetDefCoalesceParseXML(xmlNodePtr node,
str = virXPathString("string(./rx/frames/@max)", ctxt);
if (!str)
- return NULL;
-
- ret = g_new0(virNetDevCoalesce, 1);
+ return 0;
if (virStrToLong_ullp(str, NULL, 10, &tmp) < 0) {
virReportError(VIR_ERR_XML_DETAIL,
_("cannot parse value '%s' for coalesce parameter"),
str);
- goto error;
+ return -1;
}
if (tmp > UINT32_MAX) {
@@ -7545,15 +7543,13 @@ virDomainNetDefCoalesceParseXML(xmlNodePtr node,
_("value '%llu' is too big for coalesce "
"parameter, maximum is '%lu'"),
tmp, (unsigned long) UINT32_MAX);
- goto error;
+ return -1;
}
- ret->rx_max_coalesced_frames = tmp;
- return ret;
+ *coalesce = g_new0(virNetDevCoalesce, 1);
+ (*coalesce)->rx_max_coalesced_frames = tmp;
- error:
- VIR_FREE(ret);
- return NULL;
+ return 0;
}
static void
@@ -11517,8 +11513,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
node = virXPathNode("./coalesce", ctxt);
if (node) {
- def->coalesce = virDomainNetDefCoalesceParseXML(node, ctxt);
- if (!def->coalesce)
+ if (virDomainNetDefCoalesceParseXML(node, ctxt, &def->coalesce) < 0)
goto error;
}
--
2.29.2
4 years, 1 month
[libvirt PATCH 0/2] docs: less docs for insecure SASL mechanisms
by Daniel P. Berrangé
GSSAPI and SCRAM-SHA-256 are the only two SASL mechanisms we
especially want people to be using. Even the latter is a little
questionable due to storing passwords in cleartext on the server.
Daniel P. Berrangé (2):
docs: convert auth page into RST format
docs: stop mentioning insecure / broken SASL mechanisms
docs/auth.html.in | 368 ---------------------------------------
docs/auth.rst | 323 ++++++++++++++++++++++++++++++++++
docs/meson.build | 2 +-
src/remote/libvirtd.sasl | 11 +-
4 files changed, 330 insertions(+), 374 deletions(-)
delete mode 100644 docs/auth.html.in
create mode 100644 docs/auth.rst
--
2.29.2
4 years, 1 month
[PATCH v2 0/3] Drop deprecated floppy config & bogus -drive if=T
by Markus Armbruster
v2:
* Rebased, straightforward conflict with commit f5d33dd51f
"hw/block/fdc: Remove the check_media_rate property" resolved
* PATCH 2: Commit message fixed [Kevin]
Markus Armbruster (3):
fdc: Drop deprecated floppy configuration
fdc: Inline fdctrl_connect_drives() into fdctrl_realize_common()
blockdev: Drop deprecated bogus -drive interface type
docs/system/deprecated.rst | 33 --
docs/system/removed-features.rst | 33 ++
include/sysemu/blockdev.h | 1 -
blockdev.c | 37 +-
hw/block/fdc.c | 73 +---
softmmu/vl.c | 8 +-
tests/qemu-iotests/172 | 31 +-
tests/qemu-iotests/172.out | 562 +------------------------------
8 files changed, 59 insertions(+), 719 deletions(-)
--
2.26.2
4 years, 1 month
[PATCH] virDevMapperGetTargetsImpl: Use correct length when copying into dm.name
by Michal Privoznik
For reasons unknown, when rewriting this code and dropping
libdevmapper I've mistakenly used incorrect length of dm.name. In
linux/dm-ioctl.h the dm_ioctl struct is defined as follows:
#define DM_NAME_LEN 128
struct dm_ioctl {
...
char name[DM_NAME_LEN]; /* device name */
...
};
However, when copying string into this member, DM_TABLE_DEPS was
used, which is defined as follows:
#define DM_TABLE_DEPS _IOWR(DM_IOCTL, DM_TABLE_DEPS_CMD, struct dm_ioctl)
After decryption, this results in the following size: 3241737483.
Fixes: 22494556542c676d1b9e7f1c1f2ea13ac17e1e3e
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/util/virdevmapper.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/util/virdevmapper.c b/src/util/virdevmapper.c
index fcb11e954f..2c4c2df999 100644
--- a/src/util/virdevmapper.c
+++ b/src/util/virdevmapper.c
@@ -240,7 +240,7 @@ virDevMapperGetTargetsImpl(int controlFD,
if (!(sanitizedPath = virDMSanitizepath(path)))
return 0;
- if (virStrcpy(dm.name, sanitizedPath, DM_TABLE_DEPS) < 0) {
+ if (virStrcpy(dm.name, sanitizedPath, DM_NAME_LEN) < 0) {
virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
_("Resolved device mapper name too long"));
return -1;
--
2.26.2
4 years, 1 month
[PATCH] meson: tools: depend on keycode generated sources
by Roman Bogorodskiy
Tools depend on keycode generated sources, so declare that as an
explicit dependency, otherwise it might fail with:
../tools/virsh-completer-domain.c:35:10: fatal error: 'virkeynametable_linux.h' file not found
^~~~~~~~~~~~~~~~~~~~~~~~~
Signed-off-by: Roman Bogorodskiy <bogorodskiy(a)gmail.com>
---
I noticed that build failure on FreeBSD 11.x. For some reason, it
doesn't show up on newer versions. This is strange, but as the fix appears
to be straight-forward, I didn't spend much time figuring out the
reason.
In case you're interested, a full failing build log is here:
https://people.freebsd.org/~novel/misc/libvirt-7.1.0.log
It doesn't seem to even try to generate these files. When I inspected
filesystem state, these files were missing.
src/util/meson.build | 2 ++
tools/meson.build | 1 +
2 files changed, 3 insertions(+)
diff --git a/src/util/meson.build b/src/util/meson.build
index 0080825bd0..cd3fe18524 100644
--- a/src/util/meson.build
+++ b/src/util/meson.build
@@ -168,6 +168,8 @@ foreach name : keyname_list
)
endforeach
+keycode_dep = declare_dependency(sources: keycode_gen_sources)
+
io_helper_sources = [
'iohelper.c',
]
diff --git a/tools/meson.build b/tools/meson.build
index b8c6802f0a..42dc609439 100644
--- a/tools/meson.build
+++ b/tools/meson.build
@@ -186,6 +186,7 @@ executable(
tools_dep,
readline_dep,
thread_dep,
+ keycode_dep,
],
link_args: [
coverage_flags,
--
2.30.0
4 years, 1 month
[PATCH] docs: Document qemu.conf locations
by Michal Privoznik
Surprisingly, we never documented the relationship between
connection URI and the location of qemu.conf. Users might wonder
what qemu.conf is loaded when they are connecting to the session
daemon or embed URI. And what to do if the file doesn't exist for
the URI they're using.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
docs/drvqemu.html.in | 39 +++++++++++++++++++++++++++++++++++++
docs/manpages/libvirtd.rst | 20 +++++++++++++++++++
docs/manpages/virtqemud.rst | 22 +++++++++++++++++++++
3 files changed, 81 insertions(+)
diff --git a/docs/drvqemu.html.in b/docs/drvqemu.html.in
index 31d3fee213..a784f5768c 100644
--- a/docs/drvqemu.html.in
+++ b/docs/drvqemu.html.in
@@ -162,6 +162,45 @@ qemu+ssh://root@example.com/system (remote access, SSH tunnelled)
of libvirt, once QMP processing moves to a dedicated thread.
</p>
+ <h2><a id="configFiles">Location of configuration files</a></h2>
+
+ <p>
+ The QEMU driver comes with sane default values. However, during its
+ initialization it reads a configuration file which offers system
+ administrator or an user to override some of that default. The location
+ of the file depends on the connection URI, as follows:
+ </p>
+
+ <table>
+ <tr>
+ <td><code>qemu:///system</code></td>
+ <td><code>/etc/libvirt/qemu.conf</code></td>
+ </tr>
+ <tr>
+ <td><code>qemu:///session</code></td>
+ <td><code>$XDG_CONFIG_HOME/libvirt/qemu.conf</code></td>
+ </tr>
+ <tr>
+ <td><code>qemu:///embed</code></td>
+ <td><code>$rootdir/etc/qemu.conf</code></td>
+ </tr>
+ </table>
+
+ <p>
+ If <code>$XDG_CONFIG_HOME</code> is not set in the environment, it
+ defaults to <code>$HOME/.config</code>. For the embed URI the
+ <code>$rootdir</code> represents the specified root directory from
+ the connection URI.
+ </p>
+
+ <p>
+ Please note, that it is very likely that the only qemu.conf file that
+ will exist after installing libvirt is the
+ <code>/etc/libvirt/qemu.conf</code>, if users of the session daemon or
+ the embed driver want to override a built in value, then they need to
+ create the file before connecting to the respective URI.
+ </p>
+
<h2><a id="security">Driver security architecture</a></h2>
<p>
diff --git a/docs/manpages/libvirtd.rst b/docs/manpages/libvirtd.rst
index ed591f4778..6545dc58d3 100644
--- a/docs/manpages/libvirtd.rst
+++ b/docs/manpages/libvirtd.rst
@@ -136,6 +136,16 @@ When run as *root*
The default configuration file used by ``libvirtd``, unless overridden on the
command line using the ``-f`` | ``--config`` option.
+If QEMU driver is installed, then the following file is also read:
+
+* ``@SYSCONFDIR(a)/libvirt/qemu.conf``
+
+This file contains various knobs and default values for virtual machines
+created within QEMU driver, and offers a way to override the built in defaults,
+for instance (but not limited to): paths to various supplementary binaries, TLS
+certificates location, graphical consoles configuration and others. Location of
+this file can't be overridden by any command line switch.
+
* ``@RUNSTATEDIR@/libvirt/libvirt-sock``
* ``@RUNSTATEDIR@/libvirt/libvirt-sock-ro``
@@ -166,6 +176,16 @@ When run as *non-root*
The default configuration file used by ``libvirtd``, unless overridden on the
command line using the ``-f``|``--config`` option.
+If QEMU driver is installed, then the following file is also read:
+
+* ``$XDG_CONFIG_HOME/libvirt/qemu.conf``
+
+If the file exists, it can contain various knobs and default values for virtual
+machines created within QEMU driver, and offers a way to override the built in
+defaults, for instance (but not limited to): paths to various supplementary
+binaries, TLS certificates location, graphical consoles configuration and
+others. Location of this file can't be overridden by any command line switch.
+
* ``$XDG_RUNTIME_DIR/libvirt/libvirt-sock``
The socket ``libvirtd`` will use.
diff --git a/docs/manpages/virtqemud.rst b/docs/manpages/virtqemud.rst
index fbcc6e45fa..d82d09ee61 100644
--- a/docs/manpages/virtqemud.rst
+++ b/docs/manpages/virtqemud.rst
@@ -111,6 +111,17 @@ When run as *root*
The default configuration file used by ``virtqemud``, unless overridden on the
command line using the ``-f`` | ``--config`` option.
+In addition to the default configuration file, ``virtqemud`` reads
+configuration for the qemu driver from:
+
+* ``@SYSCONFDIR(a)/libvirt/qemu.conf``
+
+This file contains various knobs and default values for virtual machines
+created within QEMU driver, and offers a way to override the built in defaults,
+for instance (but not limited to): paths to various supplementary binaries, TLS
+certificates location, graphical consoles configuration and others. Location of
+this file can't be overridden by any command line switch.
+
* ``@RUNSTATEDIR@/libvirt/virtqemud-sock``
* ``@RUNSTATEDIR@/libvirt/virtqemud-sock-ro``
* ``@RUNSTATEDIR@/libvirt/virtqemud-admin-sock``
@@ -132,6 +143,17 @@ When run as *non-root*
The default configuration file used by ``virtqemud``, unless overridden on the
command line using the ``-f``|``--config`` option.
+In addition to the default configuration file, ``virtqemud`` reads
+configuration for the qemu driver from:
+
+* ``$XDG_CONFIG_HOME/libvirt/qemu.conf``
+
+If the file exists, it can contain various knobs and default values for virtual
+machines created within QEMU driver, and offers a way to override the built in
+defaults, for instance (but not limited to): paths to various supplementary
+binaries, TLS certificates location, graphical consoles configuration and
+others. Location of this file can't be overridden by any command line switch.
+
* ``$XDG_RUNTIME_DIR/libvirt/virtqemud-sock``
* ``$XDG_RUNTIME_DIR/libvirt/virtqemud-admin-sock``
--
2.26.2
4 years, 1 month
[PATCH] virFirewallApply: Fix possible NULL dereference on error
by Peter Krempa
Commit bbc25f0d03d443efd35381463efc81b01cb6ae96 juggled around some
error reporting. Unfortunately virFirewallApply tries to report the
errno stored in the firewall object and we'd try to do that when the
firewall object is NULL too. Report EINVAL if 'firewall' is NULL.
Found by Coverity.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/util/virfirewall.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c
index c1b7d2268b..0dc0cecd53 100644
--- a/src/util/virfirewall.c
+++ b/src/util/virfirewall.c
@@ -766,8 +766,12 @@ virFirewallApply(virFirewallPtr firewall)
goto cleanup;
}
if (!firewall || firewall->err) {
- virReportSystemError(firewall->err, "%s",
- _("Unable to create rule"));
+ int err = EINVAL;
+
+ if (firewall)
+ err = firewall->err;
+
+ virReportSystemError(err, "%s", _("Unable to create rule"));
goto cleanup;
}
--
2.29.2
4 years, 1 month
