This is a partial revert of 87a43a907f
The change to use g_clear_pointer() in more places was accidentally
applied to cases involving vir_g_source_unref().
In some cases, the ordering of g_source_destroy() and
vir_g_source_unref() was reversed, which resulted in the source being
marked as destroyed, after it is already unreferenced. This
use-after-free case might work in many cases, but with versions of
glib older than 2.64.0 it may defer unref to run within the main
thread to avoid a race condition, which creates a large distance
between the g_source_unref() and g_source_destroy().
In some cases, the call to vir_g_source_unref() was replaced with a
second call to g_source_destroy(), leading to a memory leak or worse.
In our experience, the symptoms were that use of libvirt-python became
slower over time, with OpenStack nova-compute initially taking around
one second to periodically query the host PCI devices, and within an
hour it was taking over a minute to complete the same operation, until
it is was eventually running this query back-to-back, resulting in the
nova-compute process consuming 100% of one CPU thread, losing its
RabbitMQ connection frequently, and showing up as down to the control
plane.
Signed-off-by: Mark Mielke <mark.mielke@gmail.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Signed-off-by: Ján Tomko <jtomko@redhat.com>
When creating a TAP interface we can end up with multiple FDs,
each representing one queue. However, these FDs must be
relabelled as they are then passed to QEMU. In case of
qemuBuildInterfaceConnect() we allocate the array for the FDs and
then let function corresponding to the <interface/> type to fill
the array with FDs. When any of the functions meets an error,
it's also responsible for closing previously opened FDs. However,
the functions take a shortcut: iterate through each member of the
array and close it (if it's non-negative). This assumes that the
array is initialized to negative values, which use to be the case
before rewrite in v8.4.0-rc1~170 but after it it's no longer the
case. Subsequently, "random" FDs are closed (okay, not that
random since the array is allocated via g_new0(), but hey - FD 0
is still valid FD and might be valuable, actually).
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2075383#c18
Fixes: 7a38d3946b
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
For some types of SRIOV interfaces we create a temporary file
where the state of the interface is saved before we start
modifying it. The file is used then to restore the original
configuration when the interface is no longer associated with any
guest. For writing the file virFileWriteStr() is used. However,
it's given wrong argument: the last argument is supposed to be
mode to create the file with but virNetDevSaveNetConfig() passes
open(2) flags (O_CREAT|O_TRUNC|O_WRONLY). We need the file to be
writable and readable by root only (0600). Therefore, pass that
mode instead of gibberish.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Commit v7.1.0-136-g6a6d6bb520 refactored virProcessSetMaxMemLock by
moving its part into a new virProcessSetLimit, but lost "return -1" on
error.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Since the main-loop and iothread classes are derived from the
same class (EventLoopBaseClass) we don't need new capability and
can use QEMU_CAPS_IOTHREAD_THREAD_POOL_MAX directly to check
whether QEMU's capable of setting defaultiothread pool size.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
As of v7.0.0-877-g70ac26b9e5 QEMU exposes its default event loop
for devices with no IOThread assigned as an QMP object. In the
very next commit (v7.0.0-878-g71ad4713cc) it was extended for
thread-pool-min and thread-pool-max attributes. Expose them under
new <defaultiothread/> element.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Since virsh implements a wrapper over virDomainSetIOThreadParams()
(command iothreadset) let's wire up new typed parameters there too.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Introduced in previous commit, QEMU driver needs to be taught how
to set VIR_DOMAIN_IOTHREAD_THREAD_POOL_MIN and
VIR_DOMAIN_IOTHREAD_THREAD_POOL_MAX parameters on given IOThread.
Fortunately, this is fairly trivial to do and since these two
parameters are exposed in domain XML too the update of inactive
XML can be wired up too.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Our public API offers virDomainSetIOThreadParams() function which
allows users to set various aspects of IOThreads. Introduce two
new typed parameters: VIR_DOMAIN_IOTHREAD_THREAD_POOL_MIN and
VIR_DOMAIN_IOTHREAD_THREAD_POOL_MAX which will allow users to
modify the thread-pool-min and thread-pool-max attributes of an
iothread.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Now that we have a capability that reflects whether QEMU is
capable of setting iothread pool size, let's introduce a
validator check to make sure users are not trying to use this
feature with QEMU that doesn't support it.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
This capability reflects whether QEMU allows setting
thread-pool-min and thread-pool-max attributes on iothread
object. Since both attributes were introduced in the same commit
(v7.0.0-878-g71ad4713cc) and can't exist independently of each
other we can stick with one capability covering both of them.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
At least in case of QEMU an IOThread is actually a pool of
threads (see iothread_set_aio_context_params() in QEMU's code
base). As such, it can have minimal and maximal number of worker
threads. Allow setting them in domain XML.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
So far, iothread configuration structure (virDomainIOThreadIDDef)
is allocated by plain g_new0(). This is perfectly okay because
all members of the struct default to value 0 anyway. But soon
this is going to change. Therefore, replace those g_new0() with a
function so that the default value can be set consistently in one
place.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Formatting iothreads is currently open coded inside of
virDomainDefFormatInternalSetRootName(). While this works, it
makes the function needlessly long, especially if the formatting
code will expand in near future. Therefore, move it into a
separate function. At the same time, make
virDomainDefIothreadShouldFormat() accept const domain definition
so that the new function can also accept const domain definition.
Formatters shouldn't need to change definition.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
In virDomainIOThreadIDDefArrayInit() the variable @iothrid is
used only inside a loop but is declared for whole function. Bring
the variable into the loop so that it's obvious that the variable
is not used elsewhere.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Using g_autoptr() for @iothrid variable inside
virDomainDefParseIOThreads() allows us to drop explicit call to
virDomainIOThreadIDDefFree() in one case.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
For easier attribute parsing we have virXMLProp*() family of
functions. These accept flags through which a caller can pose
some conditions onto the attribute value, for instance:
VIR_XML_PROP_NONZERO when the attribute may not be zero, etc.
What we are missing is VIR_XML_PROP_NONNEGATIVE when the
attribute value may be non-negative. Obviously, this flag makes
sense only for some members of the virXMLProp*() family.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
This requires publishing the RPMs as artifacts from the regular
build job.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Since a fix for CVE-2022-24765 was released every git command is now
checked against the context repo in which it's supposed to run
resulting in a fatal error if the repo is owned by other user than the
one running the git command.
This means that in order to be able to do 'sudo make install', we have
to set the 'safe.directory' for the root user. This is because QEMU
runs 'git submodule update' automatically on 'make install'.
Signed-off-by: Erik Skultety <eskultet@redhat.com>
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Virsh has iothreadset command which allows setting various
attributes of IOThreads. However, when the command is called
without any arguments (besides domain and IOThread IDs), then
@params stays NULL and is passed to virDomainSetIOThreadParams()
which produces rather user unfriendly error message:
error: params in virDomainSetIOThreadParams must not be NULL
Introduce a check and produce better error message.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Claudio Fontana <cfontana@suse.de>
They were constructed from two separate strings using "%s: %s", which
is ugly and does not work well with translations.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
When a domain has a guest agent channel enabled and the agent is running
in the guest, we will get VSERPORT_CHANGE event on a destination host as
soon as we start vCPUs there. This is not an issue for normal migration,
but post-copy migration will remain running after we started vCPUs on
the destination. If it runs for more than 30s, the VSERPORT_CHANGE event
handler will fail to get a job and log the following error message:
Timed out during operation: cannot acquire state change lock (held
by monitor=remoteDispatchDomainMigrateFinish3Params)
and of course we will think the guest agent is not connected and thus
all APIs talking to it will fail. Until the agent or libvirt daemon is
restarted.
Luckily we only need to update the channel state (to mark it as
connected) and connect to the agent neither of which conflicts with
migration. Thus we can safely enable processing this event during
migration.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
This is a special job for operations that need to modify domain state
during an active migration. The modification must not affect any state
that could conflict with the migration code. This is useful mainly for
event handlers that need to be processed during migration and which
could otherwise time out on acquiring a normal MODIFY job.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
The original virDomainAbortJob did not support flags.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
Since all parts of post-copy recovery have been implemented now, it's
time to enable the corresponding flag.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
Every single call to qemuMigrationJobContinue needs to register a
cleanup callback in case the migrating domain dies between phases or
when migration is paused due to a failure in postcopy mode.
Let's integrate registering the callback in qemuMigrationJobContinue to
make sure the current thread does not release a migration job without
setting a cleanup callback.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
The callback will properly cleanup non-p2p migration job in case the
migrating domain dies between Begin and Perform while the client which
controls the migration is not cooperating (normally the API for the next
migration phase would handle this).
The same situation can happen even after Prepare and Perform phases, but
they both already register a suitable callback, so no fix is needed
there.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
Normally the structure is created once the source reports completed
migration, but with post-copy migration we can get here even after
libvirt daemon was restarted. It doesn't make sense to preserve the
structure in our status XML as we're going to rewrite almost all of it
while refreshing the stats anyway. So we just create the structure here
if it doesn't exist to make sure we can properly report statistics of a
completed migration.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
Everything was already done in the normal Finish phase and vCPUs are
running. We just need to wait for all remaining data to be transferred.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
The QEMU process is already running, all we need to do is to call
migrate-recover QMP command. Except for some checks and cookie handling,
of course.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
Non-postcopy case talks to QEMU monitor and thus needs to create a
nested job. Since qemuMigrationAnyConnectionClosed is called in case
there's no thread processing a migration API, we need to make the
current thread a temporary owner of the migration job to avoid "This
thread doesn't seem to be the async job owner: 0". This is done by
starting a migration phase.
While no monitor interaction happens in postcopy case and just setting
the phase (to indicate a broken postcopy migration) would be enough,
being consistent and setting the owner does not hurt anything.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
To prepare the code for handling incoming migration too.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
The function is now called qemuMigrationAnyConnectionClosed to make it
clear it is supposed to handle broken connection during migration. It
will soon be used on both sides of migration so the "Src" part was changed
to "Any" to avoid renaming the function twice in a row.
The original *Cleanup name could easily be confused with cleanup
callbacks called when a domain is destroyed.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
