Per the documentation, is_selinux_enabled() returns -1 on error.
Account for this. Previously when -1 was being returned the condition
would still be true. I was noticing this because on my system that has
selinux disabled I was getting this in the libvirt.log every 5
seconds:
error : virIdentityGetSystem:173 : Unable to lookup SELinux process context: Invalid argument
With this patch applied, I no longer get these messages every 5
seconds. I am submitting this in case its deemed useful for inclusion.
Anyone have any comments on this change? This is a patch off current
master.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
The virSocketAddrMask method did not initialize all fields
in the sockaddr_in6 struct. In paticular the 'sin6_scope_id'
field could contain random garbage, which would in turn
affect the result of any later virSocketAddrFormat calls.
This led to ip6tables rules in the FORWARD chain which
matched on random garbage sin6_scope_id. Fortunately these
were ACCEPT rules, so the impact was merely that desired
traffic was blocked, rather than undesired traffic allowed.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
To allow for fault injection of the virCommand dry run,
add the ability to register a callback. The callback will
be passed the argv, env and stdin buffer and is expected
to return the exit status and optionally fill stdout and
stderr buffers.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
In libxl driver oldStateDir is NULL when calling
virHostdevReAttachDomainHostdevs. This is allowed.
Remove ATTRIBUTE_NONNULL setting from oldStateDir.
Introduced by commit 6225cb3.
Signed-off-by: Chunyan Liu <cyliu@suse.com>
Signed-off-by: Ján Tomko <jtomko@redhat.com>
A earlier commit changed the global log buffer so that it only
records messages that are explicitly requested via the log
filters setting. This removes the performance burden, and
improves the signal/noise ratio for messages in the global
buffer. At the same time though, it is somewhat pointless, since
all the recorded log messages are already going to be sent to an
explicit log output like syslog, stderr or the journal. The
global log buffer is thus just duplicating this data on stderr
upon crash.
The log_buffer_size config parameter is left in the augeas
lens to prevent breakage for users on upgrade. It is however
completely ignored hereafter.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Currently the log filter strings are used in a string comparison
against the source filename each time log message is emitted.
If no log filters at all are set, there's obviously no string
comparison to be done. If any single log filter is set though,
this imposes a compute burden on every logging call even if logs
from the file in question are disabled. This string comparison
must also be done while the logging mutex is held, which has
implications for concurrency when multiple threads are emitting
log messages.
This changes the log filtering to be done based on the virLogSource
object name. The virLogSource struct is extended to contain
'serial' and 'priority' fields. Any time the global log filter
rules are changed a global serial number is incremented. When a
log message is emitted, the serial in the virLogSource instance
is compared with the global serial number. If out of date, then
the 'priority' field in the virLogSource instance is updated based
on the new filter rules. The 'priority' field is checked to see
whether the log message should be sent to the log outputs.
The comparisons of the 'serial' and 'priority' fields are done
with no locks held. So in the common case each logging call has
an overhead of 2 integer comparisons, with no locks held. Only
if the decision is made to forward the message to the log output,
or if the 'serial' value is out of date do locks need to be
acquired.
Technically the comparisons of the 'serial' and 'priority' fields
should be done with locks held, or using atomic operations. Both
of these options have a notable performance impact, however, and
since all writes a protected by a global mutex, it is believed
that worst case behaviour where the fields are read concurrently
with being written would merely result in an mistaken emission
or dropping of the log message in question. This is an acceptable
tradeoff for the performance benefit of avoiding locking.
As a quick benchmark, a demo program that registers 500 file
descriptors with the event loop (eg equiv of 500 QEMU monitor
commands), creates pending read I/O on every FD, and then runs
virEventRunDefaultImpl() took 4.6 seconds to do 51200 iterations.
After this optimization it only takes 3.3 seconds, with the log
APIs no longer being a relevant factor in the running time.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Any source file which calls the logging APIs now needs
to have a VIR_LOG_INIT("source.name") declaration at
the start of the file. This provides a static variable
of the virLogSource type.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
As part of the goal to get away from doing string matching on
filenames when deciding whether to emit a log message, turn
the virLogSource enum into a struct which contains a log
"name". There will eventually be one virLogSource instance
statically declared per source file. To minimise churn in this
commit though, a single global instance is used.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
The dtrace probe macros rely on the logging API. We can't make
the internal.h header include the virlog.h header though since
that'd be a circular include. Instead simply split the dtrace
probes into their own header file, since there's no compelling
reason for them to be in the main internal.h header.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
The error reporting code will invoke a callback when any error
is raised and the default callback will print to stderr. The
virRaiseErrorFull method also sends all error messages on to the
logging code, which also prints to stderr by default. To avoid
duplicated data on stderr, the logging code has some logic to
skip emission when no log outputs are configured, which checks
whether the virLogSource == VIR_LOG_FROM_ERROR.
Meanwhile the libvirtd daemon can register another callback which
is used to reduce log message priority from error to a lower level.
When this is used we do want messages to end up on stderr, so the
error code will conditionally use either VIR_LOG_FROM_FILE or
VIR_LOG_FROM_ERROR depending on whether such a callback is provided.
This will all complicate later refactoring. By pushing the checks
for whether a log output is present up a level into the error code,
the special cases can be isolated in one place.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
With the vast number of log debug statements in the code, the
logging framework has a measurable performance impact on libvirt
code, particularly in the daemon event loop.
The global log buffer records every single log message triggered
whether anyone cares to see them or not. This makes it impossible
to eliminate the overhead of printf format expansions in any of
the logging code. It is possible to disable the global log buffer
in libvirtd itself, but this doesn't help client side library
code. Also even if disabled by the config file, the existence of
the feature makes other performance improvements in the logging
layer impossible.
Instead of logging every single message to the global buffer, only
log messages that pass the log filters. This if libvirtd is set
to have log_filters="1:libvirt 1:qemu" the global log buffer will
only get filled with those messages instead of everything. This
reduces the performance burden, as well as improving the signal
to noise ratio of the log buffer.
As a quick benchmark, a demo program that registers 500 file
descriptors with the event loop (eg equiv of 500 QEMU monitor
commands), creates pending read I/O on every FD, and then runs
virEventRunDefaultImpl() took 1 minute 40 seconds to do 51200
iterations with nearly all the time shown against the logging
code. After this optimization it only takes 4.6 seconds.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Commit a1cbe4b5 added a check for spaces around assignments and this
patch extends it to checks for spaces around '=='. One exception is
virAssertCmpInt where comma after '==' is acceptable (since it is a
macro and '==' is its argument).
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
Our current pidfile acquire APis (virPidFileAcquire) simply return -1 upon
failure to acquire a lock. This patch adds a parameter 'bool waitForLock'
which instructs the APIs if we want to make it block and wait for the lock
or not.
We have to explicitly destroy TAP devices on FreeBSD because
they're not freed after being closed, otherwise we end up with
orphaned TAP devices after destroying a domain.
Commit 6b306d66 converted virHostdevManager to a virObject, but
missed adding a virObject field to the virHostdevManager struct.
Result is memory corruption when taking a reference on an instance
of the object, where atomic inc is done on the stateDir field.
Later use of stateDir crashes libvirtd.
While running vircryptotest, it was found that valgrind pointed out the
following error:
==27453== Invalid write of size 1
==27453== at 0x4C7D7C9: virCryptoHashString (vircrypto.c:76)
==27453== by 0x401C4E: testCryptoHash (vircryptotest.c:41)
==27453== by 0x402A11: virtTestRun (testutils.c:199)
==27453== by 0x401AD5: mymain (vircryptotest.c:76)
==27453== by 0x40318D: virtTestMain (testutils.c:782)
==27453== by 0x3E6CE1ED1C: (below main) (libc-start.c:226)
==27453== Address 0x51f0541 is 0 bytes after a block of size 65 alloc'd
==27453== at 0x4A0577B: calloc (vg_replace_malloc.c:593)
==27453== by 0x4C69F2E: virAllocN (viralloc.c:189)
==27453== by 0x4C7D76B: virCryptoHashString (vircrypto.c:69)
==27453== by 0x401C4E: testCryptoHash (vircryptotest.c:41)
==27453== by 0x402A11: virtTestRun (testutils.c:199)
==27453== by 0x401AD5: mymain (vircryptotest.c:76)
==27453== by 0x40318D: virtTestMain (testutils.c:782)
==27453== by 0x3E6CE1ED1C: (below main) (libc-start.c:226)
==27453==
...and many more. Two observations: hashstrlen was already set
to include the trailing NUL byte (so writing to hashstrlen as
the array offset was indeed writing one byte beyond bounds), and
VIR_ALLOC_N already guarantees zero-initialization (so we already
have a trailing NUL without needing to explicitly write one).
Signed-off-by: Eric Blake <eblake@redhat.com>
Changes parameter from vm def to specific hostdevs info and name info, so that
it could be used more widely, e.g, could be used without full vm def info.
Change any variable names with Usb, Pci or Scsi to use
USB, PCI and SCSI since they are abbreviations.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Some virHostdevXXXX methods included the string Hostdev again
as a suffix. Change the latter to Device instead.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Change any method names with Usb, Pci or Scsi to use
USB, PCI and SCSI since they are abbreviations.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Various methods in virnetdev.c and virhostdev.c were missing
const-ness for several char * parameters.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Use virObject to virHostdevManager, so that each driver using virHostdevManager
can keep a reference to it, and through counting refs to make virHostdevManager
get freed.
Commit b9dd878f caused a regression in iptables interaction by
logging non-zero status at a higher level than VIR_INFO. Revert
that portion of the commit, as well as adding a comment explaining
why we check the status ourselves.
Reported by Nehal J Wani.
* src/util/viriptables.c (virIpTablesOnceInit): Undo log regression.
Signed-off-by: Eric Blake <eblake@redhat.com>
The ebtablesRemoveForwardPolicyReject method was unused and
would not do anything useful even if called.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
The ebtRules data structure serves no useful purpose as
the table name is never used and only 1 single chain name
needs to be stored. Just store the chain name directly
in the ebtablesContext instead.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
When adding/removing ebtables rules, the code would keep
an array of all rules in memory. This list of rules was
never used for any purpose and would be lost if libvirtd
restarted. Delete all the unused code.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
The ebtablesForwardPolicyReject method is only used internally
to the ebtables code and thus should have been static.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
The future QEMU capabilities cache needs to be able to invalidate
itself if the libvirtd binary or any loadable modules are changed
on disk. Record the 'ctime' value for these binaries and provide
helper APIs to query it. This approach assumes that if libvirt.so
is changed, then libvirtd will also change, which should usually
be the case with libtool's wrapper scripts that cause libvirtd to
get re-linked
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
GNULIB provides APIs for calculating md5 and sha256 hashes,
but these APIs only return you raw byte arrays. Most users
in libvirt want the hash in printable string format. Add
some helper APIs in util/vircrypto.{c,h} for doing this.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
This resolves a Coverity RESOURCE_LEAK issue introduced by commit
id 'de6fa535' where the virSCSIDeviceSetUsedBy() didn't VIR_FREE
the 'copy' or possibly VIR_STRDUP()'d values. It also ensures that
the VIR_APPEND_ELEMENT is successful...
