Peter Krempa [Wed, 3 Jul 2019 06:25:15 +0000 (08:25 +0200)]
tests: Remove xml2sexprtest
The test was the only place calling 'xenFormatSxpr'. Drop it as there
are no other users of that code since we've dropped xend support in
commit 1dac5fbbbb06a03.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> Reviewed-by: Ján Tomko <jtomko@redhat.com>
The docs talked about an active snapshot when they meant an active
domain; they also claimed the flag was a no-op for hypervisors with no
snapshot metadata even though the flag is currently rejected as
unrecognized for hypervisors with no snapshot support at all. A later
patch may teach more drivers to ignore the flag as a no-op, but that
shouldn't conflict with the wording chosen here (since a new client
talking to an old server still runs into the same issue, even if a
newer server becomes more tolerant).
Reported-by: Peter Krempa <pkrempa@redhat.com> Signed-off-by: Eric Blake <eblake@redhat.com> Acked-by: Peter Krempa <pkrempa@redhat.com>
libvirt: remove link to outdated mailing list post
The virConnectGetType() method has an unfortunate signature, returning a
static string that must not be freed by the caller. The remote driver,
however, gets this string dynamically over an RPC call, which raised a
design discussion on the mailing list. Eventually the problem was
resolved by having the remote driver cache the returned string
internally and free it when the connection was closed.
The link to the mailing list is thus talking about a problem that does
not actually exist in the final implementation, and at best serves to
confuse the reader into thinking there might be a memory leak.
Reviewed-by: Ján Tomko <jtomko@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
rpc: ensure thread safe initialization of SASL library
Neither the sasl_client_init or sasl_server_init methods are even
remotely threadsafe. They do a bunch of one-time initialization and
merely use a simple integer counter to avoid repeated work, not even
using atomic increment/reads on the counter. This can easily race in a
threaded program. Protect the calls using a virOnce initializer function
which is guaranteed threadsafe at least from libvirt's POV.
If the application using libvirt also uses another library that makes
use of SASL then the race still exists. It is impossible to fix that
fully except in SASL code itself.
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Commit 44b8df4cb4b3 introduced a check for yajl.pc that is
extremely similar to the one we already had in place for
readline.pc - so similar, in fact, that it's still looking
for readline.pc instead of yajl.pc :)
Signed-off-by: Andrea Bolognani <abologna@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Eric Blake [Wed, 12 Jun 2019 15:48:15 +0000 (10:48 -0500)]
snapshot: Refactor test to utilize virDomainMoment more
Similar to commit a487890d for qemu, a little bit of refactoring in
the snapshot delete code will make it easier to reuse functionality
for checkpoints.
Signed-off-by: Eric Blake <eblake@redhat.com> Acked-by: Peter Krempa <pkrempa@redhat.com>
remote: conditionally declare tty variable for non-Win32 platforms
The 'tty' variable is only used on Win32. Instead of just annotating it
with ATTRIBUTE_UNUSED, make its declaration conditional on WIN32 so that
it is clear why it is not used.
Reviewed-by: Fabiano Fidêncio <fidencio@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
remote: stop declaring variables in the middle of a function
The doRemoteOpen method was a little unusual in declaring a bunch of
local variables in the middle of the function. Move them to the top as
it is normal libvirt style.
Reviewed-by: Fabiano Fidêncio <fidencio@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
tests: fix mocking of virFileGetXAttrQuiet on FreeBSD
The qemusecuritytest is failing on FreeBSD 11/12, reporting that files
are not correctly restored. Debugging code printfs show that the
virFileGetXAttrQuiet mock is returning 0, but the virFileGetXAttr
function is seeing -1 as the return value.
Essentially there appears to be some kind of optimization between the
real virFileGetXAttrQuiet and the real virFileGetXAttr, which breaks
when we mock virFileGetXAttrQuiet. Rather than trying to figure out
how to avoid this, it is simpler to just mock virFileGetXAttr too
since it is very short code.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
There are probably more situations where they could be taken
advantage of, but these are very obvious scenarios because we
either manage to get rid of a bunch of explicit capabilities,
or we make a bunch of related test cases all use the macros
by switching the only odd one out.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> Reviewed-by: Ján Tomko <jtomko@redhat.com>
Right now we have macros such as DO_TEST_CAPS_LATEST_PARSE_ERROR()
and DO_TEST_CAPS_ARCH_VER(), but there is no concise way to say
"using this version of QEMU on this architecture will result in a
failure".
Ilias Stamatis [Tue, 25 Jun 2019 21:58:07 +0000 (23:58 +0200)]
test_driver: Implement virDomainGetFSInfo
Always return / and /boot as the mount points imitating the default
Fedora installation. Use the first disk found, otherwise if no disk
device of type VIR_DOMAIN_DISK_DEVICE_DISK is present, return 0 mount
points.
Reviewed-by: Erik Skultety <eskultet@redhat.com> Signed-off-by: Ilias Stamatis <stamatis.iliass@gmail.com>
qemu: Remove a redundant function call from qemuDomainGetPerfEvents
Calling virDomainObjUpdateModificationImpact directly inside the
function body is redundant, since the same function call is embedded
into virDomainObjGetOneDef.
Signed-off-by: Ilias Stamatis <stamatis.iliass@gmail.com> Reviewed-by: Erik Skultety <eskultet@redhat.com>
Michal Privoznik [Tue, 25 Jun 2019 11:21:39 +0000 (13:21 +0200)]
qemu: De-duplicate some path definitions
There are some paths (e.g. /dev/vfio/vfio or /dev/mapper/control)
which are defined in qemu_domain.c and then in qemu_cgroup.c
again. This is suboptimal. Let's move paths into qemu_domain.h and
drop duplicate definitions.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Andrea Bolognani <abologna@redhat.com>
In my review of 89320788ac4 I've simplified assigning disk errors
too much as the code I've changed it to will set
VIR_DOMAIN_DISK_ERROR_NONE. This is in contradiction with our
documentation which specifies that disks with no errors are not
reported.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Michal Privoznik [Tue, 14 May 2019 15:05:45 +0000 (17:05 +0200)]
test_driver: Don't access @vm after it was set to NULL
If something goes wrong in testDomainGetDiskErrors() then we try
to free any strings that were previously allocated in return
array. Problem is, in my review of original patch (89320788ac4)
I've mistakenly did some changes which result in possible NULL
dereference (@vm is set to NULL as the first thing under cleanup
label).
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Now that all the reported bugs are fixed let's turn the feature
back on.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Cole Robinson <crobinso@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Michal Privoznik [Mon, 25 Mar 2019 16:02:44 +0000 (17:02 +0100)]
qemu: Move image security metadata on snapshot activity
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Cole Robinson <crobinso@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Cole Robinson <crobinso@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Cole Robinson <crobinso@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Cole Robinson <crobinso@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
A simple helper function that would be used from DAC and SELinux
drivers.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Cole Robinson <crobinso@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
The purpose of this API is to allow caller move XATTRs (or remove
them) from one file to another. This will be needed when moving
top level of disk chain (either by introducing new HEAD or
removing it).
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Cole Robinson <crobinso@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Michal Privoznik [Fri, 15 Feb 2019 10:37:03 +0000 (11:37 +0100)]
security: Don't remember owner for shared resources
This effectively reverts d7420430ce6 and adds new code.
Here is the problem: Imagine a file X that is to be shared
between two domains as a disk. Let the first domain (vm1) have
seclabel remembering turned on and the other (vm2) has it turned
off. Assume that both domains will run under the same user, but
the original owner of X is different (i.e. trying to access X
without relabelling leads to EPERM).
Let's start vm1 first. This will cause X to be relabelled and to
gain new attributes:
When vm2 is started, X will again be relabelled, but since the
new label is the same as X already has (because of vm1) nothing
changes and vm1 and vm2 can access X just fine. Note that no
XATTR is changed (especially the refcounter keeps its value of 1)
because the vm2 domain has the feature turned off.
Now, vm1 is shut off and vm2 continues running. In seclabel
restore process we would get to X and since its refcounter is 1
we would restore the $originalOwner on it. But this is unsafe to
do because vm2 is still using X (remember the assumption that
$originalOwner and vm2's seclabel are distinct?).
The problem is that refcounter stored in XATTRs doesn't reflect
the actual times a resource is in use. Since I don't see any easy
way around it let's just not store original owner on shared
resources. Shared resource in world of domain disks is:
- whole backing chain but the top layer,
- read only disk (we don't require CDROM to be explicitly
marked as shareable),
- disk marked as shareable.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Michal Privoznik [Thu, 18 Apr 2019 09:05:18 +0000 (11:05 +0200)]
qemusecuritymock: Allow some paths to be not restored
Some paths will not be restored. Because we can't possibly know
if they are still in use or not. Reflect this in the test so that
we can test more domains. Also see next commit for more detailed
explanation.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Michal Privoznik [Fri, 15 Feb 2019 14:36:06 +0000 (15:36 +0100)]
security_selinux: Allow caller to suppress owner remembering
Just like previous commit allowed to enable or disable owner
remembering for each individual path, do the same for SELinux
driver. This is going to be needed in the next commit.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Cole Robinson <crobinso@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Michal Privoznik [Fri, 15 Feb 2019 10:20:19 +0000 (11:20 +0100)]
security_dac: Allow caller to suppress owner remembering
One caller in particular (virSecurityDACSetImageLabelInternal)
will want to have the feature turned on only in some cases.
Introduce @remember member to _virSecurityDACChownItem to track
whether caller wants to do owner remembering or not.
The actual remembering is then enabled if both caller wanted it
and the feature is turned on in the config file.
Technically, we could skip over paths that don't have remember
enabled when creating a list of paths to lock. We won't touch
their XATTRs after all. Well, I rather play it safe and keep them
on the locking list for now.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Cole Robinson <crobinso@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Michal Privoznik [Fri, 15 Feb 2019 10:23:29 +0000 (11:23 +0100)]
security: Document @restore member of transaction list
Both DAC and SELinux drivers support transactions. Each item on
the transaction list consists of various variables and @restore
is one of them. Document it so that as the list of variables grow
it's easier to spot which variable does what.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Cole Robinson <crobinso@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Michal Privoznik [Tue, 15 Jan 2019 10:15:19 +0000 (11:15 +0100)]
security: Don't skip label restore on file systems lacking XATTRs
The way that virSecurityDACRecallLabel is currently written is
that if XATTRs are not supported for given path to the caller
this is not different than if the path is still in use. The value
of 1 is returned which makes secdrivers skip label restore.
This is clearly a bug as we are not restoring labels on say NFS
even though previously we were.
Strictly speaking, changes to virSecurityDACRememberLabel are not
needed, but they are done anyway so that getter and setter behave
in the same fashion.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Cole Robinson <crobinso@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Michal Privoznik [Fri, 22 Mar 2019 14:41:06 +0000 (15:41 +0100)]
virFileRemoveXAttr: Report error on failure
It's better to have the function report errors, because none of
the callers does.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Cole Robinson <crobinso@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Michal Privoznik [Fri, 22 Mar 2019 14:34:59 +0000 (15:34 +0100)]
virFileSetXAttr: Report error on failure
It's better to have the function report errors, because none of
the callers does.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Cole Robinson <crobinso@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Michal Privoznik [Fri, 22 Mar 2019 14:08:36 +0000 (15:08 +0100)]
virfile: Make virFileGetXAttr report errors
The way that security drivers use XATTR is kind of verbose. If
error reporting was left for caller then the caller would end up
even more verbose.
There are two places where we do not want to report error if
virFileGetXAttr fails. Therefore virFileGetXAttrQuiet is
introduced as an alternative that doesn't report errors.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Cole Robinson <crobinso@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
virSecuritySELinuxRestoreAllLabel: Print @migrated in the debug message too
Just like it's DAC counterpart is doing,
virSecuritySELinuxRestoreAllLabel() could print @migrated in the
debug message.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Cole Robinson <crobinso@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Michal Privoznik [Tue, 15 Jan 2019 08:19:08 +0000 (09:19 +0100)]
tools: Slightly rework libvirt_recover_xattrs.sh
Firstly, there's no reason to enumerate all XATTRs since they
differ only in the prefix and we can construct them in a loop.
Secondly, and more importantly, the script was still looking for
just one prefix "trusted.libvirt.security" even on FreeBSD.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Cole Robinson <crobinso@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Michal Privoznik [Mon, 15 Apr 2019 14:38:28 +0000 (16:38 +0200)]
qemusecuritytest: Fix capabilities loading
Having to enumerate all capabilities that we want domain to have
is too verbose and prevents us from adding more tests. Have the
domain always have the latest x86_64 capabilities. This means
that we have to drop two arm tests, but on the other hand, I'm
introducing 50 new cases. I've listed 50 biggest .args files and
added those:
libvirt.git $ ls -Sr $(find tests/qemuxml2argvdata/ \
-type f -iname "*.x86_64-latest.args") | tail -n 50
Except for two:
1) disk-backing-chains-noindex - this XML has some disks with
backing chain. And since set is done on the whole backing chain
and restore only on the top layer this would lead to instant test
failure. Don't worry, secdrivers will be fixed shortly too and
the test case will be added.
2) hostdev-mdev-display-spice-egl-headless - for this XML
secdriver tries to find IOMMU group that mdev lives in. Since we
are not mocking sysfs access this test case would fail.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Michal Privoznik [Mon, 15 Apr 2019 13:34:54 +0000 (15:34 +0200)]
qemusecuritytest: Use AUTOFREE/AUTOUNREF
This simplifies the code a bit and removes the need for cleanup
label in one case. In the other case the label is kept because
it's going to be used later.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Michal Privoznik [Mon, 15 Apr 2019 14:15:10 +0000 (16:15 +0200)]
qemusecuritymock: Introduce and use freePaths()
Problem with current approach is that if
qemuSecuritySetAllLabel() fails, then the @chown_paths and
@xattr_paths hash tables are not freed and preserve values
already stored there into the next test case.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Michal Privoznik [Mon, 15 Apr 2019 11:56:11 +0000 (13:56 +0200)]
qemusecuritymock: Actually set error on failure
I don't really know what happened when I was writing the original
code, but even if error was to be set the corresponding boolean
was set to false meaning no error.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Michal Privoznik [Mon, 15 Apr 2019 15:17:40 +0000 (17:17 +0200)]
qemusecuritymock: Fix bit arithmetic
One of the functions of this mock is that it spoofs chown() and
stat() calls. But it is doing so in a clever way: it stores the
new owner on chown() and reports it on subsequent stat(). This is
done by using a 32bit unsigned integer where one half is used to
store uid the other is for gid. Later, when stat() is called the
integer is fetched and split into halves again. Well, my bit
operation skills are poor and the code I've written does not do
that properly.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Michal Privoznik [Mon, 15 Apr 2019 11:45:38 +0000 (13:45 +0200)]
qemusecuritymock: Mock virProcessRunInFork
This test is beautiful. It checks if we haven't messed up
refcounting on security labels (well, XATTRs where the original
owner is stored). It does this by setting up tracking of XATTR
setting/removing into a hash table, then calling
qemuSecuritySetAllLabel() followed by immediate
qemuSecurityRestoreAllLabel() at which point, the hash table must
be empty. The test so beautifully written that no matter
what you do it won't fail. The reason is that all seclabel work
is done in a child process. Therefore, the hash table in the
parent is never changed and thus always empty.
There are two reasons for forking (only one of them makes sense
here though):
1) namespaces - when chown()-ing a file we have to fork() and
make the child enter desired namespace,
2) locking - because of exclusive access to XATTRs we lock the
files we chown() and this is done in a fork (see 207860927ad for
more info).
While we want to fork in real world, we don't want that in a test
suite. Override virProcessRunInFork() then.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
John Ferlan [Sun, 30 Jun 2019 13:56:53 +0000 (09:56 -0400)]
rpc: Fix build error for virNetServerNew ATTRIBUTE_NONNULL values
Commit 5a148ce84 altered the virNetServerNew to remove a parameter
but neglected to update the ATTRIBUTE_NONNULL's which causes a build
failure for when checking is enabled such as when lv_cv_static_analysis
is enabled.
Pavel Hrdina [Thu, 27 Jun 2019 12:54:54 +0000 (14:54 +0200)]
util: vircgroupv2: stop enabling missing controllers with systemd
Because of a systemd delegation policy [1] we should not write to any
cgroups files owned by systemd which in case of cgroups v2 includes
'cgroups.subtree_control'.
systemd will enable controllers automatically for us to have them
available for VM cgroups.
As it turns out it's not a good idea on systemd hosts. The root
cgroup can have all controllers enabled but they don't have to be
enabled for sub-cgroups.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> Reviewed-by: Ján Tomko <jtomko@redhat.com>
A deprecation is a warning to something that use of a feature is
being discouraged. By definition it is not an error condition to
continue to use a deprecated feature.
A VIR_ERR_DEPRECATED constant thus makes no conceptual sense. For
features which are entirely absent we already document that the
VIR_ERR_NO_SUPPORT code will be used. There is no need to distinguish
between a feature which never existed and a feature which previously
existed and was since removed.
Reviewed-by: Ján Tomko <jtomko@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
qemu: delete methods which are no longer supported
The public API entry points will report VIR_ERR_NO_SUPPORT to the
caller when a driver does not provide an implementation of a particular
method.
When deleting methods, leaving the driver API entry point explicitly
set to NULL with an version range comment, allows the hvsupport.html
page to document when the AP was removed.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Jie Wang [Tue, 18 Jun 2019 13:28:26 +0000 (21:28 +0800)]
qemu: distinguish pr disk before qemuHotplugRemoveManagedPR
when a disk without PR perform attach or detach operation,
need not call qemuHotplugRemoveManagedPR, otherwise, it will
print err log about PR, let us fix it.
Signed-off-by: Jie Wang <wangjie88@huawei.com> Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
The sys/sysctl.h header is only needed on BSD platforms to get
the sysctlbyname() function declaration. On Linux we talk to
procfs instead to change sysctls.
Unfortunately a legacy sys/sysctl.h header does exist on Linux
and including it has recently started triggering a deprecation
warning from glibc.
Protect its inclusion with a HAVE_SYSCTLBYNAME check instead
so that it only gets used on platforms where we need that
function declaration.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Pavel Hrdina [Thu, 20 Jun 2019 11:10:20 +0000 (13:10 +0200)]
util: vircgroupv2: don't error out if enabling controller fails
Currently CPU controller cannot be enabled if there is any real-time
task running and is assigned to non-root cgroup which is the case on
several distributions with graphical environment.
Instead of erroring out treat it as the controller is not available.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Pavel Hrdina [Thu, 20 Jun 2019 11:02:57 +0000 (13:02 +0200)]
util: vircgroup: improve controller detection
This affects only cgroups v2 where enabled controllers are not based on
available mount points but on the list provided in cgroup.controllers
file. However, moving it will fill in placement as well, so it needs
to be freed together with mount point if we don't need that controller.
Before this patch we were assuming that all controllers available in
root cgroup where available in all other sub-cgroups which was wrong.
In order to fix it we need to move the cgroup controllers detection
after cgroup placement was prepared in order to build correct path for
cgroup.controllers file.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> Reviewed-by: Peter Krempa <pkrempa@redhat.com>
projects / thirdparty / libvirt.git / log
