Rather than duplicate the error code, let's create an error
label to keep code common.
Signed-off-by: John Ferlan <jferlan@redhat.com>
ACKed-by: Michal Privoznik <mprivozn@redhat.com>
Alter the code path to remove the need to to go cleanup and thus
remove the label completely.
Signed-off-by: John Ferlan <jferlan@redhat.com>
ACKed-by: Michal Privoznik <mprivozn@redhat.com>
If the virStoragePoolRefresh fails and we call stopPool, the
code neglected to clean up the state file leading to the next
libvirtd restart attempting to start the pool. For a transient
pool this could make it unexpectedly reappear.
Signed-off-by: John Ferlan <jferlan@redhat.com>
ACKed-by: Michal Privoznik <mprivozn@redhat.com>
https://bugzilla.redhat.com/show_bug.cgi?id=1607202
It's essentially stated in the nwfilterBindingDelete that we
will allow the admin to shoot themselves in the foot by deleting
the nwfilter binding which then allows them to undefine the
nwfilter that is in use for the running guest...
However, by allowing this we cause a problem for libvirtd
restart reconnect processing which would then try to recreate
the missing binding attempting to use the deleted filter
resulting in an error and thus shutting the guest down.
So rather than keep adding virDomainConfNWFilterInstantiate
flags to "ignore" specific error conditions, modify the logic
to ignore, but VIR_WARN errors other than ignoreExists. This
will at least allow the guest to not shutdown for only nwfilter
binding errors that we can now perhaps recover from since we
have the binding create/delete capability.
Signed-off-by: John Ferlan <jferlan@redhat.com>
ACKed-by: Michal Privoznik <mprivozn@redhat.com>
All of the ones being removed are pulled in by internal.h. The only
exception is sanlock which expects the application to include <stdint.h>
before sanlock's headers, because sanlock prototypes use fixed width
int, but they don't include stdint.h themselves, so we have to leave
that one in place.
Signed-off-by: Erik Skultety <eskultet@redhat.com>
Acked-by: Michal Privoznik <mprivozn@redhat.com>
It doesn't really make sense for us to have stdlib.h and string.h but
not stdio.h in the internal.h header.
Signed-off-by: Erik Skultety <eskultet@redhat.com>
Acked-by: Michal Privoznik <mprivozn@redhat.com>
The libxl_domain_config_from_json API appeared in Xen 4.5, hence
there is no need to check for its existence after changing the
minimum supported Xen version to 4.6. Remove the check and its
use in the tests.
Signed-off-by: Jim Fehlig <jfehlig@suse.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
QEMU commits:
e37a5c7fa4 (v2.12.0)
i386: Add Intel Processor Trace feature support
c2f193b538 (v2.7.0)
target-i386: Add support for UMIP and RDPID CPUID bits
aff9e6e46a (v2.12.0)
x86/cpu: Enable new SSE/AVX/AVX512 cpu features
f77543772d (v2.9.0)
x86: add AVX512_VPOPCNTDQ features
5131dc433d (v3.1.0)
i386: Add CPUID bit for PCONFIG
59a80a19ca (v3.1.0)
i386: Add CPUID bit for WBNOINVD
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
When restoring a domain from a compressed image, we launch an
intermediate process for decompressing the saved data. If QEMU fails to
load the data for some reason, we force close the stdin/stdout file
descriptors of the intermediate process and wait for it to die. However,
virCommandWait can report various errors which would overwrite the real
error from QEMU. Thus instead of getting something useful:
internal error: process exited while connecting to monitor:
2018-09-17T15:17:29.998910Z qemu-system-x86_64: can't apply global
Skylake-Client-x86_64-cpu.osxsave=off: Property '.osxsave' not found
we could get an irrelevant error message:
internal error: Child process (lzop -dc --ignore-warn) unexpected
fatal signal 13
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
In 2e97450425 we've mistakenly removed gettext macro for
translating static strings. This results in table header being
printed in English regardless of user locale.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Lock all the paths we want to relabel to mutually exclude other
libvirt daemons.
The only hitch here is that directories can't be locked.
Therefore, when relabeling a directory do not lock it (this
happens only when setting up some domain private paths anyway,
e.g. huge pages directory).
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
So far the whole transaction handling is done
virSecuritySELinuxSetFileconHelper(). This needs to change for
the sake of security label remembering and locking. Otherwise we
would be locking a path when only appending it to transaction
list and not when actually relabelling it.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Firstly, the following code pattern is harder to follow:
if (func() < 0) {
error();
} else {
/* success */
}
We should put 'goto cleanup' into the error branch and move the
else branch one level up.
Secondly, 'rc' should really be named 'ret' because it holds
return value of the function. Not some intermediate value.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
This label is used in both successful and error paths. Therefore
it should be named 'cleanup' and not 'err'.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Lock all the paths we want to relabel to mutually exclude other
libvirt daemons.
The only hitch here is that directories can't be locked.
Therefore, when relabeling a directory do not lock it (this
happens only when setting up some domain private paths anyway,
e.g. huge pages directory).
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Firstly, the message that says we're setting uid:gid shouldn't be
called from virSecurityDACSetOwnershipInternal() because
virSecurityDACRestoreFileLabelInternal() is calling it too.
Secondly, there are places between us reporting label restore and
us actually doing it where we can quit. Don't say we're doing
something until we are actually about to do it.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
So far the whole transaction handling is done
virSecurityDACSetOwnershipInternal(). This needs to change for
the sake of security label remembering and locking. Otherwise we
would be locking a path when only appending it to transaction
list and not when actually relabeling it.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Two new APIs are added so that security driver can lock and
unlock paths it wishes to touch. These APIs are not for other
drivers to call but security drivers (DAC and SELinux). That is
the reason these APIs are not exposed through our
libvirt_private.syms file.
Three interesting things happen in this commit. The first is the
global @lockManagerMutex. Unfortunately, this has to exist so that
there is only one thread talking to virtlockd at a time. If there
were more threads and one of them closed the connection
prematurely, it would cause virtlockd killing libvirtd. Instead
of complicated code that would handle that, let's have a mutex
and keep the code simple.
The second interesting thing is keeping connection open between
lock and unlock API calls. This is achieved by duplicating client
FD and keeping it open until unlock is called. This trick is used
by regular disk content locking code when the FD is leaked to
qemu.
Finally, the third thing is polling implemented at client side.
Since virtlockd has only one thread that handles locking
requests, all it can do is either acquire lock or error out.
Therefore, the polling has to be implemented in client. The
polling is capped at 60 second timeout, which should be plenty
since the metadata lock is held only for a fraction of a second.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Now that we know what metadata lock manager user wishes to use we
can load it when initializing security driver. This is achieved
by adding new argument to virSecurityManagerNewDriver() and
subsequently to all functions that end up calling it.
The cfg.mk change is needed in order to allow lock_manager.h
inclusion in security driver without 'syntax-check' complaining.
This is safe thing to do as locking APIs will always exist (it's
only backend implementation that changes). However, instead of
allowing the include for all other drivers (like cpu, network,
and so on) allow it only for security driver. This will still
trigger the error if including from other drivers.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
This config option allows users to set and enable lock manager
for domain metadata. The lock manager is going to be used by
security drivers to serialize each other when changing a file
ownership or changing the SELinux label. The only supported lock
manager is 'lockd' for now.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
In some cases we might want to not load the lock driver config.
Alter virLockManagerPluginNew() and the lock drivers to cope with
this fact.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Soon there will be a virtlockd client that wants to either lock
all the resources or none (in order to avoid virtlockd killing
the client on connection close). Because on the RPC layer we can
only acquire one resource at a time, we have to perform a
rollback once we hit a resource that can't be acquired.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
This is a new type of object that lock drivers can handle.
Currently, it is supported by lockd driver only.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
The fact whether domain has or doesn't have RW disks is specific
to VIR_LOCK_MANAGER_OBJECT_TYPE_DOMAIN and therefore should
reside in union specific to it.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
We will want virtlockd to lock files on behalf of libvirtd and
not qemu process, because it is libvirtd that needs an exclusive
access not qemu. This requires new lock context.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
This flag causes virtlockd to use different offset when locking
the file.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
So far the virLockSpaceAcquireResource() locks the first byte in
the underlying file. But caller might want to lock other range.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
From time to time it's handy to jump directly to a function prototype in
a header. However, generating tags for headers is disabled by default in
ctags, let's enable it by using --c-kinds=+p.
Signed-off-by: Erik Skultety <eskultet@redhat.com>
The directory has been renamed in 562990849a, but a
reference to it was not updated at the same time, causing
'make dist' to fail ever since. Fix it.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
The file being present doesn't necessarily mean anything these
days, as it's created independently of whether the kvm module
has been loaded[1]; moreover, we're already gathering all the
information we need through QMP, so poking the filesystem at
all is entirely unnecessary.
[1] https://github.com/systemd/systemd/commit/d35d6249d5a7ed3228
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
This capability is documented as having one meaning (whether
KVM is enabled by default) but is actually assigned two other
meanings over its life: whether the query-kvm QMP command is
available at first, and later on whether KVM is usable / was
used during probing.
Since the query-kvm QMP command was available in 1.5.0, we
can avoid probing for it; additionally, we can simplify the
logic by setting the flag when it applies instead of initially
setting it and then clearing it when it doesn't.
The flag's description is also updated to reflect reality.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
A side effect of recent changes is that we would always try
to regenerate the capabilities cache for non-native QEMU
binaries based on /dev/kvm availability, which is of course
complete nonsense. Make sure that doesn't happen.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
It was already available in 1.5.0.
Moreover, we're not even formatting it on the QEMU command
line, ever: we just use it as part of some logic that decides
whether KVM support should be advertised, and as it turns out
that logic is actually buggy and dropping this capability
fixes it.
https://bugzilla.redhat.com/show_bug.cgi?id=1628469
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
More specifically, everything that's tested by qemucapabilities
now goes through qemucaps2xml as well.
Ideally we'll rewrite both so that listing all test cases is
unnecessary and they get picked up automatically by listing the
contents of the input directory instead, but that's a refactor
for another day :)
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
While qemucaps2xml has a meager two test cases to its name, we
have plenty of data from qemucapabilities which is taken from
actual QEMU binaries, covers pretty much all supported QEMU
versions and architectures and is even in the right format already!
Rewrite qemucaps2xml so that it uses qemucapabilities data as
input. Right now we have a single test case, but we're going to
add a lot more next.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
This patch add syntax-check rule for incorrect indentation and blank
first line in function body by changing check-spacing.pl.
For efficiency, it only checks the first line of function body.
But it's enough for most cases.
Signed-off-by: Shi Lei <shi_lei@massclouds.com>
Now that committing transactions using pid == -1 means that we're
not fork()-ing to run the transaction in a specific namespace, we
can utilize the transaction processing semantics in order to
start, run a or multiple commands, and then commit the
transaction without being concerned with other interactions or
transactions interrupting the processing. This will eventually
allow us to have a single place where all the paths can be
locked, followed by relabeling and unlocking again.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
It will be desirable to run transactions more often than we
currently do. Even if the domain we're relabeling the paths for
does not run in a namespace. If that's the case, there is no need
to fork() as we are already running in the right namespace. To
differentiate whether transaction code should fork() or not the
@pid argument now accepts -1 (which means do not fork).
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
In the future, the transactions are not going to be optional and
they will be run regardless of domain using namespace to collect
list of paths to be relabeled.
To make sure there won't be an API that goes behind transaction
code back update the comment that serves as decision manual
whether an API must be fully implemented or plain #define is
sufficient.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Even though the current use of the functions does not require full
implementation with transactions (none of the callers passes a path
somewhere under /dev), it doesn't hurt either. Moreover, in
future patches the paradigm is going to shift so that any API
that touches a file is required to use transactions.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Even though the current use of the function does not require full
implementation with transactions (none of the callers pass a path
somewhere under /dev), it doesn't hurt either. Moreover, in
future patches the paradigm is going to shift so that any API
that touches a file is required to use transactions.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Functions that deal with virPCIDeviceAddress exclusively
belong to util/virpci.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Fabiano Fidêncio <fidencio@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
