Commit Graph

37 Commits

Author SHA1 Message Date
Michal Privoznik
b7d4e6b67e lib: Replace VIR_AUTOSTRINGLIST with GStrv
Glib provides g_auto(GStrv) which is in-place replacement of our
VIR_AUTOSTRINGLIST.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
2020-12-02 15:43:07 +01:00
Laine Stump
b19863640d util: call iptables directly rather than via firewalld
When libvirt added support for firewalld, we were unable to use
firewalld's higher level rules, because they weren't detailed enough
and could not be applied to the iptables FORWARD or OUTPUT chains
(only to the INPUT chain). Instead we changed our code so that rather
than running the iptables/ip6tables/ebtables binaries ourselves, we
would send these commands to firewalld as "passthrough commands", and
firewalld would run the appropriate program on our behalf.

This was done under the assumption that firewalld was somehow tracking
all these rules, and that this tracking was benefitting proper
operation of firewalld and the system in general.

Several years later this came up in a discussion on IRC, and we
learned from the firewalld developers that, in fact, adding iptables
and ebtables rules with firewalld's passthrough commands actually has
*no* advantage; firewalld doesn't keep track of these rules in any
way, and doesn't use them to tailor the construction of its own rules.

Meanwhile, users have been complaining for some time that whenever
firewalld is restarted on a system with libvirt virtual networks
and/or nwfilter rules active, the system logs would be flooded with
warning messages whining that [lots of different rules] could not be
deleted because they didn't exist. For example:

firewalld[3536040]: WARNING: COMMAND_FAILED:
  '/usr/sbin/iptables -w10 -w --table filter --delete LIBVIRT_OUT
  --out-interface virbr4 --protocol udp --destination-port 68
  --jump ACCEPT' failed: iptables: Bad rule
  (does a matching rule exist in that chain?).

(See https://bugzilla.redhat.com/1790837 for many more examples and a
discussion)

Note that these messages are created by iptables, but are logged by
firewalld - when an iptables/ebtables command fails, firewalld grabs
whatever is in stderr of the program, and spits it out to the system
log as a warning. We've requested that firewalld not do this (and
instead leave it up to the calling application to do the appropriate
logging), but this request has been respectfully denied.

But combining the two problems above ( 1) firewalld doesn't do
anything useful when you use it as a proxy to add/remove iptables
rules, 2) firewalld often insists on logging lots of
annoying/misleading/useless "error" messages when you use it as a
proxy to remove iptables rules that don't already exist), leads to a
solution - simply stop using firewalld to add and remove iptables
rules. Instead, exec iptables/ip6tables/ebtables directly in the same
way we do when firewalld isn't active.

We still need to keep track of whether or not firewalld is active, as
there are some things that must be done, e.g. we need to add some
actual firewalld rules in the firewalld "libvirt" zone, and we need to
take notice when firewalld restarts, so that we can reload all our
rules.

This patch doesn't remove the infrastructure that allows having
different firewall backends that perform their functions in different
ways, as that will very possibly come in handy in the future when we
want to have an nftables direct backend, and possibly a "pure"
firewalld backend (now that firewalld supports more complex rules, and
can add those rules to the FORWARD and OUTPUT chains). Instead, it
just changes the action when the selected backend is "firewalld" so
that it adds rules directly rather than through firewalld, while
leaving as much of the existing code intact as possible.

In order for tests to still pass, virfirewalltest also had to be
modified to behave in a different way (i.e. by capturing the generated
commandline as it does for the DIRECT backend, rather than capturing
dbus messages using a mocked dbus API).

Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
2020-11-24 14:22:06 -05:00
Laine Stump
e9693502fb tests: fix iptables test case commandline options in virfirewalltest.c
This test was created with all the commandlines erroneously having
"--source-host", which is not a valid iptables option. The correct
name for the option is "--source". However, since the test is just
checking that the generated commandline matches what we told it to
generate (and never actually runs iptables, as that would be a "Really
Bad Idea"(tm)), the test has always succeeded. I only found it because
I made a change to the code that caused the test to incorrectly try to
run iptables during the test, and the error message I received was
"odd" (it complained about the bad option, rather than complaining
that I had insufficient privilege to run the command).

Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
2020-11-24 14:21:41 -05:00
Laine Stump
0a867cd895 util/tests: enable locking on iptables/ebtables commandlines by default
iptables and ip6tables have had a "-w" commandline option to grab a
systemwide lock that prevents two iptables invocations from modifying
the iptables chains since 2013 (upstream commit 93587a04 in
iptables-1.4.20).  Similarly, ebtables has had a "--concurrent"
commandline option for the same purpose since 2011 (in the upstream
ebtables commit f9b4bcb93, which was present in ebtables-2.0.10.4).

Libvirt added code to conditionally use the commandline option for
iptables/ip6tables in upstream commit ba95426d6f (libvirt-1.2.0,
November 2013), and for ebtables in upstream commit dc33e6e4a5
(libvirt-1.2.11, November 2014) (the latter actually *re*-added the
locking for iptables/ip6tables, as it had accidentally been removed
during a refactor of firewall code in the interim).

I say "conditionally" because a check was made during firewall module
initialization that tried executing a test command with the
-w/--concurrent option, and only continued using it for actual
commands if that test command completed successfully. At the time the
code was added this was a reasonable thing to do, as it had been less
than a year since introduction of -w to iptables, so many distros
supported by libvirt were still using iptables (and possibly even
ebtables) versions too old to have the new commandline options.

It is now 2020, and as far as I can discern from repology.org (and
manually examining a RHEL7.9 system), every version of every distro
that is supported by libvirt now uses new enough versions of both
iptables and ebtables that they all have support for -w/--concurrent.
That means we can finally remove the conditional code and simply
always use them.

Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
2020-11-24 14:21:29 -05:00
Laine Stump
e66451f685 util/tests: enable locking on iptables/ebtables commandlines in unit tests
All the unit tests that use iptables/ip6tables/ebtables have been
written to omit the locking/exclusive use primitive on the generated
commandlines. Even though none of the tests actually execute those
commands (and so it doesn't matter for purposes of the test whether or
not the commands support these options), it still made sense when some
systems had these locking options and some didn't.

We are now at a point where every supported Linux distro has supported
the locking options on these commands for quite a long time, and are
going to make their use non-optional. As a first step, this patch uses
the virFirewallSetLockOverride() function, which is called at the
beginning of all firewall-related tests, to set all the bools
controlling whether or not the locking options are used to true. This
means that all the test cases must be updated to include the proper
locking option in their commandlines.

The change to make actual execs of the commands unconditionally use
the locking option will be in an upcoming patch - this one affects
only the unit tests.

Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
2020-11-24 14:21:08 -05:00
Laine Stump
28a3dedddd util: fix typo in VIR_MOCK_WRAP_RET_ARGS()
When virfirewalltest.c was first written in commit 3a0ca7de51 (March
2013), a conditional accidentally tested for "ipv4" instead of
"ipv6". Since the file ended up only testing ipv4 rules, this has
never made any difference in practice, but I'm making some other
changes in this file and just couldn't let it stand :-)

Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
2020-11-24 14:20:32 -05:00
Michal Privoznik
053beeb04d virfirewalltest: Don't duplicate string when adding it onto stringlist
In our wrapper of g_dbus_connection_call_sync() in
virfirewalltest a string is duplicated and added onto a
virStringList. This leads to a memory leak because
virStringListAdd() duplicates the string itself.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
2020-10-02 12:48:55 +02:00
Pavel Hrdina
48622bb563 tests: fix incorrect free of GVariant in our GLib mock functions
GLib implementation of g_dbus_connection_call_sync() calls
g_variant_ref_sink() on the passed @parameters to make sure they have
proper reference. If the original reference is floating the
g_dbus_connection_call_sync() consumes it, but if it's normal reference
it will just add another one.

Our mock functions were only freeing the @parameters which is incorrect
and doesn't reflect how the real implementation works.

Reported-by: Cole Robinson <crobinso@redhat.com>
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
2020-10-02 12:43:15 +02:00
Pavel Hrdina
10cf523a8d src/util/virfirewalld: convert to use GLib DBus
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
2020-09-17 18:20:01 +02:00
Laine Stump
cf1ec5daac use g_autoptr() for all usages of virFirewallNew/Free
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
2020-07-08 16:36:19 -04:00
Laine Stump
25c23b95b6 tests: use g_auto for all virBuffers
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
2020-07-08 16:34:09 -04:00
Peter Krempa
205d6a2af7 util: buffer: Remove virBufferError
The function now does not return an error so we can drop it fully.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
2019-10-24 19:35:34 +02:00
Ján Tomko
29b1e859e3 tests: use g_strdup instead of VIR_STRDUP
Replace all occurrences of
  if (VIR_STRDUP(a, b) < 0)
     /* effectively dead code */
with:
  a = g_strdup(b);

Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
2019-10-21 12:51:59 +02:00
Ján Tomko
67e72053c1 Use G_N_ELEMENTS instead of ARRAY_CARDINALITY
Prefer the GLib version of the macro.

Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
2019-10-15 16:14:19 +02:00
Ján Tomko
0d94f02455 tests: use G_GNUC_UNUSED
Use G_GNUC_UNUSED from GLib instead of ATTRIBUTE_UNUSED.

Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
2019-10-15 11:25:25 +02:00
Daniel P. Berrangé
2c52ecd960 util: purge all code for testing OOM handling
The OOM handling requires special build time options which we never
enable in our CI. Even once enabled the tests are incredibly slow and
typically require manual inspection of the results to weed out false
positives.

Since there was previous agreement to switch to abort on OOM in libvirt
code, there's no point continuing to keep the unused OOM testing code.

Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
2019-09-13 10:05:17 +01:00
Michal Privoznik
5ae24a13c7 Revert "dbus: correctly build reply message"
This reverts commit 39dded7bb6.

This commit broke virpolkittest on Ubuntu 18 which has an old
dbus (v1.12.2). Any other distro with the recent one works
(v1.12.16) which hints its a bug in dbus somewhere. Revert the
commit to stop tickling it.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
2019-09-10 09:47:13 +02:00
Marc-André Lureau
39dded7bb6 dbus: correctly build reply message
dbus_message_new() does not construct correct replies by itself, it is
recommended to use dbus_message_new_method_return() instead.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
2019-09-06 12:47:46 +02:00
Roman Bolshakov
fde361083d tests: Add lib- prefix to all mocks
In preparation libtool "-module" flag removal, add lib prefix to all
mock shared objects.

While at it, introduce VIR_TEST_MOCK macros that makes path out of mock
name to be used with VIR_TEST_PRELOAD or VIR_TEST_MAIN_PRELOAD.  That,
hopefully, improves readability, reduces line length and allows to
tailor VIR_TEST_MOCK for specific platform if it has shared library
suffix different from ".so".

Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
2019-08-23 11:26:26 +01:00
Laine Stump
d8393b56e2 util: move all firewalld-specific stuff into its own files
In preparation for adding several other firewalld-specific functions,
separate the code that's unique to firewalld from the more-generic
"firewall" file.

Signed-off-by: Laine Stump <laine@laine.org>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
2019-02-01 12:08:37 -05:00
Daniel P. Berrangé
b092a4357d util: pass layer into firewall query callback
Some of the query callbacks want to know the firewall layer that was
being used for triggering the query to avoid duplicating that data.

Reviewed-by: Laine Stump <laine@laine.org>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
2019-01-29 13:35:58 +00:00
Daniel P. Berrangé
568a417224 Enforce a standard header file guard symbol name
Require that all headers are guarded by a symbol named

  LIBVIRT_$FILENAME

where $FILENAME is the uppercased filename, with all characters
outside a-z changed into '_'.

Note we do not use a leading __ because that is technically a
namespace reserved for the toolchain.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
2018-12-14 10:47:13 +00:00
Daniel P. Berrangé
4cfd709021 Fix many mistakes & inconsistencies in header file layout
This introduces a syntax-check script that validates header files use a
common layout:

  /*
   ...copyright header...
   */
  <one blank line>
  #ifndef SYMBOL
  # define SYMBOL
  ....content....
  #endif /* SYMBOL */

For any file ending priv.h, before the #ifndef, we will require a
guard to prevent bogus imports:

  #ifndef SYMBOL_ALLOW
  # error ....
  #endif /* SYMBOL_ALLOW */
  <one blank line>

The many mistakes this script identifies are then fixed.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
2018-12-14 10:46:53 +00:00
Daniel P. Berrangé
600462834f Remove all Author(s): lines from source file headers
In many files there are header comments that contain an Author:
statement, supposedly reflecting who originally wrote the code.
In a large collaborative project like libvirt, any non-trivial
file will have been modified by a large number of different
contributors. IOW, the Author: comments are quickly out of date,
omitting people who have made significant contribitions.

In some places Author: lines have been added despite the person
merely being responsible for creating the file by moving existing
code out of another file. IOW, the Author: lines give an incorrect
record of authorship.

With this all in mind, the comments are useless as a means to identify
who to talk to about code in a particular file. Contributors will always
be better off using 'git log' and 'git blame' if they need to  find the
author of a particular bit of code.

This commit thus deletes all Author: comments from the source and adds
a rule to prevent them reappearing.

The Copyright headers are similarly misleading and inaccurate, however,
we cannot delete these as they have legal meaning, despite being largely
inaccurate. In addition only the copyright holder is permitted to change
their respective copyright statement.

Reviewed-by: Erik Skultety <eskultet@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
2018-12-13 16:08:38 +00:00
Yuri Chornoivan
e5c1fbca24 Fix minor typos in messages and docs
Signed-off-by: Yuri Chornoivan <yurchor@ukr.net>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Signed-off-by: Ján Tomko <jtomko@redhat.com>
2018-12-05 10:39:54 +01:00
Andrea Bolognani
3e7db8d3e8 Remove backslash alignment attempts
Right-aligning backslashes when defining macros or using complex
commands in Makefiles looks cute, but as soon as any changes is
required to the code you end up with either distractingly broken
alignment or unnecessarily big diffs where most of the changes
are just pushing all backslashes a few characters to one side.

Generated using

  $ git grep -El '[[:blank:]][[:blank:]]\\$' | \
    grep -E '*\.([chx]|am|mk)$$' | \
    while read f; do \
      sed -Ei 's/[[:blank:]]*[[:blank:]]\\$/ \\/g' "$f"; \
    done

Signed-off-by: Andrea Bolognani <abologna@redhat.com>
2017-11-03 13:24:12 +01:00
Daniel P. Berrange
6f37cb80df Prevent test failures with ebtables/iptables/ip6tables are missing
When running tests in a restricted container (as opposed to a full
OS install), we can't assume ebtables/iptbles/ip6tables are going
to be installed. We must check this and mark the tests as skipped.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
2017-04-19 10:51:51 +01:00
Andrea Bolognani
4ceac4bf29 tests: Rename VIRT_TEST_* macros to VIR_TEST_*
We use the "vir" prefix pretty consistently in our
APIs, both external and internal, which made these
macros stood out.
2017-04-04 17:30:03 +02:00
Tomáš Ryšavý
8a48cf6efa tests: Rename virtTest00MActive to virTest00MActive.
This function doesn't follow our convention of naming functions.
2016-06-08 11:23:12 -04:00
Tomáš Ryšavý
062ea148a0 Rename virtTestDifference to virTestDifference.
This function doesn't follow our convention of naming functions.
2016-06-08 11:23:12 -04:00
Tomáš Ryšavý
cd7dd1508d tests: Rename virtTestRun to virTestRun.
This function doesn't follow our convention of naming functions.
2016-06-08 11:23:12 -04:00
Andrea Bolognani
92ec2e5e9b tests: Rename virmockdbus -> virdbusmock for consistency
All mock libraries were called vir*mock except for this one; now
the naming is consistent across the board.
2016-02-11 18:24:32 +01:00
Michal Privoznik
862298a2e7 dbus: Don't unref NULL messages
Apparently we are not the only ones with dumb free functions
because dbus_message_unref() does not accept NULL either. But if
I were to vote, this one is even more evil. Instead of returning
an error just like we do it immediately dereference any pointer
passed and thus crash you app. Well done DBus!

  Program received signal SIGSEGV, Segmentation fault.
  [Switching to Thread 0x7f878ebda700 (LWP 31264)]
  0x00007f87be4016e5 in ?? () from /usr/lib64/libdbus-1.so.3
  (gdb) bt
  #0  0x00007f87be4016e5 in ?? () from /usr/lib64/libdbus-1.so.3
  #1  0x00007f87be3f004e in dbus_message_unref () from /usr/lib64/libdbus-1.so.3
  #2  0x00007f87bf6ecf95 in virSystemdGetMachineNameByPID (pid=9849) at util/virsystemd.c:228
  #3  0x00007f879761bd4d in qemuConnectCgroup (driver=0x7f87600a32a0, vm=0x7f87600c7550) at qemu/qemu_cgroup.c:909
  #4  0x00007f87976386b7 in qemuProcessReconnect (opaque=0x7f87600db840) at qemu/qemu_process.c:3386
  #5  0x00007f87bf6edfff in virThreadHelper (data=0x7f87600d5580) at util/virthread.c:206
  #6  0x00007f87bb602334 in start_thread (arg=0x7f878ebda700) at pthread_create.c:333
  #7  0x00007f87bb3481bd in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:109
  (gdb) frame 2
  #2  0x00007f87bf6ecf95 in virSystemdGetMachineNameByPID (pid=9849) at util/virsystemd.c:228
  228         dbus_message_unref(reply);
  (gdb) p reply
  $1 = (DBusMessage *) 0x0

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
2016-02-11 11:35:02 +01:00
Martin Kletzander
42dc7a471d tests: Set up two more overrides for root builders
There are two more places after commit 3865941b that need to be adapted
in order to get rid of some test failures when building as root.

Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
2014-12-23 06:10:55 +01:00
Daniel P. Berrange
47ffd5e8fd tests: Add more test suite mock helpers
Rename the VIR_MOCK_IMPL* macros to VIR_MOCK_WRAP*
and add new VIR_MOCK_IMPL macros which let you directly
implement overrides in the preloaded source.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Signed-off-by: Jim Fehlig <jfehlig@suse.com>
2014-09-11 15:40:50 -06:00
Roman Bogorodskiy
064f49a050 tests: skip virfirewalltest on non-Linux systems
Currently firewalling is supported on Linux only, so skip the
virfirewalltest on other platforms.
2014-05-02 16:19:34 +04:00
Daniel P. Berrange
3a0ca7de51 Introduce an object for managing firewall rulesets
The network and nwfilter drivers both have a need to update
firewall rules. The currently share no code for interacting
with iptables / firewalld. The nwfilter driver is fairly
tied to the concept of creating shell scripts to execute
which makes it very hard to port to talk to firewalld via
DBus APIs.

This patch introduces a virFirewallPtr object which is able
to represent a complete sequence of rule changes, with the
ability to have multiple transactional checkpoints with
rollbacks. By formally separating the definition of the rules
to be applied from the mechanism used to apply them, it is
also possible to write a firewall engine that uses firewalld
DBus APIs natively instead of via the slow firewalld-cmd.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
2014-04-25 15:44:09 +01:00