Having to repeat "./job[1]/" XPath prefix for every single element or
attribute we want to parse is suboptimal. And even more so once we
further extract code from qemuDomainObjPrivateXMLParseJob into separate
functions.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Commit dc567cc22b introduced qemuDomainObjPrivateXMLParseJob, but forgot
to move "./job[1]/@type" parsing into it.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
The daemonStreamHandleWriteData() function is called whenever
server side of stream is able to receive some data. Nevertheless,
it calls virStreamSend() (to pass data down to virFDStream) and
depending on its return value it may abort the stream. However,
the functions it called when doing so are public APIs and as such
reset any error set previously. Therefore, if there was any error
in writing data to stream (i.e. repored in virStreamSend) it is
reset before virNetServerProgramSendReplyError() can get to it.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Some fields reported by dmidecode have plenty of useless spaces
(in fact some have nothing but spaces). To deal with this we have
introduced virSkipSpacesBackwards() and use it in
virSysinfoParseX86Processor() and virSysinfoParseX86Memory().
However, other functions (e.g. virSysinfoParseX86Chassis()) don't
use it at all and thus we are reporting nonsense:
<sysinfo type='smbios'>
<chassis>
<entry name='manufacturer'>FUJITSU</entry>
<entry name='version'> </entry>
<entry name='serial'> </entry>
<entry name='asset'> </entry>
<entry name='sku'>Default string</entry>
</chassis>
</sysinfo>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
We read from the agent until seeing a \r\n pair to indicate a completed
reply or event. To avoid memory denial-of-service though, we must have a
size limit on amount of data we buffer. 10 MB is large enough that it
ought to cope with normal agent replies, and small enough that we're not
consuming unreasonable mem.
This is identical to the flaw we had reading from the QEMU monitor
as CVE-2018-5748, so rather embarrassing that we forgot to fix
the agent code at the same time.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Policy-Kit has been replaced by polkit (referred to, respectively,
as POLKIT0 and POLKIT1 in our Makefiles).
The last build fix with old Policy-Kit was in May 2013:
commit <442eb2ba> and build with -Wunused-label was broken
since April 2016: commit <8437130>
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Split out the parser and separate it from the private data part so that
it can be later reused in other parts of the code.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Since seclabels are formatted along with the source element and will
also make sense to be passed for the backing chain we should parse them
in the place where we parse the disk source. Same applies for
validation.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Rather than checking that the security label is legal when parsing it
move the code into a separate function.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Since we already parse the <backingStore> of a disk source, we should
also validate the configuration for the whole backing chain and not only
for the top level image.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Move out formatting of 'startuPolicy' which is a property of the disk
out of the <source> element. Extracting the code formating the content
and attributes will also allow reuse in other parts of the code.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
The wrapper functionality can be moved to the only user
virDomainDiskSourceFormatInternal. Also removes comment which does not
reflect the truth any more.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Now that the function is using virXMLFormatElement we don't need to
conditionally format anything, since we'll format the element according
to the presence of content.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
We're freeing individual items in it but not the array itself.
==19200== 40 bytes in 1 blocks are definitely lost in loss record 847 of 1,059
==19200== at 0x4C2D12F: realloc (vg_replace_malloc.c:785)
==19200== by 0x52C5532: virReallocN (viralloc.c:245)
==19200== by 0x52C5628: virExpandN (viralloc.c:294)
==19200== by 0x52C58FC: virInsertElementsN (viralloc.c:436)
==19200== by 0x542856B: virSecurityDACChownListAppend (security_dac.c:115)
==19200== by 0x54286B4: virSecurityDACTransactionAppend (security_dac.c:167)
==19200== by 0x542902F: virSecurityDACSetOwnershipInternal (security_dac.c:560)
==19200== by 0x54295D6: virSecurityDACSetOwnership (security_dac.c:650)
==19200== by 0x542AEE0: virSecurityDACSetInputLabel (security_dac.c:1472)
==19200== by 0x542B61D: virSecurityDACSetAllLabel (security_dac.c:1693)
==19200== by 0x542DD67: virSecurityManagerSetAllLabel (security_manager.c:869)
==19200== by 0x54279C2: virSecurityStackSetAllLabel (security_stack.c:361)
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Probably due to copy-paste error we're storing asset tag into
def->sku which we even use in the next step to store SKU number
and thus the asset tag leaks.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Modernize the code by using the clever formatter rather than checking
manually when to format the end of the element.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
The code overwrote the internal job type and then fixed it back. Since
the job type is not accessed in the code this does not make much sense.
Use the temporary value instead.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Since data.count is not a pointer, but an integer,
compare it against an integer value instead of using
the implicit "boolean" conversion that is customarily
used for pointers.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
It is possible to deadlock libvirtd when concurrently starting a domain
and restarting the daemon. Threads involved in the deadlock are
Thread 4 (Thread 0x7fc13b53e700 (LWP 64084)):
/lib64/libpthread.so.0
at util/virthread.c:154
at qemu/qemu_monitor.c:1083
cmd=0x7fc110017700, scm_fd=-1, reply=0x7fc13b53d318) at
qemu/qemu_monitor_json.c:305
cmd=0x7fc110017700,
reply=0x7fc13b53d318) at qemu/qemu_monitor_json.c:335
at qemu/qemu_monitor_json.c:1298
at qemu/qemu_monitor.c:1697
vm=0x7fc110003d00, asyncJob=QEMU_ASYNC_JOB_START) at qemu/qemu_process.c:1763
vm=0x7fc110003d00,
asyncJob=6, logCtxt=0x7fc1100089c0) at qemu/qemu_process.c:1835
vm=0x7fc110003d00, asyncJob=6, logCtxt=0x7fc1100089c0) at
qemu/qemu_process.c:2180
driver=0x7fc12004e1e0,
vm=0x7fc110003d00, asyncJob=QEMU_ASYNC_JOB_START, incoming=0x0, snapshot=0x0,
vmop=VIR_NETDEV_VPORT_PROFILE_OP_CREATE, flags=17) at qemu/qemu_process.c:6111
driver=0x7fc12004e1e0,
vm=0x7fc110003d00, updatedCPU=0x0, asyncJob=QEMU_ASYNC_JOB_START,
migrateFrom=0x0,
migrateFd=-1, migratePath=0x0, snapshot=0x0,
vmop=VIR_NETDEV_VPORT_PROFILE_OP_CREATE,
flags=17) at qemu/qemu_process.c:6334
xml=0x7fc110000ed0 "<!--\nWARNING: THIS IS AN AUTO-GENERATED FILE.
CHANGES TO IT ARE LIKELY TO BE\nOVERWRITTEN AND LOST. Changes to this xml
configuration should be made using:\n virsh edit testvv\nor other
applicati"..., flags=0) at qemu/qemu_driver.c:1776
...
Thread 1 (Thread 0x7fc143c66880 (LWP 64081)):
/lib64/libpthread.so.0
at util/virthread.c:122
conf/nwfilter_conf.c:159
sig=0x7ffe0a831e30,
opaque=0x0) at remote/remote_daemon.c:724
opaque=0x558c5328b230) at rpc/virnetdaemon.c:654
at util/vireventpoll.c:508
rpc/virnetdaemon.c:858
remote/remote_daemon.c:1496
(gdb) thr 1
[Switching to thread 1 (Thread 0x7fc143c66880 (LWP 64081))]
/lib64/libpthread.so.0
(gdb) f 1
at util/virthread.c:122
122 pthread_rwlock_wrlock(&m->lock);
(gdb) p updateLock
$1 = {lock = {__data = {__lock = 0, __nr_readers = 1, __readers_wakeup = 0,
__writer_wakeup = 0, __nr_readers_queued = 0, __nr_writers_queued = 1,
__writer = 0,
__shared = 0, __rwelision = 0 '\000', __pad1 = "\000\000\000\000\000\000",
__pad2 = 0, __flags = 0},
__size = "\000\000\000\000\001", '\000' <repeats 15 times>, "\001",
'\000' <repeats 34 times>, __align = 4294967296}}
Reloading of the nwfilter driver is stuck waiting for a write lock, which
already has a reader (from qemuDomainCreateXML) in the critical section.
Since the reload occurs in the context of the main event loop thread,
libvirtd becomes deadlocked. The deadlock can be avoided by offloading
the reload work to a thread.
Signed-off-by: Jim Fehlig <jfehlig@suse.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
The bhyve driver crashes in bhyveBuildNetArgStr() when
network interface model is not defined. As it has to be provided
explicitly, add a check to report an error if it's missing.
Signed-off-by: Roman Bogorodskiy <bogorodskiy@gmail.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Clang 6.0.0 complains when initializing structure with { NULL }:
conf/domain_addr.c:1494:38: error: missing field 'type' initializer [-Werror,-Wmissing-field-initializers]
virDomainDeviceInfo nfo = { NULL };
Use { 0 } instead to make it happy.
Signed-off-by: Roman Bogorodskiy <bogorodskiy@gmail.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
This time around it's not enough to just pick the latest commit,
because with aed87bb2aa6ed83b49574eb982e3bdd4c36acf17 keycodemapdb
renamed the 'rfb' keycode to 'qnum' and we need to accept the new
name while maintaining backwards compatibility.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
https://bugzilla.redhat.com/show_bug.cgi?id=1553162
When validating a device XML config we check if user provided
alias is unique. We do this by maintaining a hash table of device
aliases as we iterated over all devices defined for the domain.
However, it may happen that what appears as two devices in domain
XML is in fact just one interface in hypervisor. We can assume
libvirt generated aliases to be unique and thus really check user
provided ones only.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
https://bugzilla.redhat.com/show_bug.cgi?id=1553075
For some weird reason this function is getting live and
persistent def for domain but then accesses vm->def and
vm->newDef directly. This is rather unsafe as we can be
accessing NULL pointer.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Recently, this warning is appearing while libvirt is being compiled:
Function 'qemuAssignDeviceDiskAlias' argument order different:
declaration 'vmdef, def' definition 'def, disk'
This commit change the default declaration for qemuAssignDeviceDiskAlias
specified at src/qemu/qemu_alias.c.
Signed-off-by: Julio Faracco <jcfaracco@gmail.com>
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
The qemu driver registered the helpers from util code, but it will be
necessary to format also some qemu-specific data.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
We've implemented all existing checks, and more, in the new
function, so we can finally drop the old one.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Laine Stump <laine@laine.org>
This change catches an invalid use of the option in our
test suite.
https://bugzilla.redhat.com/show_bug.cgi?id=1483816
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Laine Stump <laine@laine.org>
This change catches an invalid use of the option in our
test suite.
https://bugzilla.redhat.com/show_bug.cgi?id=1483816
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Laine Stump <laine@laine.org>
The existing function is renamed and called from the new one, so
that even while we're in the process of implementing new checks
all the existing ones will be performed.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Laine Stump <laine@laine.org>
In remoteConnectOpen, conn->uri cannot be NULL in the second
part of the OR expression due to short-circuit evaluation.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
In qemuMigrationSrcRun, we already checked for non-NULL mig
and then dereferenced it. It's only possible for mig to be
NULL in the error section.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
The size argument accounts for the nul-byte to terminate
the string. Use sizeof and remove the pointless assignment.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Currently both virtlogd and virtlockd use a single worker thread for
dispatching RPC messages. Even this is overkill and their RPC message
handling callbacks all run in short, finite time and so blocking the
main loop is not an issue like you'd see in libvirtd with long running
QEMU commands.
By setting max_workers==0, we can turn off the worker thread and run
these daemons single threaded. This in turn fixes a serious problem in
the virtlockd daemon whereby it loses all fcntl() locks at re-exec due
to multiple threads existing. fcntl() locks only get preserved if the
process is single threaded at time of exec().
Reviewed-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Jim Fehlig <jfehlig@suse.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
If max_workers is set to zero, then the worker thread pool won't be
created, so when serializing state for pre-exec we must set various
parameters to zero.
Reviewed-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Jim Fehlig <jfehlig@suse.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Currently if the virNetServer instance is created with max_workers==0 to
request a non-threaded dispatch process, we deadlock during dispatch
#0 0x00007fb845f6f42d in __lll_lock_wait () from /lib64/libpthread.so.0
#1 0x00007fb845f681d3 in pthread_mutex_lock () from /lib64/libpthread.so.0
#2 0x000055a6628bb305 in virMutexLock (m=<optimized out>) at util/virthread.c:89
#3 0x000055a6628a984b in virObjectLock (anyobj=<optimized out>) at util/virobject.c:435
#4 0x000055a66286fcde in virNetServerClientIsAuthenticated (client=client@entry=0x55a663a7b960)
at rpc/virnetserverclient.c:1565
#5 0x000055a66286cc17 in virNetServerProgramDispatchCall (msg=0x55a663a7bc50, client=0x55a663a7b960,
server=0x55a663a77550, prog=0x55a663a78020) at rpc/virnetserverprogram.c:407
#6 virNetServerProgramDispatch (prog=prog@entry=0x55a663a78020, server=server@entry=0x55a663a77550,
client=client@entry=0x55a663a7b960, msg=msg@entry=0x55a663a7bc50) at rpc/virnetserverprogram.c:307
#7 0x000055a662871d56 in virNetServerProcessMsg (msg=0x55a663a7bc50, prog=0x55a663a78020, client=0x55a663a7b960,
srv=0x55a663a77550) at rpc/virnetserver.c:148
#8 virNetServerDispatchNewMessage (client=0x55a663a7b960, msg=0x55a663a7bc50, opaque=0x55a663a77550)
at rpc/virnetserver.c:227
#9 0x000055a66286e4c0 in virNetServerClientDispatchRead (client=client@entry=0x55a663a7b960)
at rpc/virnetserverclient.c:1322
#10 0x000055a66286e813 in virNetServerClientDispatchEvent (sock=<optimized out>, events=1, opaque=0x55a663a7b960)
at rpc/virnetserverclient.c:1507
#11 0x000055a662899be0 in virEventPollDispatchHandles (fds=0x55a663a7bdc0, nfds=<optimized out>)
at util/vireventpoll.c:508
#12 virEventPollRunOnce () at util/vireventpoll.c:657
#13 0x000055a6628982f1 in virEventRunDefaultImpl () at util/virevent.c:327
#14 0x000055a6628716d5 in virNetDaemonRun (dmn=0x55a663a771b0) at rpc/virnetdaemon.c:858
#15 0x000055a662864c1d in main (argc=<optimized out>,
#argv=0x7ffd105b4838) at logging/log_daemon.c:1235
Reviewed-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Jim Fehlig <jfehlig@suse.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Currently virNetServerClientDispatchFunc implementations are only
responsible for free'ing the "msg" parameter upon success. Simplify the
calling convention by making it their unconditional responsibility to
free the "msg", and close the client if desired.
Reviewed-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Jim Fehlig <jfehlig@suse.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
There's no reason why the virNetServerClientDispatchRead method needs to
acquire an extra reference on the "client" object. An extra reference is
only needed if the registered dispatch callback is going to keep hold of
the "client" for work in the background. Thus we can push reference
acquisition into virNetServerDispatchNewMessage.
Reviewed-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Jim Fehlig <jfehlig@suse.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
This function was introduced in commit 41f5c2ca27 as a way
to probe the same property for multiple devices at once.
Although the resulting representation is very compact, it
doesn't provide any extra features compared to the existing
virQEMUCapsProcessStringFlags() mechanism, which is already
used for pretty much all device properties.
Drop the custom function and datatypes and start using the
standard ones instead.
Note that, in theory, the end result is not identical
because we're no longer probing properties for
virtio-serial-pci
virtio-9p-pci
virtio-rng-pci
virtio-input-host-pci
virtio-keyboard-pci
virtio-mouse-pci
virtio-tablet-pci
However, chances of any of those devices being compiled
into a QEMU binary where
virtio-balloon-pci
virtio-blk-pci
virtio-scsi-pci
virtio-net-pci
virtio-gpu-pci
are compiled out are slim enough that it doesn't make any
difference in practice, as the lack of test suite churn
shows.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
In some cases, we are probing multiple devices for the same
property and setting the corresponding capability if it's
found on any of the devices: when that happens, we can quit
early after finding the first property and avoiding a bunch
of string comparisons.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Commit 4ae59411fa introduced the ability to make probing for
device properties conditional on a capability being set, but
didn't extend the use of this feature to existing devices.
This commit does the last bit of work, which results in a lot
of pointless QMP chatter no longer happening and our test suite
shrinking a fair bit.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
The function used the 'cleanup' label only in error cases. This patch
makes the code pass the cleanup label in every case and removes few
unnecessary VIR_FREEs.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
In certain cases a xml contains paths that do not yet exist, but
are valid as qemu will create them later on - for example
vhostuser mode=server sockets.
In any such cases so far the check to virFileExists failed and due to
that the paths stayed non-resolved in regard to symlinks.
But for apparmor those non-resolved rules are non functional as they
are evaluated after resolving any symlinks.
Therefore for non-existent files and partially non-existent paths
resolve as much as possible to get valid rules.
Example:
<interface type='vhostuser'>
<model type='virtio'/>
<source type='unix' path='/var/run/symlinknet' mode='server'/>
</interface>
Got rendered as:
"/var/run/symlinknet" rw,
But correct with "/var/run" being a symlink to "/run" is:
"/run/symlinknet" rw,
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
Acked-by: Michal Privoznik <mprivozn@redhat.com>
When commit 3545cbef moved the sysfs attribute reading logic from
_udev.c module to virmdev.c, it had to replace our udev read wrappers
with the ones available from virfile.c. The problem is that the original
logic worked correctly with udev read wrappers which don't return an
error code for a missing attribute, virfile.c readers however - not so
much. Therefore add another parameter to the macro, so we can again
accept the fact that optional attributes may be missing.
Signed-off-by: Erik Skultety <eskultet@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
If the user tries to define a domain that has
<controller type='usb' model='none'/>
and also some USB devices, we report an error:
error: internal error: No free USB ports
Which is technically still correct for a domain with no USB ports.
Change it to:
USB is disabled for this domain, but USB devices are present in the domain XML
https://bugzilla.redhat.com/show_bug.cgi?id=1347550
Signed-off-by: Ján Tomko <jtomko@redhat.com>
When trying to destroy a domain (e.g. because we've seen EOF on
the monitor) we try to acquire QEMU_JOB_DESTROY. However, if
max_queued is set in qemu.conf this may fail and since our code
doesn't count on that we will still report domain as active even
though the qemu process is long gone. More specifically, if we've
seen EOF on the monitor, qemuProcessHandleMonitorEOF() is called
which sends MONITOR_EOF job to the event worker pool and
unregisters monitor from the event loop. The worker pool calls
processMonitorEOFEvent() which tries to set job which may fail
due to the limit as described above.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Fix this common typo and assign a value rather than implicitly
type-casted comparison result. Introduced by commit b6a264e855.
Signed-off-by: Erik Skultety <eskultet@redhat.com>
so it's not affected by flags that might be passed in $(*_LIBS) like
-L/usr/lib which might result in linking against system library and
requiring incorrect version of private symbols
Signed-off-by: Jan Palus <atler@pld-linux.org>
Pretty much any reasonable compiler would do this automatically,
but there's no harm in being explicit about it.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Laine Stump <laine@laine.org>
We allow the postParse callbacks to fail for some reasons (missing
emulator binary) when parsing the configs from /etc/libvirt.
In that case, def->postParseFailed is set to true and the post
parse callbacks are re-executed on domain startup.
However this bool was only set when virDomainDefPostParse was called
with the ALLOW_POST_PARSE_FAIL flag set. If the callback failed
again on domain startup, the bool would be reset and subsequent
startups would not attempt to reexecute the callback.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
There is a pattern of using two temporary utf16/utf8 variables
for every value we get from VirtualBox and put in the domain
definition right away.
Reuse the same variable name to improve the chances of getting
the function on one screen.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Use the virMacAddrParse helper that does not require colon-separated
values instead of using extra code to format it that way.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Use VIR_APPEND_ELEMENT instead and change the return type
to int to catch allocation errors.
This removes the need to figure out the adapter count
upfront.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
