Ports allocated on virtual networks with type=nat|route|open all get
given an actual type of 'network'.
Only ports in networks with type=bridge use an actual type of 'bridge'.
This distinction makes little sense since the virtualization drivers
will treat both actual types in exactly the same way, as they're all
just bridge devices a VM needs to be connected to.
This doesn't affect user visible XML since the "actual" device XML
is internal only, but we need code to convert the data upgrades.
Reviewed-by: Laine Stump <laine@laine.org>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
The virNetDevBandwidthParse method uses the interface type to decide
whether to allow use of the "floor" parameter. Using the interface
type is not convenient as callers may not have that available, but
still wish to allow use of "floor". Switch to an explicit boolean
to control its usage.
Reviewed-by: Laine Stump <laine@laine.org>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Reword error messages to make it clear that the combined floor settings
of all NICs are exceeding the network inbound peak/average
settings. Including the actual values being checked helps to diagnose
what is actually wrong.
Reviewed-by: Laine Stump <laine@laine.org>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
In extreme cases libvirt can get mixed up about what VMs are running and
attached to a network leading to the cached floor sum value being
outdated. When this happens the only option is to destroy the network
and then restart libvirtd. If we set floor sum back to zero when
starting the network, we avoid the need for a libvirtd restart at least.
Reviewed-by: Laine Stump <laine@laine.org>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Replaced all virSaveLastError and virSetError/virFreeError usages to
virErrorPreserveLast and virErrorRestore respectively.
Reviewed-by: Cole Robinson <crobinso@redhat.com>
Signed-off-by: Syed Humaid <syedhumaidbinharoon@gmail.com>
The networkPlugBandwidth & networkUnplugBandwidth methods currently take
a virDomainNetDefPtr. To remove the dependency on the domain config
struct, pass individual parameters instead.
Reviewed-by: Laine Stump <laine@laine.org>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
All but one of the network types supports port profiles. Rather than
duplicating the code to merge profiles 3 times, do it once and then
later report an error if used from the wrong place.
Reviewed-by: Laine Stump <laine@laine.org>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Firstly, VIR_STRDUP() accepts NULL, so there is no need to check
if the string we want to duplicate is not-NULL. Secondly,
virDomainNetSetModelString() also accepts NULL. Thirdly, we have
VIR_AUTOFREE().
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
f1056279 removed virDomainSnapshotDef.current and leaved
using vm->current_snapshot only. Later 4819f54bd moved current snapshot
tracking to virDomainSnapshotObjList. As vz driver never used
vm->current_snaphot this patch includes fixes after both commits.
Reviewed-by: Cole Robinson <crobinso@redhat.com>
ACKed-by: Maxim Nestratov <mnestratov@virtuozzo.com>
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
Wire up the accessor functions necessary for the testsuite to install
an alternative post-parse handler from normal drivers. I could have
modified the signature for virDomainXMLOptionNew() to take another
parameter, but thought it was easier to add a new set function rather
than chase down all existing callers. Until code actually sets the
override, there is no change in behavior.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Cole Robinson <crobinso@redhat.com>
Move the non-deterministic code that sets snapshot properties
independently of what the incoming XML described to instead live in a
default post-parse function common to virDomainMoment (as checkpoints
will also reuse it in later patches). This patch is just code motion,
with no difference to any callers; but the next patch will further
refactor things to allow for a per-driver override, used by the
testsuite to perform deterministic post-parse actions for better
coverage of parser/formatter code.
Note that the post-parse code is intentionally not run during a
snapshot redefine, since that code path already requires a valid
snapshot name and creation time from the XML.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Cole Robinson <crobinso@redhat.com>
None of the existing drivers actually use the 0-valued 'nostate'
snapshot state; rather, it was a fluke of implementation. In fact,
some drivers, like qemu, actively reject 'nostate' as invalid during a
snapshot redefine. Normally, a driver computes the state post-parse
from the current domain, and thus virDomainSnapshotGetXMLDesc() will
never expose the state. However, since the testsuite lacks any
associated domain to copy state from, and lacks post-parse processing
that normal drivers have, the testsuite output had several spots with
the state, coupled with a regex filter to ignore the oddity.
It is better to follow the lead of other XML defaults, by not
outputting anything during format if post-parse defaults have not been
applied, and rejecting the default value during parsing. The testsuite
needs a bit of an update, by adding another flag for when to simulate
a post-parse action of setting a snapshot state, but none of the
drivers are impacted other than rejecting XML that was previously
already suspicious in nature.
Similarly, don't expose creation time 0 (for now, only possible if a
user redefined a snapshot to claim creation at the Epoch, but also
happens once setting the creation time is deferred to a post-parse
handler).
This is also a step towards cleaning up snapshot_conf.c to separate
its existing post-parse work (namely, setting the creationTime and
default snapshot name) from the pure parsing work, so that we can get
rid of the testsuite hack of regex filtering of the XML and instead
have more accurate testing of our parser/formatter code.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Cole Robinson <crobinso@redhat.com>
This requires drivers to opt in to handle the raw modelstr
network model, all others will error if a passed in XML value
is not in the model enum.
Enable this feature for libxl/xen/xm and qemu drivers
Acked-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
Convert the vbox driver to net model enum, which requires adding
enum values for Am79C970A, Am79C973, 82540EM, 82545EM, 82543GC. We
preserve the same casing that vbox historically used for these model
names.
Remove the now unused virDomainNetStrcaseeqModelString
Acked-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
Convert the vmware/vmx driver to net model enum, which requires
adding enum values for vlance, vmxnet, vmxnet2, and vmxnet3.
Acked-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
vbox and vmx drivers do net case insensitive net model comparisons,
so for example 'VMXNET3' and 'vmxnet3' and 'VmxNeT3' in the XML will
translate to the same driver configuration. To convert these drivers
to use net model enum, we will need to do case insensitive comparisons
as well.
Essentially we implement virEnumToString, but with case insensitive
comparison. XML will always be formatted with the enum model string
we track internally, but we will accept any case insensitive variant.
Acked-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
This converts the qemu driver to the net model enum, for all
the model values that we have hardcoded for various checks,
which adds e1000e, virtio-transitional, virtio-non-transitional,
usb-net, spapr-vlan, lan9118, smc91c111
Because the qemu driver has historically also allowed the raw
model string onto the qemu command line, this isn't a full
conversion. Unwinding that will require more thought. However
for all new driver code we should be adding explicit enum
values for any model name we have special handling for.
Remove the now unused virDomainNetStreqModelString
Acked-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
The bhyve driver only works with the virtio and e1000 models,
which we already have in the enum. Some error reporting is
slightly downgraded to avoid some subtle usage of modelstr
Acked-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
The vz driver only handles three models: virtio, e1000, and rtl8139.
Add enum values for those models, and convert the vz driver to
handling net->model natively
Acked-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
This adds a network model enum. The virDomainNetDef property
is named 'model' like most other devices.
When the XML parser or a driver calls NetSetModelString, if
the passed string is in the enum, we will set net->model,
otherwise we copy the string into net->modelstr
Add a single example for the 'netfront' xen model, and wire
that up, just to verify it's all working
Acked-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
We will be adding a 'model' enum in upcoming patches. Rename
the existing value to make the differentiation clear
Acked-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
To ease converting the net->model value to an enum, add
the wrapper functions:
virDomainNetGetModelString
virDomainNetSetModelString
virDomainNetStreqModelString
virDomainNetStrcaseeqModelString
Acked-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
Switch over to use the new API for re-attaching the bridge device
Reviewed-by: Cole Robinson <crobinso@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
In the case of a network with forward=bridge, which has a bridge device
listed, we are capable of setting bandwidth limits but fail to call the
function to register them.
Reviewed-by: Cole Robinson <crobinso@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
hostdevs have a link back to the original network device. This is fairly
generic accepting any type of device, however, we don't intend to make
use of this approach in future. It can thus be specialized to network
devices.
Reviewed-by: Cole Robinson <crobinso@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
The actual network def was updated to save the bridge name back
in 1.2.11:
commit a360912179
Author: Laine Stump <laine@laine.org>
Date: Fri Nov 21 12:20:37 2014 -0500
network: save bridge name in ActualNetDef when actualType==network too
The chance that someone is running libvirt < 1.2.11 and wants
todo a live upgrade to 5.3.0 without a host reboot is essentially
zero. We can thus reasonably drop the back compat code now.
Reviewed-by: Laine Stump <laine@laine.org>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
The APIs for allocating/notifying/removing network ports just take
an internal domain interface struct right now. As a step towards
turning these into public facing APIs, add a virNetworkPtr argument
to all of them.
Reviewed-by: Cole Robinson <crobinso@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
The port allocation APIs are currently called unconditionally for all
types of NIC, but (mostly) only do anything for NICs with type=network.
The exception is the port allocate API which does some validation even
for NICs with type!=network. Relying on this validation is flawed,
however, since the network driver may not even be installed. IOW virt
drivers must not delegate validation to the network driver for NICs
with type != network.
This change allows us to report errors when the virtual network driver
is not registered.
Reviewed-by: Cole Robinson <crobinso@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
This helps in a scenarios where vCPUs run with a priority that is so high they
might starve the emulator thread. And it also fits with the rest of the
settings.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
This will be used later when we want to format emulator scheduler parameters
which don't apply for multiple threads.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
This will become useful later when parsing emulatorsched parameters which don't
need the rest of the current function.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Qemu commit 767abe7 ("chardev: forbid 'wait' option with client
sockets") effectively deprecates usage of "wait" with client sockets
starting with qemu 4.0, and earlier versions ignored the value.
Cc: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Failed new gnutls context allocations in virNetTLSContextNew function
results in double free and segfault. Occasional memory leaks may also
occur.
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Adrian Brzezinski <redhat@adrb.pl>
We're setting seclabels on unix sockets but never restoring them.
Surprisingly, we are doing so in SELinux driver.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
When writing the VMX file from the domain XML, write the firmware key
according to the firmware autoselection. Though, at the moment only
'efi' is supported.
Reviewed-by: Cole Robinson <crobinso@redhat.com>
Signed-off-by: Pino Toscano <ptoscano@redhat.com>
Convert the firmware key to a type of autoselected firmware.
Only the 'efi' firmware is allowed for now, in case the key is present.
It seems VMware (at least ESXi) does not write the key in VMX files when
setting BIOS as firmware.
Reviewed-by: Cole Robinson <crobinso@redhat.com>
Signed-off-by: Pino Toscano <ptoscano@redhat.com>
The network driver used to reload the firewall rules whenever a dbus
NameOwnerChanged message for org.fedoraproject.FirewallD1 was
received. Presumably at some point in the past this was successful at
reloading our rules after a firewalld restart. Recently though I
noticed that once firewalld was restarted, libvirt's logs would get this
message:
The name org.fedoraproject.FirewallD1 was not provided by any .service files
After this point, no networks could be started until libvirtd itself
was restarted.
The problem is that the NameOwnerChanged message is sent twice during
a firewalld restart - once when the old firewalld is stopped, and
again when the new firewalld is started. If we try to reload at the
point the old firewalld is stopped, none of the firewalld dbus calls
will succeed.
The solution is to check the new_owner field of the message - we
should reload our firewall rules only if new_owner is non-empty (it is
set to "" when firewalld is stopped, and some sort of epoch number
when it is again started).
Signed-off-by: Laine Stump <laine@laine.org>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
When virDBusMessageRead() and virDBusMessageDecode were first added in
commit 834c9c94, they were identical except that virDBusMessageRead()
would unref the message after decoding it.
This difference was eliminated later in commit dc7f3ffc after it
became apparent that unref-ing the message so soon was never the right
thing to do. The two identical functions remained though, with the
tests and virDBus library itself calling the Decode variant, and all
other users calling the Read variant.
This patch eliminates the duplication, switching all users to
virDBusMessageDecode (and moving the nice API documentation comment
from the Read function up to the Decode function).
Signed-off-by: Laine Stump <laine@laine.org>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Support for compiling this version was dropped in an earlier commit.
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Support for compiling this version was dropped in an earlier commit.
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Support for compiling this version was dropped in an earlier commit.
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Support for compiling this version was dropped in an earlier commit.
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Support for compiling this version was dropped in an earlier commit.
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Support for compiling this version was dropped in an earlier commit.
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Support for all the 4.x releases was ended by VirtualBox maintainers in
Dec 2015. Even the "newest" 4.3.40 of those is only supported on old
versions of Linux (Ubuntu <= 13.03, RHEL <= 6, SLES <= 11), which are all
discontinued hosts from libvirt's POV.
We can thus reasonably drop all 4.x support from the libvirt VirtualBox
driver.
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Since:
commit 9f4e35dc73
Author: Daniel P. Berrangé <berrange@redhat.com>
Date: Mon Mar 18 17:31:21 2019 +0000
network: improve error report when firewall chain creation fails
We cache an error when failing to create the top level firewall chains.
This commit failed to account for fact that we may invoke
networkPreReloadFirewallRules() many times while libvirtd is running.
For example when firewalld is restarted.
When this happens the original failure may no longer occurr and we'll
successfully create our top level chains. We failed to clear the cached
error resulting in us failing to start virtual networks.
Reviewed-by: Laine Stump <laine@laine.org>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Spotted by Lintian (manpage-has-bad-whatis-entry tag).
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
We need commit 6280c94f306d in order to fix our generated
man pages.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
My earlier commit be46f61326 was incomplete. It removed caching of
microcode version in the CPU driver, which means the capabilities XML
will see the correct microcode version. But it is also cached in the
QEMU capabilities cache where it is used to detect whether we need to
reprobe QEMU. By missing the second place, the original commit
be46f61326 made the situation even worse since libvirt would report
correct microcode version while still using the old host CPU model
(visible in domain capabilities XML).
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Support for kqemu was dropped in libvirt by commit 8e91a400c and even
back then we never set these capabilities when doing QMP probing.
Since no QEMU we aim to support has these, drop them completely.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
In e17d10386 these functions were mistakenly moved into an #ifdef
block, but remained used outside of it leaving the build broken
for platforms where #ifdef evaluated to false.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
Model specific registers are a thing only on x86. Also, the
/dev/cpu/0/msr path exists only on Linux and the fallback
mechanism (asking KVM) exists on Linux and FreeBSD only.
Therefore, move the function within #ifdef that checks all
aforementioned constraints and provide a dummy stub for all
other cases.
This fixes the build on my arm box, mingw-* builds, etc.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
The NVIDIA V100 GPU has an onboard RAM that is mapped into the
host memory and accessible as normal RAM via an NVLink2 bridge. When
passed through in a guest, QEMU puts the NVIDIA RAM window in a
non-contiguous area, above the PCI MMIO area that starts at 32TiB.
This means that the NVIDIA RAM window starts at 64TiB and go all the
way to 128TiB.
This means that the guest might request a 64-bit window, for each PCI
Host Bridge, that goes all the way to 128TiB. However, the NVIDIA RAM
window isn't counted as regular RAM, thus this window is considered
only for the allocation of the Translation and Control Entry (TCE).
For more information about how NVLink2 support works in QEMU,
refer to the accepted implementation [1].
This memory layout differs from the existing VFIO case, requiring its
own formula. This patch changes the PPC64 code of
@qemuDomainGetMemLockLimitBytes to:
- detect if we have a NVLink2 bridge being passed through to the
guest. This is done by using the @ppc64VFIODeviceIsNV2Bridge function
added in the previous patch. The existence of the NVLink2 bridge in
the guest means that we are dealing with the NVLink2 memory layout;
- if an IBM NVLink2 bridge exists, passthroughLimit is calculated in a
different way to account for the extra memory the TCE table can alloc.
The 64TiB..128TiB window is more than enough to fit all possible
GPUs, thus the memLimit is the same regardless of passing through 1 or
multiple V100 GPUs.
Further reading explaining the background
[1] https://lists.gnu.org/archive/html/qemu-devel/2019-03/msg03700.html
[2] https://www.redhat.com/archives/libvir-list/2019-March/msg00660.html
[3] https://www.redhat.com/archives/libvir-list/2019-April/msg00527.html
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
The NVLink2 support in QEMU implements the detection of NVLink2
capable devices by verifying the attributes of the VFIO mem region
QEMU allocates for the NVIDIA GPUs. To properly allocate an
adequate amount of memLock, Libvirt needs this information before
a QEMU instance is even created, thus querying QEMU is not
possible and opening a VFIO window is too much.
An alternative is presented in this patch. Making the following
assumptions:
- if we want GPU RAM to be available in the guest, an NVLink2 bridge
must be passed through;
- an unknown PCI device can be classified as a NVLink2 bridge
if its device tree node has 'ibm,gpu', 'ibm,nvlink',
'ibm,nvlink-speed' and 'memory-region'.
This patch introduces a helper called @ppc64VFIODeviceIsNV2Bridge
that checks the device tree node of a given PCI device and
check if it meets the criteria to be a NVLink2 bridge. This
new function will be used in a follow-up patch that, using the
first assumption, will set up the rlimits of the guest
accordingly.
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
This does not cause a problem in usual scenarios thanks to us allowing
CAP_DAC_OVERRIDE for the qemu process, however in some scenarios this might be
an issue because the directory is created with mkdtemp(3) which explicitly
creates that with 0700 permissions and qemu running as non-root cannot access
that.
The scenarios include:
- Builds without CAPNG
- Running libvirtd in certain container configurations [1]
- and possibly others.
[1] https://github.com/kubevirt/kubevirt/pull/2181#issuecomment-481840304
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
The new virHostCPUGetMSR internal API will try to read the MSR from
/dev/cpu/0/msr and if it is not possible (the device does not exist or
libvirt is running unprivileged), it will fallback to asking KVM for the
MSR using KVM_GET_MSRS ioctl.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
They are static and we will need to call them a little bit closer to the
beginning of the file.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The structure can only be used for CPUID data now. Adding a type
indicator and moving the data into a union will let us store alternative
data types.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The function now works on virCPUx86DataItem and it's called
virCPUx86DataItemMatch.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The function is renamed as virCPUx86DataItemMatchMasked to reflect the
change in parameter types.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The function now works on virCPUx86DataItem and it's renamed as
virCPUx86DataItemAndBits.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The parameters changed from virCPUx86CPUID to virCPUx86DataItem and the
function is now called virCPUx86DataItemClearBits.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The function is renamed as virCPUx86DataItemSetBits and it works on
virCPUx86DataItem now.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
virCPUx86DataSorter already compares two virCPUx86DataItem structs.
Let's add a tiny wrapper around it called virCPUx86DataCmp and use it
instead of open coded comparisons.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
It is called virCPUx86DataSorter since the function will work on any CPU
data type.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The function is now called virCPUx86DataNext to reflect its purpose: it
is an iterator over CPU data (both CPUID and MSR in the near future).
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Although vendor string is always reported by CPUID, the container struct
is used for consistency and thus "cpuid" name is not a good fit anymore.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The following patches introduce CPU features read from MSR in addition
to those queried via CPUID instruction. Let's introduce a container
struct which will be able to describe either feature type.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Introduced in QEMU 3.1.0 by commit
c7a88b52f62b30c04158eeb07f73e3f72221b6a8
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Vim has trouble figuring out the filetype automatically because
the name doesn't follow existing conventions; annotations like
the ones we already have in Makefile.ci help it out.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Firstly, virCommandRun() does report an error on failure (which
in most cases is more accurate than what we overwrite it with).
Secondly, usually errno is not set (or gets overwritten in the
cleanup code) which makes virReportSystemError() report useless
error messages. Drop all virReportSystemError() calls in cases
like this (I've found three occurrences).
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
The latter is deprecated and will be removed soon. The advised
replacement is '-overcommit mem-lock=on|off'.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Added in QEMU commit of v3.0.0-rc0~48^2~9 (then fixed by
v3.1.0-rc0~119^2~37) QEMU is replacing '-realtime mlock' with
'-overcommit mem-lock'. Add a capability to tell if we're dealing
new new enough qemu to use the replacement.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The '-realtime mlock' cmd line argument was introduced in QEMU
commit v1.5.0-rc0~190 which matches minimal QEMU version we
require. Therefore, the capability will always be present.
Apparently, nearly none of our xml2argv test cases had the
capability hence slightly bigger change under qemuxml2argvdata/.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The 'bandwidths' variable is allocated using VIR_RESIZE_N so it has to
be freed as well.
==118315== 8 bytes in 1 blocks are definitely lost in loss record 299 of 2,401
==118315== at 0x4C29DAD: malloc (vg_replace_malloc.c:308)
==118315== by 0x4C2C100: realloc (vg_replace_malloc.c:836)
==118315== by 0x52C3FAF: virReallocN (viralloc.c:245)
==118315== by 0x52C4079: virExpandN (viralloc.c:294)
==118315== by 0x532BBA8: virResctrlAllocParseProcessMemoryBandwidth (virresctrl.c:1156)
==118315== by 0x532BBA8: virResctrlAllocParseMemoryBandwidthLine (virresctrl.c:1211)
==118315== by 0x532BBA8: virResctrlAllocParse (virresctrl.c:1414)
==118315== by 0x532BBA8: virResctrlAllocGetGroup (virresctrl.c:1446)
==118315== by 0x532C11D: virResctrlAllocGetDefault (virresctrl.c:1464)
==118315== by 0x532D15E: virResctrlAllocAssign (virresctrl.c:1923)
==118315== by 0x532D15E: virResctrlAllocCreate (virresctrl.c:2042)
==118315== by 0x31E1ABEE: qemuProcessResctrlCreate (qemu_process.c:2596)
==118315== by 0x31E1ABEE: qemuProcessLaunch (qemu_process.c:6444)
==118315== by 0x31E1E341: qemuProcessStart (qemu_process.c:6721)
==118315== by 0x31E81315: qemuDomainObjStart.constprop.50 (qemu_driver.c:7288)
==118315== by 0x31E81A65: qemuDomainCreateWithFlags (qemu_driver.c:7341)
==118315== by 0x54DDB4B: virDomainCreate (libvirt-domain.c:6534)
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
A bunch of files include src/rpc/virnetsaslcontext.h, which
in turn includes <sasl/sasl.h>, and without the corresponding
CFLAGS the compiler can't locate the latter if it happens to
be installed outside of the default include path as is the
case, for example, on FreeBSD.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Standardize on putting the _LAST enum value on the second line
of VIR_ENUM_IMPL invocations. Later patches that add string labels
to VIR_ENUM_IMPL will push most of these to the second line anyways,
so this saves some noise.
Signed-off-by: Cole Robinson <crobinso@redhat.com>
Getting the guest time and hostname both require use of guest agent
commands. These must not be allowed for read-only users, so the
permissions check must validate "write" permission not "read".
Fixes CVE-2019-3886
Reviewed-by: Jim Fehlig <jfehlig@suse.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
The virDomainGetHostname API is fetching guest information and this may
involve use of an untrusted guest agent. As such its use must be
forbidden on a read-only connection to libvirt.
Fixes CVE-2019-3886
Reviewed-by: Jim Fehlig <jfehlig@suse.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Few of the scripts in build-aux are included in EXTRA_DIST. This is not
a serious problem since they are primarily tools intended for developers
upstream, and downstream builds won't need them. Having them missing,
however, complicates downstream patching because it means patches that
are auto-exported from git will fail to apply if they include a change
to a file in build-aux/. By bundling all these scripts in the dist we
make patching more straightforward.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
The function open-codes addition into an array. Use the helper instead.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Demonstrate how VIR_RETURN_PTR is used by refactoring qemu_block.c
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
With the introduction of more and more internal data types which support
VIR_AUTOPTR it's becoming common to see the following pattern:
VIR_AUTOPTR(virSomething) some = NULL
virSomethingPtr ret = NULL;
... (ret is not touched ) ...
VIR_STEAL_PTR(ret, some);
return ret;
This patch introduces a macro named VIR_RETURN_PTR which returns the
pointer directly without the need for an explicitly defined return
variable and use of VIR_STEAL_PTR. Internally obviously a temporary
pointer is created to allow setting the original pointer to NULL so that
the VIR_AUTOPTR function does not free the memory which we want to
actually return.
The name of the temporary variable is deliberately long and complex to
minimize the possibility of collision.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
This is a locally used helper struct but we can make use of automatic
freeing for it.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Commit 6864d8f740 moved this one level up
for qemuBuildMemoryBackendProps but left qemuBuildMemPathStr intact.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
If a management application wants to use firmware auto selection
feature it can't currently know if the libvirtd it's talking to
support is or not. Moreover, it doesn't know which values that
are accepted for the @firmware attribute of <os/> when parsing
will allow successful start of the domain later, i.e. if the mgmt
application wants to use 'bios' whether there exists a FW
descriptor in the system that describes bios.
This commit then adds 'firmware' enum to <os/> element in
<domainCapabilities/> XML like this:
<enum name='firmware'>
<value>bios</value>
<value>efi</value>
</enum>
We can see both 'bios' and 'efi' listed which means that there
are descriptors for both found in the system (matched with the
machine type and architecture reported in the domain capabilities
earlier and not shown here).
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Acked-by: Laszlo Ersek <lersek@redhat.com>
The point of this API is to fetch all FW descriptors, parse them
and return list of supported interfaces and SMM feature for given
combination of machine type and guest architecture.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Acked-by: Laszlo Ersek <lersek@redhat.com>
This reverts commit a5e1602090.
Getting rid of unistd.h from our headers will require more work than
just fixing the broken mingw build. Revert it until I have a more
complete proposal.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
util/virutil.h bogously included unistd.h. Drop it and replace it by
including it directly where needed.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
virutil.(c|h) is a very gross collection of random code. Remove the enum
handlers from there so we can limit the scope where virtutil.h is used.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
'viralloc.h' does not provide any type or macro which would be necessary
in headers. Prevent leakage of the inclusion.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Keeping them with viralloc.h forcibly pulls in the other stuff from
viralloc.h into other header files. This in turn creates a mess
as more and more headers pull in the 'viral' header file.
If we want to make 'viralloc.h' omnipresent we should pick a different
approach.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Define VMX_CONFIG_FORMAT_ARGV to replace the hardcoded 'vmware-vmx'
string used by the domxml-X-native APIs. This follows the pattern used
by other drivers.
Reviewed-by: Cole Robinson <crobinso@redhat.com>
Signed-off-by: Han Han <hhan@redhat.com>
Now that the memory disposal is handled automatically we can simplify
the cleanup paths. In this case it's not as simple as sometimes the
value of the called function is returned.
While at it fix the initialization value of the returned variable.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
This commit fixes an unitialized variable to avoid garbage value
when virNetDevBridgeGet method returns error. When, that method fails
before initialize 'val' variable, it can cause problems related to
that.
Signed-off-by: Julio Faracco <jcfaracco@gmail.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Commit b647d2195 introduced a use-after-free situation when the caller
is trying to delete a snapshot and its children: if the callback
function deletes the parent, it is no longer safe to query the parent
to learn which children also need to be deleted (where we previously
saved deleting the parent for last). To fix the problem, while still
maintaining support for topological visits of callback functions, we
have to stash off any information needed for later traversal prior to
using a callback function (virDomainMomentForEachChild already does
this, it is only virDomainMomentActOnDescendant that was running into
problems).
Sadly, the testsuite did not cover the problem at the time. Worse,
even though I later added commit 280a2b41e to catch problems like
this, and even though that test is indeed sufficient to detect the
problem when run under valgrind or suitable MALLOC_PERTURB_ settings,
I'm guilty of not running the test in such an environment. Thus,
v5.2.0 has a regression that could have been prevented had we used the
testsuite to its full power. On the bright side, deleting snapshots
requires ACL domain:snapshot, which is arguably as powerful as
domain:write, so I don't think this use-after-free forms a security
hole.
At some point, it would be nice to convert virDomainMomentObj into a
virObject, at which point, the solution is even simpler: add
virObjectRef/Unref around the callback. But as that will require
auditing even more places in the code, I went with the simplest patch
for the regression fix.
Fixes: b647d2195
Reported-by: Roman Bogorodskiy <bogorodskiy@gmail.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
Tested-by: Roman Bogorodskiy <bogorodskiy@gmail.com>
A feature with no cpuid element is invalid and it should not be silently
treated as a feature with all CPUID bits set to zero.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The microcode version checks are used to invalidate cached CPU data we
get from QEMU. To minimize /proc/cpuinfo parsing the microcode version
was only read when libvirtd started and cached for the daemon's
lifetime. However, the CPU microcode can change anytime (updating the
microcode package can automatically upload it to the CPU) and we need to
stop caching it to avoid using stale CPU model data.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
This helper has solely to do with virObjects. Move it together with
other virObject stuff.
This also avoids the potential problem where VIR_AUTOUNREF uses
virObjectAutoUnref which is defined in virobject.h.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The rules are the same for all virt guests, regardless of the
architecture.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Cole Robinson <crobinso@redhat.com>
Our PCIe topology depends on the availability of PCIe Root Ports,
so if none of the suitable devices (pcie-root-port, ioh3420) is
compiled into QEMU we should fall back to virtio-mmio rather than
trying to use PCI addresses only to fail immediately afterwards
when we realize we can't use the necessary controllers.
Note that this additional check is basically moot for ARM virt
guests, because PCIe Root Ports were enabled in QEMU builds for
the architecture well before guest OS support had been widely
available; however, the opposite is true for RISC-V, and tweaking
the code this way will allow us to share it between architectures.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Cole Robinson <crobinso@redhat.com>
Since the STOP event handler can use the pausedReason as sent to
qemuProcessStopCPUs, we no longer need to send duplicate suspended
lifecycle events because we know what caused the stop along with extra
details. This processing allows us to also remove the duplicated state
change from qemuProcessStopCPUs.
Reviewed-by: John Ferlan <jferlan@redhat.com>
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
Map is based on existing cases in code where we send suspended
event after changing domain state to paused.
Reviewed-by: John Ferlan <jferlan@redhat.com>
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
Similar to commit [1] which saves and passes the running reason to
the RESUME event handler, during qemuProcessStopCPUs let's save and pass
the pause reason in the domain private data so that the STOP event
handler can use it.
[1] 5dab984ed : qemu: Pass running reason to RESUME event handler
Reviewed-by: John Ferlan <jferlan@redhat.com>
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
If there are two concurrent threads, one of which is removing an
nwfilter from the list and the other is trying to add it back they
may serialize in the following order:
1) obj->removing is set and @obj is unlocked.
2) The tread that's trying to add the nwfilter onto the list locks
the list and tries to find, if the nwfilter already exists.
3) Our lookup functions say it doesn't, so the thread proceeds to
virHashAddEntry() which fails with 'Duplicate key' error.
This is obviously not helpful error message at all.
The problem lies in our lookup function
(virNWFilterBindingObjListFindByPortDevLocked()) which return
NULL even if the object is still on the list. They do this so
that the object is not mistakenly looked up by some API. The fix
consists of moving 'removing' check one level up and thus
allowing virNWFilterBindingObjListAddLocked() to produce
meaningful error message.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Cole Robinson <crobinso@redhat.com>
If there are two concurrent threads, one of which is removing a
domain from the list and the other is trying to add it back they
may serialize in the following order:
1) vm->removing is set and @vm is unlocked.
2) The tread that's trying to add the domain onto the list locks
the list and tries to find, if the domain already exists.
3) Our lookup functions say it doesn't, so the thread proceeds to
virHashAddEntry() which fails with 'Duplicate key' error.
This is obviously not helpful error message at all.
The problem lies in our lookup functions
(virDomainObjListFindByUUIDLocked() and
virDomainObjListFindByNameLocked()) which return NULL even if the
object is still on the list. They do this so that the object is
not mistakenly looked up by some driver. The fix consists of
moving 'removing' check one level up and thus allowing
virDomainObjListAddLocked() to produce meaningful error message.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Cole Robinson <crobinso@redhat.com>
Parsing of the cpu affinity list was using virParseNumber. Modernize it
to get rid of the virParseNumber call.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
https://bugzilla.redhat.com/show_bug.cgi?id=1693066
Up until memfd introduction (in 24b74d187c) we did not need to
know @pagesize because qemuGetDomainHupageMemPath() could deal
with it being zero (value of zero means use the default hugetlbfs
mount). But since for memfd we are not passing a path to
hugetlbfs mount rather the page size value we need to know its
value upfront.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
This helper returns the default hugetlbfs mount point from given
array of mount points.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Since commit 66460e3 dropped support for YAJL 1, we no longer need
these.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Acked-by: Peter Krempa <pkrempa@redhat.com>
Now that we do not need to cater to YAJL 1, move the check for the
return value of yajl_gen_alloc earlier, so that we can assume it
was successful in later code.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Now that we require YAJL2, drop the code dealing with YAJL 1.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
qemuMigrationSrcPerform callers expect it to call virDomainObjEndAPI
in any case so on error paths we miss the virDomainObjEndAPI call.
To fix this let's make qemuMigrationSrcPerform callers responsible
for the virDomainObjEndAPI call.
ACKed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
The d_type field cannot be assumed to be filled. Some filesystems, such
as older XFS, will simply report DT_UNKNOWN.
Even if the d_type is filled in, the use of it in the SELinux functions
is dubious. If labelling all files in a directory there's no reason to
skip things which are not regular files. We merely need to skip "." and
"..", which is done by virDirRead() already.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
d_type is a non-portable extension to the struct dirent and even if it
exists, its value may be DT_UNKNOWN if the filesystem doesn't support
it. This is common with older versions of XFS which have ftype=0
feature.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Use virJSONValueToBuffer so that we can append the command terminator
string without copying of the string again. Also avoid a 'strlen' as we
can query the buffer use size.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Laine Stump <laine@laine.org>
The internal qemu machinery already logs the sent message via the PROBE
point in qemuMonitorSend and the monitor receive function. Those are way
better as they are easy grepable. Remove the additional ones from the
monitor code which just duplicate the sent data.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Laine Stump <laine@laine.org>
We have tests that validate the XML formatter. Additionally almost every
guide tells users to disable JSON logging. Drop logging of output string
in virJSONValueToString.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Laine Stump <laine@laine.org>
The last step of the conversion involves copying of the generated JSON
into a separate string. We can use a virBuffer to do this as this will
also allow to subsequently use the buffer when we actually need to do
some other formatting of the string.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Laine Stump <laine@laine.org>
Use size_t for all sizes. The '*' modifier unfortunately does require an
int so a temporary variable is necessary in the tests.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Laine Stump <laine@laine.org>
This was meant to stop abusing the members directly, but we don't do
this for other internal structs. Additionally this did not stop the
test from touching the members. Remove the header obscurization.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Laine Stump <laine@laine.org>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Refactor code paths which clear strings on cleanup paths to use the
automatic helper.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
VIR_AUTODISPOSE_STR is similar to VIR_AUTOFREE(char *) but uses
virDispose for clearing of the stored string.
This patch also refactors VIR_DISPOSE to use the new helper which is
used for the new macro.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
'blockdev-snapshot-sync' is present in QEMU since v0.14.0-rc0 and
'transaction' since v1.1.0 (52e7c241ac766406f05fa)
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
qemu added the 'drive-mirror' command in v1.3.0 (d9b902db3fb71fdc)
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
qemu added the 'block-commit' command in v1.3.0 (ed61fc10e8c8d2)
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
This was detected by the presence of 'block-stream' which is present in
qemu since v1.1 (db58f9c0605fa151b8c4)
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Similarly to the disk source we need to keep the disk index (which is in
the qemu driver used for identification of the source for block jobs)
for the <mirror> element so that when it's replaced as a disk source
after pivoting all the allocated data is present.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
When the block copy operation is started with a reused external file in
incremental mode libvirt will need to open and insert the backing chain
for that file into qemu (in -blockdev mode). This means that we'll need
to track the backing chain and metadata such as node names for the full
chain of <mirror>.
This patch invokes the full backing chain formatter and parser for
<mirror> so that the chain can be kept with <mirror>.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
We have the proper flags available so we can pass them to the fomatter.
The added bonus is that private data may be formatted into the status
XML.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The helper converts the 'type', 'format' and index values to enum
values/numbers and does validation.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
virDomainDiskSourceParse was now just a thin wrapper without any extra
value. Replace all usage of it by the function it calls and remove the
function.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Modify the check that the format is in range to be standalone and use
the convertor function directly.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Now that the cleanup is handled automatically it can be removed.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
When adding <migrationSource> I've used a slightly unusual approach. To
allow using the disk source XML parser and formatter convert
<migrationSource> to look like <disk>. This means that <source> will be
added as a subelement of <migrationSource> rather than being formatted
inline.
Conversion from the old format in the parser is very simple as it
involves only moving the XPath context current node slightly if the new
format is found.
The status XML to XML test shows that the upgrade is done correctly.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
All callers including transitive callers through
virDomainDiskSourceFormatInternal always pass true. Remove the argument.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
We parse the seclabels and use them internally so omitting them when
formatting would be misleading. Additionally our schema actually allows
them.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Using copy_on_read for removable disks is a hassle. It also does not
work for CDROMs at all as the image is supposed to be read-only and we
might ignore it for floppies when they are started as empty. Forbid it
for floppies completely rather than trying to support what probably
nobody is using.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Until the block job completes we can't change the disk chain. Removal
would fail as the block job still has reference to the chain.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Unref the config pointer automatically in code paths which get a local
copy.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
qemuDomainChangeGraphicsPasswords and qemuDomainRemoveHostDevice
don't use 'cfg' any more since commits 4327df7eee and 802c59d4b9
respectively.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Failure of qemuMonitorGetVersion is fatal now that we only support QMP
based qemus. Remove the debug message since we report an error already.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
If the detected qemu version is below our required version 'package'
would be leaked.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Move the check out of virQEMUCapsInitQMPMonitor similarly to other
functions.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Move the code out of virQEMUCapsInitQMPMonitor similarly to other
functions.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Move the check out of virQEMUCapsInitQMPMonitor similarly to other
functions.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Some caps are cleared according to some more advanced logic after
detection. Split all that logic out into virQEMUCapsInitProcessCaps.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
virQEMUCapsInitQMPMonitor is massive now since it collects calls to the
various probing functions and also version based capabilities. Split
out the version based caps into a separate function.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Add hppa, nios2, or1k, riscv32 and riscv64 to the profile.
Fixes: https://bugs.debian.org/914940
Signed-off-by: intrigeri <intrigeri@boum.org>
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Check that the attribute is the same in qemuDomainDiskChangeSupported
in case somebody tries to change it using the UpdateDevice API.
https://bugzilla.redhat.com/show_bug.cgi?id=1601677
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Laine Stump <laine@laine.org>
https://bugzilla.redhat.com/show_bug.cgi?id=1601677
This reverts commit 047cfb05ee
Using numeric comparison on strings means we reject every update
that does include the group name, even if it's unchanged.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Laine Stump <laine@laine.org>
Even though Coverity can prove that 'last' is always set if the prior
loop executed, gcc 8.0.1 cannot:
CC conf/libvirt_conf_la-virdomainmomentobjlist.lo
../../src/conf/virdomainmomentobjlist.c: In function 'virDomainMomentMoveChildren':
../../src/conf/virdomainmomentobjlist.c:178:19: error: 'last' may be used uninitialized in this function [-Werror=maybe-uninitialized]
last->sibling = to->first_child;
~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~
Rewrite the loop to a form that should be easier for static analysis
to work with.
Fixes: ced0898f86
Reported-by: Bjoern Walk <bwalk@linux.ibm.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
commit edaf13565 modified the stats retrieval for OVS interfaces to
not fail when one of the fields was unrecognized by the ovs-vsctl
command, but ovs-vsctl was still returning an error, and libvirt was
cluttering the logs with these inconsequential error messages.
This patch modifies the GET_STAT macro to add "--if-exists" to the
ovs-vsctl command, which causes it to return an empty string (and exit
with success) if the requested statistic isn't in its database, thus
eliminating the ugly error messages from the log.
Resolves: https://bugzilla.redhat.com/1683175
Signed-off-by: Laine Stump <laine@laine.org>
The warning is reported at a code path which already reports a proper
error so it's pointless to add yet another line into logs.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Avoid the extra parameter passing in the disk 'dst' parameter to be
reported instead of the device alias. Using 'dst' instead of alias does
not add much value.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
qemuDomainRemoveDiskDevice calls qemuDomainReleaseDeviceAddress which
already calls virDomainUSBAddressRelease so we don't need to call it
again.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
There is one specific caller (testInfoSetArgs() in
qemuxml2argvtest.c) which expect the va_list argument to change
after returning from the virQEMUCapsSetVAList() function.
However, since we are passing plain va_list this is not
guaranteed. The man page of stdarg(3) says:
If ap is passed to a function that uses va_arg(ap,type), then
the value of ap is undefined after the return of that function.
(ap is a variable of type va_list)
I've seen this in action in fact: on i686 the qemuxml2argvtest
fails on the second test case because testInfoSetArgs() sees
ARG_QEMU_CAPS and calls virQEMUCapsSetVAList to process the
capabilities (in this case there's just one
QEMU_CAPS_SECCOMP_BLACKLIST). But since the changes are not
reflected in the caller, in the next iteration testInfoSetArgs()
sees the QEMU capability and not ARG_END.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
This reverts commit 6b90a84738.
It turns out gcc -O2 is not happy with it, complaining:
/home/pipo/libvirt/src/qemu/qemu_driver.c: In function 'qemuDomainSnapshotCreateXML':
/home/pipo/libvirt/src/qemu/qemu_driver.c:15389:26: error: potential null pointer dereference [-Werror=null-dereference]
bool memory = snapdef->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL;
~~~~~~~^~~~~~~~
/home/pipo/libvirt/src/qemu/qemu_driver.c:15389:26: error: potential null pointer dereference [-Werror=null-dereference]
In file included from /home/pipo/libvirt/src/util/virbuffer.h:27,
from /home/pipo/libvirt/src/conf/capabilities.h:27,
from /home/pipo/libvirt/src/conf/domain_conf.h:32,
from /home/pipo/libvirt/src/qemu/qemu_agent.h:26,
from /home/pipo/libvirt/src/qemu/qemu_driver.c:40:
/home/pipo/libvirt/src/util/viralloc.h:125:34: error: potential null pointer dereference [-Werror=null-dereference]
# define VIR_ALLOC_N(ptr, count) virAllocN(&(ptr), sizeof(*(ptr)), (count), true, \
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
VIR_FROM_THIS, __FILE__, __FUNCTION__, __LINE__)
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/pipo/libvirt/src/qemu/qemu_driver.c:15103:9: note: in expansion of macro 'VIR_ALLOC_N'
if (VIR_ALLOC_N(ret, snapdef->ndisks) < 0)
^~~~~~~~~~~
/home/pipo/libvirt/src/qemu/qemu_driver.c:15798:45: error: null pointer dereference [-Werror=null-dereference]
virDomainSnapshotObjGetDef(snap)->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) {
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~
As the patch simplified one or two callers at the risk of making
many other callers now candidates to trigger aggressive compiler
warnings, it isn't worth it.
Signed-off-by: Eric Blake <eblake@redhat.com>
Use the common base class virDomainMoment for iterator callbacks
related to snapshots from the qemu code, so that when checkpoint
operations are introduced, they can share the same callbacks.
Simplify the code for qemuDomainSnapshotCurrent by better utilizing
virDomainMoment helpers.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
The qemu driver already had a full-blown virDomainMomentObjPtr to
check against, and the test driver ought to have one since we get
better error checking that the user passed in a valid object. Removes
the need for a helper function added in commit commit 4819f54b.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
The VIR_MIGRATE_PARALLEL flag is implemented using QEMU's multifd
migration capability and the corresponding multifd-channels migration
parameter.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Prepare for introducing a bunch of new public APIs related to
backup checkpoints by first introducing a new internal type
and errors associated with that type. Checkpoints are modeled
heavily after virDomainSnapshotPtr (both represent a point in
time of the guest), although a snapshot exists with the intent
of rolling back to that state, while a checkpoint exists to
make it possible to create an incremental backup at a later
time.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Since I was copying this text to form checkpoint XML and API
documentation, I might as well make improvements along the way. Most
of these changes are based on reviews of the checkpoint docs.
Among other things: grammar tweaks, point to a single source of
documentation rather than repeating verbosity, reword things for
easier legibility.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
This reverts commit 86c0ed6f70, and
subsequent refactorings of the function into new files. There are no
callers of this function - I had originally proposed it for
implementing a new bulk snapshot API, but that proved to be too
invasive given RPC limits. I also tried using it for streamlining how
the qemu driver stores snapshot state across libvirtd restarts
internally, but in the end, the risks of a new internal format
outweighed the benefits of one file per snapshot.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
This reverts commit 1b57269cbc, and
subsequent refactorings of the function into new files. There are no
callers of this function - I had originally proposed it for
implementing a new bulk snapshot API, but that proved to be too
invasive given RPC limits. I also tried using it for streamlining how
the qemu driver stores snapshot state across libvirtd restarts
internally, but in the end, the risks of a new internal format
outweighed the benefits of one file per snapshot.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
For [some unknown reason, possibly/probably pure chance], Net devices
have been taken offline and their bandwidth tc rules cleared as the
very first operation when detaching the device. This is contrary to
every other type of device, where all hostside teardown is delayed
until we receive the DEVICE_DELETED event back from qemu, indicating
that the guest has finished with the device.
This patch delays these two operations until receipt of
DEVICE_DELETED, which removes an ugly wart from
qemuDomainDetachDeviceLive(), and also seems to be a more correct
sequence of events.
Signed-off-by: Laine Stump <laine@laine.org>
ACKed-by: Peter Krempa <pkrempa@redhat.com>
The VIR_DOMAIN_EVENT_ID_DEVICE_REMOVED event is sent after qemu has
responded to a device_del command with a DEVICE_DELETED event. Before
queuing the event, *some* of the final teardown of the device's
trappings in libvirt is done, but not *all* of it. As a result, an
application may receive and process the DEVICE_REMOVED event before
libvirt has really finished with it.
Usually this doesn't cause a problem, but it can - in the case of the
bug report referenced below, vdsm is assigning a PCI device to a guest
with managed='no', using livirt's virNodeDeviceDetachFlags() and
virNodeDeviceReAttach() APIs. Immediately after receiving a
DEVICE_REMOVED event from libvirt signalling that the device had been
successfully unplugged, vdsm would cal virNodeDeviceReAttach() to
unbind the device from vfio-pci and rebind it to the host driverm but
because the event was received before libvirt had completely finished
processing the removal, that device was still on the "activeDevs"
list, and so virNodeDeviceReAttach() failed.
Experimentation with additional debug logs proved that libvirt would
always end up dispatching the DEVICE_REMOVED event before it had
removed the device from activeDevs (with a *much* greater difference
with managed='yes', since in that case the re-binding of the device
occurred after queuing the device).
Although the case of hostdev devices is the most extreme (since there
is so much involved in tearing down the device), *all* device types
suffer from the same problem - the DEVICE_REMOVED event is queued very
early in the qemuDomainRemove*Device() function for all of them,
resulting in a possibility of any application receiving the event
before libvirt has really finished with the device.
The solution is to save the device's alias (which is the only piece of
info from the device object that is needed for the event) at the
beginning of processing the device removal, and then queue the event
as a final act before returning. Since all of the
qemuDomainRemove*Device() functions (except
qemuDomainRemoveChrDevice()) are now called exclusively from
qemuDomainRemoveDevice() (which selects which of the subordinates to
call in a switch statement based on the type of device), the shortest
route to a solution is to doing the saving of alias, and later
queueing of the event, in the higher level qemuDomainRemoveDevice(),
and just completely remove the event-related code from all the
subordinate functions.
The single exception to this, as mentioned before, is
qemuDomainRemoveChrDevice(), which is still called from somewhere
other than qemuDomainRemoveDevice() (and has a separate arg used to
trigger different behavior when the chr device has targetType ==
GUESTFWD), so it must keep its original behavior intact, and must be
treated differently by qemuDomainRemoveDevice() (similar to the way
that qemuDomainDetachDeviceLive() treats chr and lease devices
differently from all the others).
Resolves: https://bugzilla.redhat.com/1658198
Signed-off-by: Laine Stump <laine@laine.org>
ACKed-by: Peter Krempa <pkrempa@redhat.com>
Now that all the qemuDomainDetachPrep*() functions look nearly
identical at the end, we can put one copy of that identical code in
qemuDomainDetachDeviceLive() at the point after the individual prep
functions have been called, and remove the duplicated code from all
the prep functions. The code to locate the target "detach" device
based on the "match" device remains, as do all device-type-specific
validations.
Unfortunately there are a few things going on at once in this patch,
which makes it a bit more difficult to follow than the others; it was
just impossible to do the changes in stages and still have a
buildable/testable tree at each step.
The other changes of note:
* The individual prep functions no longer need their driver or async
args, so those are removed, as are the local "ret" variables, since
in all cases the functions just directly return -1 or 0.
* Some of the prep functions were checking for a valid alias and/or
for attempts to detach a multifunction PCI device, but not all. In
fact, both checks are valid (or at least harmless) for *all* device
types, so they are removed from the prep functions, and done a
single time in the common function.
(any attempts to *create* an alias when there isn't one has been
removed, since that is doomed to failure anyway; the only way the
device wouldn't have an alias is if 1) the domain was created by
calling virsh qemu-attach to attach an existing qemu process to
libvirt, and 2) the qemu command that started said process used "old
style" arguments for creating devices that didn't have any device
ids. Even if we constructed a device id for one of these devices,
qemu wouldn't recognize it in the device_del command anyway, so we
may as well fail earlier with "device missing alias" rather than
failing later with "couldn't delete device net0".)
* Only one type of device has shutdown code that must not be called
until after *all* validation of the device is done (including
checking for multifunction PCI and valid alias, which is done in the
toplevel common code). For this reason, the Net function has been
split in two, with the 2nd half (qemuDomainDetachShutdownNet())
called from the common function, right before sending the delete
command to qemu.
Signed-off-by: Laine Stump <laine@laine.org>
ACKed-by: Peter Krempa <pkrempa@redhat.com>
Although all hotpluggable devices other than lease, controller,
watchdof, and vsock can be audited, and *are* audited when an unplug
is successful, only disk, net, and hostdev were actually being audited
on failure.
This patch corrects that omission.
Signed-off-by: Laine Stump <laine@laine.org>
ACKed-by: Peter Krempa <pkrempa@redhat.com>
This function can be called with a virDomainDevicePtr and whether or
not the removal was successful, and it will call the appropriate
virDomainAudit*() function with the appropriate args for whatever type
of device it's given (or do nothing, if that's appropriate). This
permits generalizing some code that currently has a separate copy for
each type of device.
NB: Although the function initially will be called only with
success=false, that has been made an argument so that in the future
(when the qemuDomainRemove*Device() functions have had their common
functionality consolidated into qemuDomainRemoveDevice()), this new
common code can call qemuDomainRemoveAuditDevice() for all types.
Signed-off-by: Laine Stump <laine@laine.org>
ACKed-by: Peter Krempa <pkrempa@redhat.com>
qemuDomainDetachDeviceChr and qemuDomainDetachDeviceLease are more
consistent with each other.
Signed-off-by: Laine Stump <laine@laine.org>
ACKed-by: Peter Krempa <pkrempa@redhat.com>
Most of these functions will soon contain only some setup for
detaching the device, not the detach code proper (since that code is
identical for these devices). Their device specific functions are all
being renamed to qemuDomainDetachPrep*(), where * is the
name of that device's data member in the virDomainDeviceDef
object.
Since there will be other code in qemuDomainDetachDeviceLive() after
the calls to qemuDomainDetachPrep*() that could still fail, we no
longer directly set "ret" with the return code from
qemuDomainDetachPrep*() functions, but simply return -1 on
failure, and wait until the end of qemuDomainDetachDeviceLive() to set
ret = 0.
Along with the rename, qemuDomainDetachPrep*() functions are also
given similar arglists, including an arg called "match" that points to
the proto-object of the device we want to delete, and another arg
"detach" that is used to return a pointer to the actual object that
will be (for now *has been*) detached. To make sure these new args
aren't confused with existing local pointers that sometimes had the
same name (detach), the local pointer to the device is now named after
the device type ("controller", "disk", etc). These point to the same
place as (*detach)->data.blah, it's just easier on the eyes to have,
e.g., "disk->dst" rather than "(*detach)->data.disk-dst".
Signed-off-by: Laine Stump <laine@laine.org>
ACKed-by: Peter Krempa <pkrempa@redhat.com>
The Chr and Lease devices have detach code that is too different from
the other device types to handle with common functionality (which will
soon be added at the end of qemuDomainDetachDeviceLive(). In order to
make this difference obvious, move the cases for those two device
types to the top of the switch statement in
qemuDomainDetachDeviceLive(), have the cases return immediately so the
future common code at the end of the function will be skipped, and
also include some hopefully helpful comments to remind future
maintainers why these two device types are treated differently.
Any attempt to detach an unsupported device type should also skip the
future common code at the end of the function, so the case for
unsupported types is similarly changed from a simple break to a return
-1.
Signed-off-by: Laine Stump <laine@laine.org>
ACKed-by: Peter Krempa <pkrempa@redhat.com>
I'm about to add a second virDomainDeviceDef to this function that
will point to the actual device in the domain object. while this is
just a partially filled-in example of what to look for. Naming it
match will make the code easier to follow.
Signed-off-by: Laine Stump <laine@laine.org>
ACKed-by: Peter Krempa <pkrempa@redhat.com>
These are no longer called from qemu_driver.c, since the function that
called them (qemuDomainDetachDeviceLive()) has been moved to
qemu_hotplug.c, and they are no longer called from testqemuhotplug.c
because it now just called qemuDomainDetachDeviceLive() instead of all
the subordinate functions.
Signed-off-by: Laine Stump <laine@laine.org>
ACKed-by: Peter Krempa <pkrempa@redhat.com>
qemuDomainDetachDeviceLive() is called from two places in
qemu_driver.c, and qemuDomainUpdateDeviceList() is called from the
end of qemuDomainDetachDeviceLive(), which is now in qemu_hotplug.c
This patch replaces the single call to qemuDomainUpdateDeviceList()
with two calls to it immediately after return from
qemuDomainDetachDeviceLive(). This is only done if the return from
that function is exactly 0, in order to exactly preserve previous
behavior.
Removing that one call from qemuDomainDetachDeviceList() will permit
us to call it from the test driver hotplug test, replacing the
separate calls to qemuDomainDetachDeviceDiskLive(),
qemuDomainDetachChrDevice(), qemuDomainDetachShmemDevice() and
qemuDomainDetachWatchdog(). We want to do this so that part of the
common functionality of those three functions (and the rest of the
device-specific Detach functions) can be pulled up into
qemuDomainDetachDeviceLive() without breaking the test. (This is done
in the next patch).
NB: Almost certainly this is "not the best place" to call
qemuDomainUpdateDeviceList() (actually, it is provably the *wrong*
place), since it's purpose is to retrieve an "up to date" list of
aliases for all devices from qemu, and if the guest OS hasn't yet
processed the detach request, the now-being-removed device may still
be on that list. It would arguably be better to instead call
qemuDomainUpdateDevicesList() later during the response to the
DEVICE_DELETED event for the device. But removing the call from the
current point in the detach could have some unforeseen ill effect due
to changed timing, so the change to move it into
qemuDomainRemove*Device() will be done in a separate patch (in order
to make it easily revertible in case it causes a regression).
Signed-off-by: Laine Stump <laine@laine.org>
ACKed-by: Peter Krempa <pkrempa@redhat.com>
qemuDomainDetachDeviceControllerLive() just checks if the controller
type is SCSI, and then either returns failure, or calls
qemuDomainDetachControllerDevice().
Instead, lets just check for type != SCSI at the top of the latter
function, and call it directly.
Signed-off-by: Laine Stump <laine@laine.org>
ACKed-by: Peter Krempa <pkrempa@redhat.com>
This function is going to take on some of the functionality of its
subordinate functions, which all live in qemu_hotplug.c.
qemuDomainDetachDeviceControllerLive() is only called from
qemuDomainDetachDeviceLive() (and will soon be merged into
qemuDomainDetachControllerDevice(), which is in qemu_hotplug.c), so
it is also moved.
Signed-off-by: Laine Stump <laine@laine.org>
ACKed-by: Peter Krempa <pkrempa@redhat.com>
They were added in qemu commit 7572150c189c6553c2448334116ab717680de66d
released in v0.14.0.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Rather than hard-coding the snapshot filter bit values into the
generic code, add another layer of indirection: callers must map which
of their public filter bits correspond to supported moment bits, then
pass two separate flags (the ones translated for moment code to
operate on, and the remaining ones for the filter callback to operate
on).
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
The Attach and Detach Lease functions were together in the middle of
the Detach functions. Put them at the end of their respective
sections, since they behave differently from the other attach/detach
functions (DetachLease doesn't use qemuDomainDeleteDevice(), and is
always synchronous).
Signed-off-by: Laine Stump <laine@laine.org>
ACKed-by: Peter Krempa <pkrempa@redhat.com>
There were two outliers at the end of the file beyond the Vcpu
functions.
Signed-off-by: Laine Stump <laine@laine.org>
ACKed-by: Peter Krempa <pkrempa@redhat.com>
It was sitting down in the middle of all the qemuDomainDetach*()
functions. Move it up with the rest of the qemuDomain*Graphics*()
functions.
Signed-off-by: Laine Stump <laine@laine.org>
ACKed-by: Peter Krempa <pkrempa@redhat.com>
It's now only called from one place, and combining the two functions
highlights the similarity with Detach functions for other device
types.
Signed-off-by: Laine Stump <laine@laine.org>
ACKed-by: Peter Krempa <pkrempa@redhat.com>
Back in the bad old days different device types required a different
qemu monitor call to detach them, and so an <interface type='hostdev'>
needed to call the function for detaching hostdevs, while other
<interface> types could be deleted as netdevs.
Times have changed, and *all* device types are detached by calling the
common function qemuDomainDeleteDevice(vm, alias), so we don't need to
differentiate between hostdev interfaces and the others for that
reason.
There are a few other netdev-specific functions called during
qemuDomainDetachNetDevice() (clearing bandwidth limits, stopping the
interface), but those turn into NOPs when type=hostdev, so they're
safe to call for type=hostdev.
The only thing that is different + not a NOP is the call to
virDomainAudit*() when qemuDomainDeleteDevice() fails, so if we add a
conditional for that small bit of code, we can eliminate the callout
from qemuDomainDetachNetDevice() to qemuDomainDetachThisDevice(),
which makes this function fit the desired pattern for merging with the
other detach functions, and paves the way to simplifying
qemuDomainDetachHostDevice() too.
Signed-off-by: Laine Stump <laine@laine.org>
ACKed-by: Peter Krempa <pkrempa@redhat.com>
qemuDomainDetachDiskDevice() is only called from one place. Moving the
contents of the function to that place makes
qemuDomainDetachDiskLive() more similar to the other Detach functions
called by the toplevel qemuDomainDetachDevice().
The goal is to make each of the device-type-specific functions do this:
1) find the exact device
2) do any device-specific validation
3) do general validation
4) do device-specific shutdown (only needed for net devices)
5) do the common block of code to send device_del to qemu, then
optionally wait for a corresponding DEVICE_DELETED event from
qemu.
with the final aim being that only items 1 & 2 will remain in each
device-type-specific function, while 3 & 5 (which are the same for
almost every type) will be de-duplicated and moved to the toplevel
function that calls all of these (qemuDomainDetachDeviceLive(), which
will also contain a callout to the one instance of (4) (netdev).
Signed-off-by: Laine Stump <laine@laine.org>
ACKed-by: Peter Krempa <pkrempa@redhat.com>
qemuDomainDetachHostDevice() has a check at the end that calls
qemuDomainDetachNetDevice() in the case that the hostdev is actually a
Net device of type='hostdev'. A long time ago when device removal was
(supposedly but not actually) synchronous, this would cause some extra
code to be run prior to removing the device (e.g. restoring the original MAC
address of the device, undoing some sort of virtual port profile, etc).
For quite awhile now the device removal has been asynchronous, so that
"extra teardown" isn't handled by the detach function, but instead is
handled by the Remove function called at a later time. The result is
that when we call qemuDomainDetachNetDevice() from
qemuDomainDetachHostDevice(), it ends up just calling
qemuDomainDetachThisHostDevice() and returning, which is exactly what
we do for all other hostdevs anyway.
Based on that, remove the behavioral difference when parent.type ==
VIR_DOMAIN_DEVICE_NET, and just call qemuDomainDetachThisHostDevice()
for all hostdevs.
Signed-off-by: Laine Stump <laine@laine.org>
ACKed-by: Peter Krempa <pkrempa@redhat.com>
There are separate Detach functions for PCI, USB, SCSI, Vhost, and
Mediated hostdevs, but the functions are all 100% the same code,
except that the PCI function checks for the guest side of the device
being a PCI Multifunction device, while the other 4 check that the
device's alias != NULL.
The check for multifunction PCI devices should be done for *all*
devices that are connected to the PCI bus in the guest, not just PCI
hostdevs, and qemuIsMultiFunctionDevice() conveniently returns false
if the queried device doesn't connect with PCI, so it is safe to make
this check for all hostdev devices. (It also needs to be done for many
other device types, but that will be addressed in a future patch).
Likewise, since all hostdevs are detached by calling
qemuDomainDeleteDevice(), which requires the device's alias, checking
for a valid alias is a reasonable thing for PCI hostdevs too.
Signed-off-by: Laine Stump <laine@laine.org>
ACKed-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Having an InfoPtr named "dev" made my brain hurt. Renaming it to
"info" gives one less thing to confuse when looking at the code.
Signed-off-by: Laine Stump <laine@laine.org>
ACKed-by: Peter Krempa <pkrempa@redhat.com>
When support for hotplug/unplug of SCSI controllers was added way back
in December 2009 (commit da9d937b), unplug was handled by calling the
now-extinct function qemuMonitorRemovePCIDevice(), which required a
PCI address as an argument. At the same time, the idea of every device
in the config having a PCI address apparently was not yet fully
implemented, because the author of the patch including a check for a
valid PCI address in the device object.
These days, all PCI devices are guaranteed to have a valid PCI
address. But more important than that, we no longer detach devices by
PCI address, but instead use qemuDomainDeleteDevice(), which
identifies the device by its alias. So checking for a valid PCI
address is just pointless extra code that obscures the high level of
similarity between all the individual qemuDomainDetach*Device()
functions.
Signed-off-by: Laine Stump <laine@laine.org>
ACKed-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
qemuDomainRemoveRNGDevice() calls qemuDomainDetachExtensionDevice().
According to commit 1d1e264f1 that added this code, it should not be
necessary to explicitly remove the zPCI extension device for a PCI
device during unplug, because "QEMU implements an unplug callback
which will unplug both PCI and zPCI device in a cascaded way". In
fact, no other devices call qemuDomainDetachExtensionDevice() during
their qemuDomainRemove*Device() function, so it should be removed from
qemuDomainRemoveRNGDevice as well.
Signed-off-by: Laine Stump <laine@laine.org>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
qemuDomainDetachControllerDevice() calls
qemuDomainDetachExtensionDevice() when the controller type is
PCI. This is incorrect in multiple ways:
* Any code that tears down a device should be in the
qemuDomainRemove*Device() function (which is called after libvirt
gets a DEVICE_DELETED event from qemu indicating that the guest is
finished with the device on its end. The qemuDomainDetach*Device()
functions should only contain code that ensures the requested
operation is valid, and sends the command to qemu to initiate the
unplug.
* qemuDomainDetachExtensionDevice() is a function that applies to
devices that plug into a PCI slot, *not* necessarily PCI controllers
(which is what's being checked in the offending code). The proper
way to check for this would be to see if the DeviceInfo for the
controller device had a PCI address, not to check if the controller
is a PCI controller (the code being removed was doing the latter).
* According to commit 1d1e264f1 that added this code (and other
support for hotplugging zPCI devices on s390), it's not necessary to
explicitly detach the zPCI device when unplugging a PCI device. To
quote:
There's no need to implement hot unplug for zPCI as QEMU
implements an unplug callback which will unplug both PCI and
zPCI device in a cascaded way.
and the evidence bears this out - all the other uses of
qemuDomainDetachExtensionDevice() (except one, which I believe is
also in error, and is being removed in a separate patch) are only to
remove the zPCI extension device in cases where it was successfully
added, but there was some other failure later in the hotplug process
(so there was no regular PCI device to remove and trigger removal of
the zPCI extension device).
* PCI controllers are not hot pluggable, so this is dead code
anyway. (The only controllers that can currently be
hotplugged/unplugged are SCSI controllers).
Signed-off-by: Laine Stump <laine@laine.org>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
Commit 55c2ab3e accidentally introduced an infinite loop while
checking whether a redefined snapshot would cause an infinite loop in
chasing its parents back to a root. Alas, 'make check' did not catch
it, so my next patch will be a testsuite improvement that would have
hung and prevented the bug from being checked in to begin with.
Signed-off-by: Eric Blake <eblake@redhat.com>
Acked-by: Michal Privoznik <mprivozn@redhat.com>
In a case where we want to hotplug the following disk:
<disk type='file' device='disk'>
(...)
<address type='drive' controller='0' bus='0' target='0' unit='0'/>
</disk>
In a QEMU guest that has a single OS disk, as follows:
<disk type='file' device='disk'>
(...)
<address type='drive' controller='0' bus='0' target='0' unit='0'/>
</disk>
What happens is that the existing guest disk will receive the ID
'scsi0-0-0-0' due to how Libvirt calculate the alias based on
the address in qemu_alias.c, qemuAssignDeviceDiskAlias. When hotplugging
a disk that happens to have the same address, Libvirt will calculate
the same ID to it and attempt to device_add. QEMU will refuse it:
$ virsh attach-device ub1810 hp-disk-dup.xml
error: Failed to attach device from hp-disk-dup.xml
error: internal error: unable to execute QEMU command 'device_add': Duplicate ID 'scsi0-0-0-0' for device
And Libvirt follows it up with a cleanup code in qemuDomainAttachDiskGeneric
that ends up removing what supposedly is a faulty hotplugged disk but, in
this case, ends up being the original guest disk.
This patch adds an address verification for all attached devices, avoid
calling the driver attach() function using a device with duplicated address.
The change is done in virDomainDefCompatibleDevice when @action is equal
to VIR_DOMAIN_DEVICE_ACTION_ATTACH. The affected callers are:
- qemuDomainAttachDeviceLiveAndConfig, both LIVE and CONFIG cases;
- lxcDomainAttachDeviceFlags, both LIVE and CONFIG.
The check is done using the virDomainDefHasDeviceAddress, a generic
function that can check address duplicates for all supported device
types, not limiting just to DeviceDisk type.
After this patch, this is the result of the previous attach-device call:
$ ./run tools/virsh attach-device ub1810 hp-disk-dup.xml
error: Failed to attach device from hp-disk-dup.xml
error: Requested operation is not valid: Domain already contains a device with the same address
Reported-by: Srikanth Aithal <bssrikanth@in.ibm.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
This functions tries to add a domain moment (love the name!) onto
a list of domain moments. Firstly, it checks if another moment
with the same name already exists. Then, it creates an empty
moment (without initializing its definition) and tries to add the
moment onto the list dereferencing moment definition in that
process. If it succeeds (which it never can), only after that it
sets moment->def.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Commit 3bd4ed46 introduced this element as required which
breaks backcompat for test driver. Let's make the element optional.
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
Commit d1a7c08eb changed filter instantiation code to ignore MAC and IP
variables explicitly specified for filter binding. It just replaces
explicit values with values associated with the binding. Before the
commit virNWFilterCreateVarsFrom was used so that explicit value
take precedence. Let's bring old behavior back.
This is useful. For example if domain has two interfaces it makes
sense to list both mac adresses in MAC var of every interface
filterref. So that if guest make a bond of these interfaces
and start sending frames with one of the mac adresses from
both interfaces we can pass outgress traffic from both
interfaces too.
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
Now that the generic moment code does pretty much everything that both
snapshots and checkpoints will need, it's time to replace the
now-duplicate code in virdomainsnapshotobjlist.c with simpler calls
into the generic code. I considered using sub-classing (a
'virDomainMomentObjList parent;' member, but that requires making the
opaque type visible in headers; so for now, I stuck with a container
instead (a 'virDomainMomentObjListPtr base;' member).
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
The new code here very heavily resembles the code in
virDomainSnapshotObjList. There are still a couple of spots that are
tied a little too heavily to snapshots (the free function lacks a
polymorphic cleanup until we refactor code to use virObject; and an
upcoming patch will add internal VIR_DOMAIN_MOMENT_LIST flags to
replace the snapshot flag bits), but in general this is fairly close
to the state needed to add checkpoint lists.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Now that we have made virDomainMomentObj sufficiently generic to
support both snapshots and checkpoints, it is time to rename the file
that it lives in. The split between a generic object and a list of the
generic objects doesn't buy us as much, so it will be easier to stick
all the moment list code in one file, with more code moving in the
next patch. The changes during the move are fairly minor, although it
is worth pointing out that the log/error messages for the new file
report that they are from "domain", since the file will eventually be
shared by both "domain snapshot" and "domain checkpoint".
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Now that the core of SnapshotObj is agnostic to snapshots and can be
shared with upcoming checkpoint code, it is time to rename the struct
and the functions specific to list operations. A later patch will
shuffle which file holds the common code. This is a fairly mechanical
patch.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Another step towards making the object list reusable for both
snapshots and checkpoints: the list code only ever needs items that
are in the common virDomainMomentDef base type. This undoes a lot of
the churn in accessing common members added in the previous patch, and
the bulk of the patch is mechanical. But there was one spot where I
had to unroll a VIR_STEAL_PTR to work around changed types.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Pull out the common parts of virDomainSnapshotDef that will be reused
for virDomainCheckpointDef into a new base class. Adjust all callers
that use the direct fields (some of it is churn that disappears when
the next patch refactors virDomainSnapshotObj; oh well...).
Someday, I hope to switch this type to be a subclass of virObject, but
that requires a more thorough audit of cleanup paths, and besides
minimal incremental changes are easier to review.
As for the choice of naming:
I promised my teenage daughter Evelyn that I'd give her credit for her
contribution to this commit. I asked her "What would be a good name
for a base class for DomainSnapshot and DomainCheckpoint". After
explaining what a base class was (using the classic OOB Square and
Circle inherit from Shape), she came up with "DomainMoment", which is
way better than my initial thought of "DomainPointInTime" or
"DomainPIT".
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Separate the algorithm for which list members to vist (which is
generic and can be shared with checkpoints, provided that common
filtering bits are either declared with the same value or have a
mapping from public API to common value) from the decision on which
members to return (which is specific to snapshots). The typedef for
the callback function feels a bit heavy here, but will make it easier
to move the common portions in a later patch.
As part of the refactoring, note that the macros for selecting filter
bits are specific to listing functionality, so they belong better in
virdomainsnapshotobjlist.h (missed in commit 9b75154c).
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
An upcoming patch will rework virDomainSnapshotObjList to be generic
for both snapshots and checkpoints; reduce the churn by adding a new
accessor virDomainSnapshotObjGetDef() which returns the
snapshot-specific definition even when the list is rewritten to
operate only on a base class, then using it at sites that that are
specific to snapshots. Use VIR_STEAL_PTR when appropriate in the
affected lines.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Rather than allowing a leaky abstraction where multiple drivers have
to open-code operations that update the relations in a
virDomainSnapshotObjList, it is better to add accessor functions so
that updates to relations are maintained closer to the internals.
This patch finishes the job started in the previous patch, by getting
rid of all direct access to nchildren, first_child, or sibling outside
of the lowest level functions, making it easier to refactor later on.
The lone new caller to virDomainSnapshotObjListSize() checks for a
return != 0, because it wants to handles errors (-1, only possible if
the hash table wasn't allocated) and existing snapshots (> 0) in the
same manner; we can drop the check for a current snapshot on the
grounds that there shouldn't be one if there are no snapshots.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Rather than allowing a leaky abstraction where multiple drivers have
to open-code operations that update the relations in a
virDomainSnapshotObjList, it is better to add accessor functions so
that updates to relations are maintained closer to the internals.
This patch starts the task with a single new function:
virDomainSnapshotMoveChildren(). The logic might not be immediately
obvious [okay, that's an understatement - the existing code uses black
magic ;-)], so here's an overview: The old code has an implicit for
loop around each call to qemuDomainSnapshotReparentChildren() by using
virDomainSnapshotForEachChild() (you'll need a wider context than
git's default of 3 lines to see that); the new code has a more visible
for loop. Then it helps if you realize that the code is making two
separate changes to each child object: STRDUP of the new parent name
prior to writing XML files (unchanged), and touching up the pointer to
the parent object (refactored); the end result is the same whether a
single pass made both changes (both in driver code), or whether it is
split into two passes making one change each (one in driver code, the
other in the new accessor).
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
It is easier to track the current snapshot as part of the list of
snapshots. In particular, doing so lets us guarantee that the current
snapshot is cleared if that snapshot is removed from the list (rather
than depending on the caller to do so, and risking a use-after-free
problem, such as the one recently patched in 1db9d0efbf). This
requires the addition of several new accessor functions, as well as a
useful return type for virDomainSnapshotObjListRemove(). A few error
handling sites that were previously setting vm->current_snapshot =
NULL can now be dropped, because the previous function call has now
done it already. Also, qemuDomainRevertToSnapshot() was setting the
current vm twice, so keep only the one used on the success path.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Rework the logic in qemuDomainSnapshotLoad() to set
vm->current_snapshot only once at the end of the loop, rather than
repeatedly querying it during the loop, to make it easier for the next
patch to use accessor functions rather than direct manipulation of
vm->current_snapshot. When encountering multiple snapshots claiming
to be current (based on the presence of an <active>1</active> element
in the XML, which libvirt only outputs for internal use and not for
any public API), this changes behavior from warning only once and
running with no current snapshot, to instead warning on each duplicate
and selecting the last one encountered (which is arbitrary based on
readdir() ordering, but actually stands a fair chance of being the
most-recently created snapshot whether by timestamp or by the
propensity of humans to name things in ascending order).
Note that the code in question is only run by libvirtd when it first
starts, reading state from disk from the previous run into memory for
this run. Since the data resides somewhere that only libvirt should be
touching (typically /var/lib/libvirt/qemu/snapshot/*), it should be
clean. So in the common case, the code touched here is unreachable.
But if someone is actually messing with files behind libvirt's back,
they deserve the change in behavior.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
The only use for the 'current' member of virDomainSnapshotDef was with
the PARSE/FORMAT_INTERNAL flag for controlling an internal-use
<active> element marking whether a particular snapshot definition was
current, and even then, only by the qemu driver on output, and by qemu
and test driver on input. But this duplicates vm->snapshot_current,
and gets in the way of potential simplifications to have qemu store a
single file for all snapshots rather than one file per snapshot. Get
rid of the member by adding a bool* parameter during parse (ignored if
the PARSE_INTERNAL flag is not set), and by adding a new flag during
format (if FORMAT_INTERNAL is set, the value printed in <active>
depends on the new FORMAT_CURRENT).
Then update the qemu driver accordingly, which involves hoisting
assignments to vm->current_snapshot to occur prior to any point where
a snapshot XML file is written (although qemu kept
vm->current_snapshot and snapshot->def_current in sync by the end of
the function, they were not always identical in the middle of
functions, so the shuffling gets a bit interesting). Later patches
will clean up some of that confusing churn to vm->current_snapshot.
Note: even if later patches refactor qemu to no longer use
FORMAT_INTERNAL for output (by storing bulk snapshot XML instead), we
will always need PARSE_INTERNAL for input (because on upgrade, a new
libvirt still has to parse XML left from a previous libvirt).
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
When a future patch converts virDomainSnapshotDef to be a virObject,
we need to be careful that converting VIR_FREE() to virObjectUnref()
does not result in double frees. Reorder the assignment of def into
the object to the point after object is in the hash table (as
otherwise the virHashAddEntry() error path would have a shot at
freeing def prematurely).
Suggested-by: John Ferlan <ferlan@redhat.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
Change the return value of virDomainSnapshotObjListParse() to return
the number of snapshots imported, and allow a return of 0 (the
original proposal of adding a flag to virDomainSnapshotCreateXML
required returning an arbitrary non-NULL snapshot, but that idea was
abandoned; and by returning a count, we are no longer constrained to a
non-empty list).
Document which flags are supported (namely, just SECURE) in
virDomainSnapshotObjListFormat().
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
An upcoming patch will be reworking virDomainSnapshotDef to have a
base class; minimize the churn by using a local variable to reduce the
number of dereferences required when acessing the domain definition
associated with the snapshot.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
And adjust virQEMUCapsSetList to use it. It will also be used in future
patches.
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
The following virsh command was triggering a use-after-free:
$ virsh -c test:///default '
snapshot-create-as test s1
snapshot-create-as test s2
snapshot-delete --children-only test s1
snapshot-current --name test'
Domain snapshot s1 created
Domain snapshot s2 created
Domain snapshot s1 children deleted
error: name in virGetDomainSnapshot must not be NULL
I got lucky on that run - although the error message is quite
unexpected. On other runs, I was able to get a core dump, and
valgrind confirms there is a definitive problem.
The culprit? We were inconsistent about whether we set
vm->current_snapshot, snap->def->current, or both when updating how
the current snapshot was being tracked. As a result, deletion did not
see that snapshot s2 was previously current, and failed to update
vm->current_snapshot, so that the next API using the current snapshot
failed because it referenced stale memory for the now-gone s2 (instead
of the intended s1).
The test driver code was copied from the qemu code (which DOES track
both pieces of state everywhere), but was purposefully simplified
because the test driver does not have to write persistent snapshot
state to the file system. But when you realize that the only reason
snap->def->current needs to exist is when writing out one file per
snapshot for qemu, it's just as easy to state that the test driver
never has to mess with the field (rather than chasing down which
places forgot to set the field), and have vm->current_snapshot be the
sole source of truth in the test driver.
Ideally, I'd get rid of the 'current' member in virDomainSnapshotDef,
as well as the 'current_snapshot' member in virDomainDef, and instead
track the current member in virDomainSnapshotObjList, coupled with
writing ALL snapshot state for qemu in a single file (where I can use
<snapshots current='...'> as a wrapper, rather than
VIR_DOMAIN_SNAPSHOT_FORMAT_INTERNAL to output <current>1</current> XML
on a per-snapshot file basis). But that's a bigger change, so for now
I'm just patching things to avoid the test driver segfault.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
https://bugzilla.redhat.com/show_bug.cgi?id=1686927
When trying to create a nwfilter binding via
nwfilterBindingCreateXML() we may encounter a crash. The sequence
of functions called is as follows:
1) nwfilterBindingCreateXML() parses the XML and calls
virNWFilterBindingObjListAdd() which calls
virNWFilterBindingObjListAddLocked()
2) Here, @binding is not found because binding->remove is set.
3) Therefore, controls continue with creating new @binding,
setting its def to the one from 1) and adding it to the hash
table.
4) This fails, because the binding is still in the hash table
(duplicate key is detected).
5) The control jumps to 'error' label where
virNWFilterBindingObjEndAPI() is called which frees the binding
definition passed.
6) Error is propagated to the caller, which calls
virNWFilterBindingDefFree() over the definition again.
The solution is to unset binding->def in case of failure so it's
not freed in step 5).
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Storage source private data can be parsed along with other components of
private data rather than a separate function which is called from
multiple places.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
virDomainDiskSourcePrivateDataParse and virDomainDiskSourcePRParse don't
need the 'cleanup' label any more thanks to VIR_XPATH_NODE_AUTORESTORE.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The function does not have any code in the 'cleanup' label so we can
simplify the control flow.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
We can use our VIR_AUTOPTR machinery also for libxml2's xmlDoc and
xmlXPathContext.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
In a1c453dc08, during VIR_AUTOFREE() rewrite this wasn't done
properly. @port might be leaked because it's allocated in a for()
loop.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
In 669018bc9c I've introduced def->refresh which might be
allocated by virStoragePoolDefRefreshParse() but is never freed.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
The refresh_volume_allocation variable in
virStoragePoolDefParseXML() has been unused since its
introduction in commit 669018bc9c, and Clang rightfully
complains about this fact.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Use the new refresh volume allocation pool override to skip
computing the actual volume usage if disabled.
Signed-off-by: Jason Dillaman <dillaman@redhat.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
The new 'refresh' element can override the default refresh operations
for a storage pool. The only currently supported override is to set
the volume allocation size to the volume capacity. This can be specified
by adding the following snippet:
<pool>
...
<refresh>
<volume allocation='capacity'/>
</refresh>
...
</pool>
This is useful for certain backends where computing the actual allocation
of a volume might be an expensive operation.
Signed-off-by: Jason Dillaman <dillaman@redhat.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
The librbd API will transparently revert to a slow disk usage
calculation method if the fast-diff map is marked as invalid.
Signed-off-by: Jason Dillaman <dillaman@redhat.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
The unprivileged libvirtd does not have permission to create firewall
rules, or bridge devices, or do anything to the host network in
general. Historically we still activate the network driver though and
let the network start API call fail.
The startup code path which reloads firewall rules on active networks
would thus effectively be a no-op when unprivileged as it is impossible
for there to be any active networks
With the change to use a global set of firewall chains, however, we now
have code that is run unconditionally.
Ideally we would not register the network driver at all when
unprivileged, but the entanglement with the virt drivers currently makes
that impractical. As a temporary hack, we just make the firewall reload
into a no-op.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
During startup libvirtd creates top level chains for both ipv4
and ipv6 protocols. If this fails for any reason then startup
of virtual networks is blocked.
The default virtual network, however, only requires use of ipv4
and some servers have ipv6 disabled so it is expected that ipv6
chain creation will fail. There could equally be servers with
no ipv4, only ipv6.
This patch thus makes error reporting a little more fine grained
so that it works more sensibly when either ipv4 or ipv6 is
disabled on the server. Only the protocols that are actually
used by the virtual network have errors reported.
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
During startup we create some top level chains in which all
virtual network firewall rules will be placed. The upfront
creation is done to avoid slowing down creation of individual
virtual networks by checking for chain existance every time.
There are some factors which can cause this upfront creation
to fail and while a message will get into the libvirtd log
this won't be seen by users who later try to start a virtual
network. Instead they'll just get a message saying that the
libvirt top level chain does not exist. This message is
accurate, but unhelpful for solving the root cause.
This patch thus saves any error during daemon startup and
reports it when trying to create a virtual network later.
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
The rbd_list method has been deprecated in Ceph >= 14.0.0
in favour of the new rbd_list2 method which populates an
array of structs.
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
The rbd_list method has a quite unpleasant signature returning an
array of strings in a single buffer instead of an array. It is
being deprecated in favour of rbd_list2. To maintain clarity of
code when supporting both APIs in parallel, split the rbd_list
code out into a separate method.
In splitting this we now honour the rbd_list failures.
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
After this, newly added enums will not automatically show up in
driver output unless the driver code specifically sets report=true
Acked-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
Set report=true for all enums currently formatted in the XML
Acked-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
Set report=true for all enums currently formatted in the XML
Acked-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
Set report=true for all enums currently formatted in the XML
Acked-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
virCapsEnum report is an internal bool indicating whether we
should format the enum in the XML at all. This is unused for
now but will be handled in future patches.
We use a plain bool instead of tristate because the case here
is a bit different than the explicit @supported output. We
already report the equivalent of supported=YES|NO based on
what enum values are filled in. This adds report=false to
handle the ABSENT case.
Acked-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
Change domcaps to skip formatting XML if the default
TRISTATE_BOOL_ABSENT is found. Now when domcaps is extended, driver
XML output won't change until an explicit TRISTATE_BOOL value is set
in driver code.
Acked-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
<hostdev> and <features> are not supported. <loader>, <graphics>,
and <video> are supported conditionally
Acked-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
None of the <feature> bits are supported, and the <loader> piece
is only conditionally supported
Acked-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
Only gic->supported needs an explicit BOOL_NO setting, all other
'supported' values are handling things correctly
Acked-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
Switch most 'supported' handling to use virTristateBool, so eventually
we can handle the ABSENT state.
For now the XML formatter treats ABSENT the same as FALSE, so there's
no functional output change. This will be addressed in later patches
Acked-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
Similar to the macros we have for formatting enums, add a macro to
simplify formatting the pattern:
<FOO supported='yes|no'/>
Acked-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
In 0eca80e60 _class was renamed to klass for variety of struct
members. However, gather_usb_cap() was missed out in this rename
leaving FreeBSD build broken.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
This code originates from:
commit d0aa10fdd6
Author: Daniel P. Berrange <berrange@redhat.com>
Date: Tue Mar 3 12:03:44 2009 +0000
QEMU security driver usage for sVirt support (James Morris, Dan Walsh, Daniel Berrange)
Originally in the qemudDomainGetSecurityLabel function. It doesn't
appear to have done anything useful back then either. The other two
instances look like copy+paste
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
In d16f803d78 we've tried to solve an issue that after wiping an
image its format might have changed (e.g. from qcow2 to raw) but
libvirt wasn't probing the image format. We fixed this by calling
virStorageBackendRefreshVolTargetUpdate() which is what
refreshPool() would end up calling. But this shortcut is not good
enough because the function is called only for local types of
volumes (like dir, fs, netfs). But now that more backends support
volume wiping we have to call the function with more caution.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
So far we have two branches: either we zero BLOCK_PER_PACKET
(currently 128) block at once, or if we're close to the last block
then we zero out one block at the time. This is very suboptimal.
We know how many block are there left. Might as well just write
them all at once.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
This info can be useful to filter devices visible
to mgmt clients so that they won't see devices that
unsafe/not meaningful to pass thru.
Provide class info the way it is provided by udev or
kernel that is as single 6-digit hexadecimal.
Class element is not optional. I guess this should not
break users that use virNodeDeviceCreateXML because
they probably specify only scsi_host capability on
input and then node device driver gets other capabilities
from udev after device appeared.
HAL driver does not get support for the new element in
this patch.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Vim treats *.h files as cpp ones with respect to syntax highlighting.
Thus "class" in _virNodeDevCapPCIDev highlighted mistakenly.
This can be fixed by filetype detection code tunables but it
is more convinient to skip this tuning by every project member.
Let's just use "klass" as field name instead of _class or class
and add syntax rule.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Commit [1] moved snapshot list functions declaration into
its own file but missed a fix for vz driver.
[1] 9b75154c : snapshot: Break out virDomainSnapshotObjList into its own file
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
If pool refresh failed, then the internal table of volumes is
probably left in inconsistent or incomplete state anyways. Clear
it out then. This has an advantage that we can move the
virStoragePoolObjClearVols() from those very few backends that
do call it.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
This is a wrapper over refreshPool() call as at all places we are
doing basically the same. Might as well have a single function to
call.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
In bf5cf610f2 I've fixed a problem where iscsi-direct
backend was reporting only the last LUN. The fix consisted of
moving virStoragePoolObjClearVols() one level up. However, as it
turns out, storage driver already calls it before calling
refreshPool callback (which is
virStorageBackendISCSIDirectRefreshPool() in this case).
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
If virStorageBackendISCSIDirectVolWipeZero() fails, it has
already reported an error which is probably specific enough. Do
not overwrite it with some generic one.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
This function reports error for one of the two error paths. This
is unfortunate as a caller see this function failing but doesn't
know right away if an error was reported.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
Despite the misleading name, these were supposed to be used
with a System V style init; however, none of the platforms we
target is using that kind of init anymore: almost all Linux
distributions have switched to systemd, those that haven't
(such as Gentoo and Alpine) are mostly using OpenRC with
custom init scripts, and the BSDs have been doing their own
thing all along.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Not a single one of the platforms we target still uses Upstart, and
the Upstart project itself has been abandoned for several years now.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
snapshot_conf.h was mixing three separate types: the snapshot
definition, the snapshot object, and the snapshot object list.
Separate out the snapshot object list code into its own file, and
update includes for affected clients.
This is just code motion, but done in preparation of sharing a lot of
the object list code with checkpoints.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The next patch will require access to the helper functions
virDomainSnapshotDefFormatInternal and
virDomainSnapshotRedefineValidate from two different files; make the
file split easier by exporting these functions.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
snapshot_conf.h was mixing three separate types: the snapshot
definition, the snapshot object, and the snapshot object list.
Separate out the snapshot object code into its own file, which
includes moving a typedef to avoid circular inclusions.
Mostly straight code motion, although I fixed a comment along
the way, now that virDomainSnapshotForEachDescendent now
guarantees a topological visit (missed in b647d219).
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
It's easier to locate a typedef if they are stored in sorted order;
do so mechanically via:
$ sed -i '/typedef struct/ {N; N; s/\n//g}' src/conf/virconftypes.h
$ # sorting the lines
$ sed -i '/typedef struct/ s/;/;\n/g' src/conf/virconftypes.h
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
As explained in the previous patch, collecting pointer typedefs into a
common header makes it easier to avoid circular inclusions. Continue
the efforts by pulling the appropriate typedefs from capabilities.h
into the new header.
This patch is just straight code motion (all typedefs are listed in
the same order before and after the patch); a later patch will sort
things for legibility.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Right now, snapshot_conf.h is rather large - it deals with three
separate types: virDomainSnapshotDef (the snapshot definition as it
maps to XML), virDomainSnapshotObj (an object containing a def and the
relationship to other snapshots), and virDomainSnapshotObjList (a list
of snapshot objects), where two of the three types are currently
public rather than opaque. What's more, the types are circular: a
snapshot def includes a virDomainPtr, which contains a snapshot list,
which includes a snapshot object, which includes a snapshot def.
In order to split the three objects into separate files, while still
allowing each header to use sane typedefs to incomplete pointers, the
obvious solution is to lift the typedefs into yet another header, with
no other dependencies. Start the split by factoring out all struct
typedefs from domain_conf.h (enum typedefs don't get used in function
signatures, and function typedefs tend not to suffer from circular
referencing, so those stay put). The only other exception is
virDomainStateReason, which is only ever used directly rather than via
a pointer.
This patch is just straight code motion (all typedefs are listed in
the same order before and after the patch); a later patch will sort
things for legibility.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Checking that the derived class is larger than the requested parent
class saves us from some obvious mistakes, but as written, it does not
catch all the cases; in particular, it is easy to forget to update a
VIR_CLASS_NEW when changing the 'parent' member from virObject to
virObjectLockabale, but where the size checks don't catch that. Add a
parameter for one more layer of sanity checking.
It would be cool if we could get gcc to stringize typeof(parent) into
the string name of that type, so that we could confirm that the
precise parent class is in use rather than just a struct that happens
to have the same size as the parent class. But sizeof checks are
better than nothing.
Note that I did NOT change the fact that we require derived classes to
be larger (as the difference in size makes it easy to tell classes
apart), which means that even if a derived class has no functionality
to add (but rather exists for compiler-enforced type-safety), it must
still include a dummy member. But I did fix the wording of the error
message to match the code.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
By default, qemu user's home dir points to '/' which shouldn't be used
at all. We therefore pass the HOME variable from the current variable
iff not running as SUID, which means that for systemd we never set it.
This patch makes sure, that for system QEMU this is always set to
libDir/<driver>, session mode is left untouched.
Signed-off-by: Erik Skultety <eskultet@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
For session mode, only XDG_CACHE_HOME is set, because we want to remain
integrating with services in user session, but for system mode, this
would have become reading/writing to '/' which carries the obvious issue
with permissions (also, '/' is the wrong location in 99.9% cases anyway).
Signed-off-by: Erik Skultety <eskultet@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Some modules/libraries within QEMU could make use of the XDG_ vars when
writing their data to the disk. Define the most common XDG variables
and point them to the specific driver's libDir, i.e.
XDG_CACHE_HOME -> /var/lib/libvirt/<driver>/.cache
XDG_DATA_HOME -> /var/lib/libvirt/<driver>/.local/share
XDG_CONFIG_HOME -> /var/lib/libvirt/<driver>/.config
Signed-off-by: Erik Skultety <eskultet@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
The functions do basically exactly the same thing modulo few checks.
In case of virtio disks we check that the device is not multifunction as
that can't be unplugged at once. In case of USB and SCSI disks we
checked that no active block job is running.
The check for running blockjobs should have also been done for virtio
disks. By moving the multifunction check into the common function we fix
this case and also simplify the code.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Use the correct type in switch and populate the missing cases.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
We don't have any cleanup section, we can return the value directly.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Based on recent list questions about the proposed addition of
virDomainCheckpointCreateXML(REDEFINE), it is worth adding some
clarification to the existing snapshot redefine documentation that is
serving as the basis for checkpoints.
Normal snapshot creation requires very few elements from the user XML
(libvirt can pick sane defaults for items that are omitted, and many
fields, including <domain>, are documented as readonly output fields
ignored on input, produced by drivers that track it). But during
REDEFINE, the API wants the complete XML produced by an earlier
virDomainSnapshotGetXMLDesc; as the domain definition has likely
changed since the snapshot was first created, libvirt is unable to
recreate a <domain> sub-element that matches the original output
representing the domain state at the time the snapshot was first
created. In fact, reverting without a <domain> sub-element is risky
enough that we had to add a FORCE flag for virDomainSnapshotRevert().
In short, we only support omitting domain for qemu because of
backwards-compatibility to snapshots created before 0.9.5 started
capturing <domain>; even though there are other drivers like vbox that
do not output <domain> because they have other reliable ways to
revert.
And based on the confusion caused when omitting <domain> from snapshot
XML, the initial design for checkpoints in later patches will make
<domain> a mandatory element during its REDEFINE.
[Side note: the fact that <domain> can appear in <domainsnapshot> is a
reason we cannot add a new API for a bulk listing or redefine of all
snapshots of a single domain in one XML call (for example, a 1M
<domain> XML * 16 snapshots explodes into 16M in a bulk form, which
gets difficult to send over RPC). Perhaps we could add a flag to
request that the <domain> sub-element be omitted on output, but such
output is no longer suitable for sane REDEFINE input.]
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
I had to inspect the code to learn whether a final virObjectUnref()
calls ALL dispose callbacks in child-to-parent order (akin to C++
destructors), or whether I manually had to call a parent-class dispose
when writing a child class dispose method. The answer is the
former. (Thankfully, since VIR_FREE wipes out pointers for safety,
even if I had guessed wrong, I probably would not have tripped over a
double-free fault when the parent dispose ran for the second time). I
also had to read the code to learn if a dispose method was even
mandatory (it is not, although getting NULL through VIR_CLASS_NEW
requires a macro). While at it, the VIR_CLASS_NEW macro requires that
the virObject component at offset 0 be reached through the name
'parent', not 'object'.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
https://bugzilla.redhat.com/show_bug.cgi?id=1623389
If a device is detached twice from the same domain the following
race condition may happen:
1) The first DetachDevice() call will issue "device_del" on qemu
monitor, but since the DEVICE_DELETED event did not arrive in
time, the API ends claiming "Device detach request sent
successfully".
2) The second DetachDevice() therefore still find the device in
the domain and thus proceeds to detaching it again. It calls
EnterMonitor() and qemuMonitorSend() trying to issue "device_del"
command again. This gets both domain lock and monitor lock
released.
3) At this point, qemu sends us the DEVICE_DELETED event which is
going to be handled by the event loop which ends up calling
qemuDomainSignalDeviceRemoval() to determine who is going to
remove the device from domain definition. Whether it is the
caller that marked the device for removal or whether it is going
to be the event processing thread.
4) Because the device was marked for removal,
qemuDomainSignalDeviceRemoval() returns true, which means the
event is to be processed by the thread that has marked the device
for removal (and is currently still trying to issue "device_del"
command)
5) The thread finally issues the "device_del" command, which
fails (obviously) and therefore it calls
qemuDomainResetDeviceRemoval() to reset the device marking and
quits immediately after, NOT removing any device from the domain
definition.
At this point, the device is still present in the domain
definition but doesn't exist in qemu anymore. Worse, there is no
way to remove it from the domain definition.
Solution is to note down that we've seen the event and if the
second "device_del" fails, not take it as a failure but carry on
with the usual execution.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
ACKed-by: Peter Krempa <pkrempa@redhat.com>
A caller might be interested in differentiating the cause for
error, especially if DeviceNotFound error occurred.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
ACKed-by: Peter Krempa <pkrempa@redhat.com>
The aim of this function will be to fix return value of
qemuMonitorDelDevice() in one specific case. But that is yet to
come. Right now this is nothing but a plain substitution.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
ACKed-by: Peter Krempa <pkrempa@redhat.com>
Any job which is able to provide statistics that can be queried via
virDomainGetJob{Stats,Info} has to set an appropriate statsType.
Without a proper statsType qemuDomainJobInfoToParams and
qemuDomainJobInfoToInfo have no idea what statistics should be sent to
the API caller.
https://bugzilla.redhat.com/show_bug.cgi?id=1688774
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Fill in a default volume type for every pool type, as reported
by the VolGetInfo API. Now that we cover the whole enum, report
an error for invalid values.
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
We provide a custom configure option --enable-test-coverage and
'make cov' target to generate code coverage reports. However gnulib
already provides a 'make coverage' which 'just works' and doesn't
require a special configure option.
This drops our custom implementation in favor of 'make coverage'.
Reports are now output to cov/index.html
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
Introduce a simple validation helper to perform the cputune period and
quota checks so that we can get rid of those repetitive chunks. Since
this is a validation helper, this patch also moves the checks from the
'parse' phase into the 'validation' phase.
Signed-off-by: Suyang Chen <dawson0xff@gmail.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Apparently this was necessary in the past because old versions
of autoconf/automake didn't make them available, but these
days all of the platforms we target include recent enough
autotools - as evidenced by the fact that, for example, we
already use abs_top_srcdir in tools/ despite the fact that
tools/Makefile.am is missing the same boilerplate.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
According to the official documentation for autoconf[1], the
correct names for these variables are abs_top_{src,build}dir
rather than abs_top{src,build}dir; in fact, we're already
using the correct names in various places, so let's just make
everything nice and consistent.
[1] https://www.gnu.org/software/autoconf/manual/autoconf-2.69/html_node/Preset-Output-Variables.html
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
An upcoming patch wants to reuse XML parsing of both unix and tcp
network host descriptions in the context of setting up a backup
NBD server. Make that easier by refactoring the existing parser.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
We copy-and-paste a lot of our docs, as evidenced by the number of
*GetXMLDesc() functions which had the same unusual indentation and
missing capital in the second sentence of the returns paragraph.
Signed-off-by: Eric Blake <eblake@redhat.com>
Commit 09eb1ae0 added a new enum type for xenbus, and adjusted
affected switch statements in the qemu driver, but failed to notice
that the vbox driver had a similar switch statement.
Signed-off-by: Eric Blake <eblake@redhat.com>
Add support in the domXML<->native config converter for
max_grant_frames. Include a test for the conversion.
Signed-off-by: Jim Fehlig <jfehlig@suse.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Add support for setting max_grant_frames in libxl domain config
object and include a test to check that it is properly converted
from XML to libxl domain config.
Signed-off-by: Jim Fehlig <jfehlig@suse.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
All Xen domains have a xenbus device. Implicitly add one if not
already explicitly specified in the domain config.
Signed-off-by: Jim Fehlig <jfehlig@suse.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
xenbus is virtual controller (akin to virtio controllers) for Xen
paravirtual devices. Although all Xen VMs have a xenbus, it has
never been modeled in libvirt, or in Xen native VM config format
for that matter.
Recently there have been requests to support Xen's max_grant_frames
setting in libvirt. max_grant_frames is best modeled as an attribute
of xenbus. It describes the maximum IO buffer space (or DMA space)
available in xenbus for use by connected paravirtual devices. This
patch introduces a new xenbus controller type that includes a
maxGrantFrames attribute.
Signed-off-by: Jim Fehlig <jfehlig@suse.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Commit a3ab6d42 changed the libvirtd profile to a named profile,
breaking the apparmor driver's ability to detect if the profile is
active. When the apparmor driver loads it checks the status of the
libvirtd profile using the full binary path, which fails since the
profile is now referenced by name. If the apparmor driver is
explicitly requested in /etc/libvirt/qemu.conf, then libvirtd fails
to load too.
Instead of only checking the profile status by full binary path,
also check by profile name. The full path check is retained in case
users have a customized libvirtd profile with full path.
Signed-off-by: Jim Fehlig <jfehlig@suse.com>
Acked-by: Jamie Strandboge <jamie@canonical.com>
This helper performs a conversion from a "yes|no" string to a
corresponding boolean. This allows us to drop several repetitive
if-then-else string->bool conversion blocks.
Signed-off-by: Shotaro Gotanda <g.sho1500@gmail.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Luckily, the function returns only 0 or -1 so all the checks work
as expected. Anyway, our rule is that a positive value means
success so if the function ever returns a positive value these
checks will fail. Make them check for a negative value properly.
At the same time fix qemuDomainDetachExtensionDevice() reval
check. It is somewhat related to the aim of this patch.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
The qemuFirmwareFetchConfigs() function is supposed to fetch all
firmware descriptions from paths defined by firmware.json
specification. This includes user's $HOME directory. However, it
was agreed that if libvirtd is running as privileged user then
his $HOME is ignored (thus $HOME is included in the search only
for regular users). Well, I got the condition wrong - it should
have been reversed.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
snapshot_conf does all the hard work, the qemu driver just has to
accept the new flag.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
snapshot_conf does all the hard work, the test driver just has to
accept the new flag.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Wire up support for VIR_DOMAIN_SNAPSHOT_LIST_TOPOLOGICAL in the
domain-agnostic support code.
Clients of snapshot_conf using virDomainSnapshotForEachDescendant()
are using a depth-first visit but with postfix visits of a given
node. Changing this to a prefix visit of the given node instantly
turns this into a topologically-ordered visit. (A prefix
breadth-first visit would also be topologically sorted, but that
requires a queue while our recursion naturally has a stack).
With that change, we now always have a topological sort for
virDomainSnapshotListAllChildren() regardless of the new public API
flag. Then with one more tweak, we can also get a topological rather
than a faster random hash visit for virDomainListAllSnapshots(), by
doing a descendent walk from our internal metaroot (there, we let the
public API flag control behavior, because a topological sort DOES
require more stack and slightly more time).
Note that virDomainSnapshotForEach() still uses a random hash visit;
we could change that signature to take a tri-state for random, prefix,
or postfix visit if we ever had clients that cared about the
distinctions, but for now, none of the drivers seem to care.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
When using virDomainSnapshotCreateXML with the REDEFINE flag on
multiple snapshot metadata XML descriptions, we require that a child
cannot be redefined before its parent. Since libvirt already tracks a
DAG, it is more convenient if we can ensure that
virDomainListAllSnapshots() and friends have a way to return data in
an order that we can directly reuse, rather than having to
post-process the data ourselves to reconstruct the DAG.
Add VIR_DOMAIN_SNAPSHOT_LIST_TOPOLOGICAL as our new guarantee (well, a
guarantee at the time of the API call conclusion; there's always a
possible TOCTTOU race where someone redefining snapshots in between
the API results and the client actually using the list might render
the list out-of-date). Four listing APIs are directly benefitted by
the new flag; additionally, since we document that the older racy
ListNames interfaces should be sized by using the same flags on their
Num counterparts, the Num interfaces must document when they accept
(and ignore) the flag.
We could have supported the new flag just for the ListAll APIs (to
discourage people from using the older racy Num/ListNames APIs), but
it feels weird to special-case this flag value as being applicable to
only a subset of the API while all other List-related flags are
trivially applicable to all 6.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
https://bugzilla.redhat.com/show_bug.cgi?id=1564270
Now that everything is prepared for qemu driver we can enable
parser feature to allow users define such domains.
At the same time, introduce bunch of tests to test the feature.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
The firmware selection code will enable the feature if needed.
There's no need to require SMM to be enabled in that case.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
And finally the last missing piece. This is what puts it all
together.
At the beginning, qemuFirmwareFillDomain() loads all possible
firmware description files based on algorithm described earlier.
Then it tries to find description which matches given domain.
The criteria are:
- firmware is the right type (e.g. it's bios when bios was
requested in domain XML)
- firmware is suitable for guest architecture/machine type
- firmware allows desired guest features to stay enabled (e.g.
if s3/s4 is enabled for guest then firmware has to support
it too)
Once the desired description has been found it is then used to
set various bits of virDomainDef so that proper qemu cmd line is
constructed as demanded by the description file. For instance,
secure boot enabled firmware might request SMM -> it will be
enabled if needed.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Implementation for yet another part of firmware description
specification. This one covers selecting which files to parse.
There are three locations from which description files can be
loaded. In order of preference, from most generic to most
specific these are:
/usr/share/qemu/firmware
/etc/qemu/firmware
$XDG_CONFIG_HOME/qemu/firmware
If a file is found in two or more locations then the most specific
one is used. Moreover, if file is empty then it means it is
overriding some generic description and disabling it.
Again, this is described in more details and with nice examples
in firmware.json specification (qemu commit 3a0adfc9bf).
However, there's one slight difference - for the root user the
home directory is not searched. This follows rules laid out by
similar look up processes, e.g. PKI x509 certs are not searched
in /root but they are looked for under /home.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
The firmware description is a JSON file which follows
specification from qemu.git/docs/interop/firmware.json. The
description file basically says: Firmware file X is {bios|uefi},
supports these targets and machine types, requires these features
to be enabled on qemu cmd line and this is how you put it onto
qemu cmd line.
The firmware.json specification covers more (i.e. how to select
the right firmware) but that will be covered and implemented in
next commits.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
The idea is that using this attribute users enable libvirt to
automagically select firmware image for their domain. For
instance:
<os firmware='efi'>
<type arch='x86_64' machine='pc-q35-4.0'>hvm</type>
<loader secure='no'/>
</os>
<os firmware='bios'>
<type arch='x86_64' machine='pc-q35-4.0'>hvm</type>
</os>
(The automagic of selecting firmware image will be described in
later commits.)
Accepted values are 'bios' and 'efi' to let libvirt select
corresponding type of firmware.
I know it is a good sign to introduce xml2xml test case when
changing XML config parser but that will have to come later.
Firmware auto selection is not enabled for any driver just yet so
any xml2xml test would fail right away.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
This is going to extend virDomainLoader enum. The reason is that
once loader path is NULL its type makes no sense. However, since
value of zero corresponds to VIR_DOMAIN_LOADER_TYPE_ROM the
following XML would be produced:
<os>
<loader type='rom'/>
...
</os>
To solve this, introduce VIR_DOMAIN_LOADER_TYPE_NONE which would
correspond to value of zero and then use post parse callback to
set the default loader type to 'rom' if needed.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Except not really. At least for now.
In the future, the firmware will be selected automagically.
Therefore, it makes no sense to require the pathname of a
specific firmware binary in the domain XML. But since it is not
implemented do not really allow the path to be NULL. Only move
code around to prepare it for further expansion.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
In some cases, the string representing architecture is different
in qemu and libvirt. That is the reason why we have
virQEMUCapsArchFromString() and virQEMUCapsArchToString(). So
far, we did not need them outside of qemu_capabilities code, but
this will change shortly. Expose them then.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Move the code that (possibly) generates filename of NVRAM VAR
store into a single function so that it can be re-used later.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Similarly to CAT, when you set some values in an group, remove the group and
recreate it, the previous values will be kept there. In order to not get values
from a previous setting (a previous VM, for example), we need to set them to
sensible defaults. The same way we do that for CAT, just set the same values as
the default group has.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
For CAT we calculate unallocated parts of the cache, however with MBA this does
not make sense as the purpose of that is to limit the bandwidth and the setting
is only proportional relative to bandwidth settings for other groups.
This means it makes sense to set the values to 100% even if there are other
groups with some allocations and that we don't need to find the available
(unallocated) bandwidth in all the groups.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
The existing behavior for ppc64 guests is to always add a USB
keyboard and mouse combo if graphics are present; unfortunately,
this means any attempt to use a USB tablet will cause both pointing
devices to show up in the guest, which in turn will result in poor
user experience.
We can't just stop adding the USB mouse or start adding a USB tablet
instead, because existing applications and users might rely on the
current behavior; however, we can avoid adding the USB mouse if a USB
tablet is already present, thus allowing users and applications to
create guests that contain a single pointing device.
https://bugzilla.redhat.com/show_bug.cgi?id=1683681
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Cole Robinson <crobinso@redhat.com>
While the parser and schema have to accept all possible models,
virtio-(non-)transitional models are only applicable to
type=passthrough and should be otherwise rejected.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Cole Robinson <crobinso@redhat.com>
Only PCI devices have '/sys/class/net/<ifname>/device/resource' so we
need to skip this check for all other network devices.
Without this patch and RDMA enabled libvirt will not detect any network
device that doesn't have the path above which includes 'lo', 'virbr',
'tun', etc.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1639258
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
If we pass XML to virDomainDefineXML API with these two elements:
...
<title></title>
<description></description>
...
libvirt correctly ignores these two elements and they will not appear
in the parsed XML.
However, if we use virDomainSetMetadata API and with "" as value for
title or description we will end up with the parsed XML that contains
these empty elements.
Let's fix the behavior of this API to behave the same as
virDomainDefineXML.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1518042
Reviewed-by: John Ferlan <jferlan@redhat.com>
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Fix an incorrect @xmlDesc comment, as well as adding more details
about which XML element should be root.
Signed-off-by: Eric Blake <eblake@redhat.com>
Add a new function to make it possible to parse a list of snapshots
at once. This is a counterpart to an earlier patch making it
possible to produce all snapshots in a single XML string, and
intentionally parses the same top-level element <snapshots> with
an optional attribute current='name'.
Note that since we know we started with no relations at all, and
since checking parent relationships per-snapshot is not viable as
we don't control which order the snapshots appear in, that we are
fine with doing a final pass to update all parent/child
relationships among the definitions.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Pull out the portion of virDomainSnapshotRefinePrep() that deals
with definition sanity into a separate helper routine that can
be reused with bulk redefine, leaving behind only the code
specific to loop checking and in-place updates that are only
needed in single-definition handling.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Right now, the only callers of qemuDomainSnapshotDiscardAllMetadata()
are right before freeing the virDomainSnapshotObjList, so it did not
matter if the list's metaroot (which points to all the defined root
snapshots) is left inconsistent. But an upcoming patch will want to
clear all snapshots if a bulk redefine fails partway through, in
which case things must be reset. Make this work by teaching the
existing virDomainSnapshotUpdateRelations() to be safe regardless of
the incoming state of the metaroot (since we don't want to leak that
internal detail into qemu code), then fixing the qemu code to use
it after deleting all snapshots. Additionally, the qemu code must
reset vm->current_snapshot if the current snapshot was removed,
regardless of whether the overall removal succeeded or failed later.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Add a new function to output all of the domain's snapshots in one
buffer.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Split out an internal helper that produces format into a
virBuffer, similar to what domain_conf.c does, and making
the next patch easier to write.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
virDomainSnapshotDefFormat currently takes two sets of knobs:
an 'unsigned int flags' argument that can currently just be
VIR_DOMAIN_DEF_FORMAT_SECURE, and an 'int internal' argument used as
a bool to determine whether to output an additional element. It
then reuses the 'flags' knob to call into virDomainDefFormatInternal(),
which takes a different set of flags. In fact, prior to commit 0ecd6851
(1.2.12), the 'flags' argument actually took the public
VIR_DOMAIN_XML_SECURE, which was even more confusing. Let's borrow
from the style of that earlier commit, by introducing a function
for translating from the public flags (VIR_DOMAIN_SNAPSHOT_XML_SECURE
was just recently introduced) into a new enum specific to snapshot
formatting, and adjust all callers to use snapshot-specific enum
values when formatting, and where the formatter now uses a new
variable 'domainflags' to make it obvious when we are translating
from snapshot flags back to domain flags. We don't even have to
use the conversion function for drivers that don't accept the
public VIR_DOMAIN_SNAPSHOT_XML_SECURE flag.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Clean up the previous patch which abused switch on virDomainState
while working with a variable containing virDomainSnapshotState, by
converting the two affected switch statements to now use the right
enum.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
The existing virDomainSnapshotState is a superset of virDomainState,
adding one more state (disk-snapshot) on top of valid domain states.
But as written, the enum cannot be used for gcc validation that all
enum values are covered in a strongly-typed switch condition, because
the enum does not explicitly include the values it is adding to.
Copy the style used in qemu_blockjob.h of creating new enum names
for every inherited value, and update most clients to use the new
enum names anywhere snapshot state is referenced. The exception is
two switch statements in qemu code, which instead gain a fixme
comment about odd type usage (which will be cleaned up in the next
patch). The rest of the patch is fairly mechanical (I actually did
it by temporarily s/state/xstate/ in snapshot_conf.h to let the
compiler find which spots in the code used the field, did the
obvious search and replace in those functions, then undid the rename).
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
qemuDomainSnapshotWriteMetadata does not modify the directory name,
and making it const-correct aids in writing an upcoming patch.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
The current qemu code rejects the combination of the two flags
VIR_DOMAIN_SNAPSHOT_CREATE_LIVE in tandem with
VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE, but rather late in the cycle
(after the snapshot was already parsed), and with a rather confusing
message (complaining that live snapshots require external storage,
even if the redefined snapshot already declares external storage).
Hoist the rejection message to occur earlier (before parsing any
XML, which also aids upcoming patches that will implement bulk
redefine), and with a more typical error message about mutually
exclusive flags.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Failure would have occurred for @ctxt before in callers' other
virXPath calls and @def derefs.
Found by Coverity due to commit 66a508d2 using VIR_XPATH_NODE_AUTORESTORE
to access @ctxt before the if condition. The @def was noted by review.
Signed-off-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
If shutting down a container via setting the runlevel fails, the
control jumps right onto endjob label and doesn't even try
sending the signal. If flags allow it, we should try both
methods.
Signed-off-by: Maxim Kozin <kolomaxes@gmail.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
We can use STRNEQ() instead of STRNEQLEN() since we're only
interested in the trailing part of the string and we've
already verified that the length of file, name and suffix
are those we expect.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
ACKed-by: Peter Krempa <pkrempa@redhat.com>
It's a predicate, so bool is the appropriate return type.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
ACKed-by: Peter Krempa <pkrempa@redhat.com>
While this function is not, strictly speaking, a predicate,
it still mostly behaves like one as evidenced by the vast
majority of its callers, so using bool rather than int as
the return type makes sense.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
ACKed-by: Peter Krempa <pkrempa@redhat.com>
It's a predicate, so bool is the appropriate return type.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
ACKed-by: Peter Krempa <pkrempa@redhat.com>
Since qemu 2.13 reports the target architecture in a property called
'target' additionally to the property 'arch', that has been used in
qemu 2.12 in the response data of 'query-cpus-fast'.
Libvirts monitor code prefers the 'target' property over 'arch'.
At least for s390(x), target is reported as 's390x' while arch is 's390'.
In a later step a comparison is performed against 's390' which fails for
qemu 2.13 and later.
In consequence the architecture specific data for s390 won't be extracted
from the returned data, leading to incorrect values being reported by
virsh domstats --vcpu.
Changing to check explicitly for 's390' and 's390x'.
Signed-off-by: Viktor Mihajlovski <mihajlov@linux.ibm.com>
Reviewed-by: Bjoern Walk <bwalk@linux.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
Problem is that if there are no signatures for a CPU, then we
still allocate cpu->signatures (even though with size 0). Later,
we access cpu->signatures[0] if cpu->signatures is not NULL.
Invalid read of size 4
at 0x5F439D7: virCPUx86Translate (cpu_x86.c:2930)
by 0x5F3C239: virCPUTranslate (cpu.c:927)
by 0x57CE7A1: qemuProcessUpdateGuestCPU (qemu_process.c:5870)
...
Address 0xf752d40 is 0 bytes after a block of size 0 alloc'd
at 0x4C30EC6: calloc (vg_replace_malloc.c:711)
by 0x5DBDE4E: virAllocN (viralloc.c:190)
by 0x5F3E4FA: x86ModelCopySignatures (cpu_x86.c:990)
by 0x5F3E60F: x86ModelCopy (cpu_x86.c:1008)
by 0x5F3E7CB: x86ModelFromCPU (cpu_x86.c:1068)
by 0x5F4397E: virCPUx86Translate (cpu_x86.c:2922)
by 0x5F3C239: virCPUTranslate (cpu.c:927)
by 0x57CE7A1: qemuProcessUpdateGuestCPU (qemu_process.c:5870)
...
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
There is a lot of documentation in the comments about how PPC64 handles
passthrough VFIO devices to calculate the @memLockLimit. And more will
be added with the PPC64 NVLink2 support code.
Let's remove the PPC64 code from qemuDomainGetMemLockLimitBytes()
body and put it into a helper function. This will simplify the
flow of qemuDomainGetMemLockLimitBytes() that handles all the other
platforms and improves readability of the PPC64 specifics.
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
@passthroughLimit is being calculated even if @usesVFIO is false. After
that, an if-else conditional is used to check if we're going to sum it
up with @baseLimit.
This patch initializes @passthroughLimit to zero and always returns
@memKB = @baseLimit + @passthroughLimit. The conditional is then used to
calculate @passthroughLimit if @usesVFIO == true. This results in some
cycles being spared for the @usesVFIO == false scenario, but the real
motivation is to make the code simpler to add an alternative formula to
calculate @passthroughLimit for NVLink2.
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Capabilities should not duplicate data that are obvious from our
documentation and will not change with different QEMU binaries
or the way how we compile libvirt.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
When dealing with internal paths we don't need to worry about
whether or not suffixes are lowercase since we have full control
over them, which means we can avoid performing case-insensitive
string comparisons.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
ACKed-by: Peter Krempa <pkrempa@redhat.com>
This is the case-sensitive counterpart of the existing
virStringHasCaseSuffix() function.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
ACKed-by: Peter Krempa <pkrempa@redhat.com>
Despite its name, this is really just a general-purpose string
manipulation function, so it should be moved to the virstring
module and renamed accordingly.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
ACKed-by: Peter Krempa <pkrempa@redhat.com>
Despite its name, this is really just a general-purpose string
manipulation function, so it should be moved to the virstring
module and renamed accordingly.
A few trivial whitespace changes are squashed in.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
ACKed-by: Peter Krempa <pkrempa@redhat.com>
Despite its name, this is really just a general-purpose string
manipulation function, so it should be moved to the virstring
module and renamed accordingly.
In addition to the obvious s/File/String/, also tweak the name
to make it clear that the presence of the suffix is verified
using case-insensitive comparison.
A few trivial whitespace changes are squashed in.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
ACKed-by: Peter Krempa <pkrempa@redhat.com>
In these cases the check that is removed has been done a few
lines above already (as can even be seen in the context). Drop
them.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Commit a3ab6d42 changed the libvirtd profile to a named profile
but neglected to accommodate the change in the qemu profile
ptrace and signal rules. As a result, libvirtd is unable to
signal confined qemu processes and hence unable to shutdown
or destroy VMs.
Add ptrace and signal rules that reference the libvirtd profile
by name in addition to full binary path.
Signed-off-by: Jim Fehlig <jfehlig@suse.com>
Acked-by: Jamie Strandboge <jamie@canonical.com>
https://bugzilla.redhat.com/show_bug.cgi?id=1581670
Create the storage driver code to generate the output for the
storage pool capabilities XML.
Signed-off-by: John Ferlan <jferlan@redhat.com>
ACKed-by: Michal Privoznik <mprivozn@redhat.com>
Introduce the API to expose the storage pool capabilities along
with all the remote munglement required to hook up the client.
Signed-off-by: John Ferlan <jferlan@redhat.com>
ACKed-by: Michal Privoznik <mprivozn@redhat.com>
Add support to format the storage pool capabilities using
the virStoragePoolTypeInfoPtr to determine what capabilities
exist for the various pools and the driver capabilities to
determine whether the pool is compiled in and supported.
Signed-off-by: John Ferlan <jferlan@redhat.com>
ACKed-by: Michal Privoznik <mprivozn@redhat.com>
https://bugzilla.redhat.com/show_bug.cgi?id=1581670
During storage driver backend initialization, let's save
which backends are available in the storage pool capabilities.
In order to format those, we need add a connectGetCapabilities
processor to the storageHypervisorDriver. This allows a storage
connection, such as "storage:///system" to find the API and
format the results, such as:
virsh -c storage:///system capabilities
<capabilities>
<pool>
<enum name='type'>
<value>dir</value>
<value>fs</value>
<value>netfs</value>
<value>logical</value>
<value>iscsi</value>
<value>iscsi-direct</value>
<value>scsi</value>
<value>mpath</value>
<value>disk</value>
<value>rbd</value>
<value>sheepdog</value>
<value>gluster</value>
<value>zfs</value>
</enum>
</pool>
</capabilities>
Signed-off-by: John Ferlan <jferlan@redhat.com>
ACKed-by: Michal Privoznik <mprivozn@redhat.com>
Introduce the bare bones functions to processing capability
data for the storage driver.
Since there will be no need for the <host> output, we need
to filter that data.
Signed-off-by: John Ferlan <jferlan@redhat.com>
ACKed-by: Michal Privoznik <mprivozn@redhat.com>
The ZFS pool is documented as not using pool format types, so remove
the defaultFormat value.
Signed-off-by: John Ferlan <jferlan@redhat.com>
ACKed-by: Michal Privoznik <mprivozn@redhat.com>
The multipath pool is documented as not using the volume type,
so let's just remove it.
Signed-off-by: John Ferlan <jferlan@redhat.com>
ACKed-by: Michal Privoznik <mprivozn@redhat.com>
The iscsi and iscsi-direct pools are documented as not using
the volume type, so let's just remove it. Besides it would
have produced bad output since formatting uses the Disk types.
Signed-off-by: John Ferlan <jferlan@redhat.com>
ACKed-by: Michal Privoznik <mprivozn@redhat.com>
The scsi pool is documented as not using the volume type,
so let's just remove it.
Signed-off-by: John Ferlan <jferlan@redhat.com>
ACKed-by: Michal Privoznik <mprivozn@redhat.com>
The rbd pool is documented as not using the volume type,
so let's just remove it.
Signed-off-by: John Ferlan <jferlan@redhat.com>
ACKed-by: Michal Privoznik <mprivozn@redhat.com>
The sheepdog pool is documented as not using the volume type,
so let's just remove it. Besides it would have produced bad
results since the defaultType is FILE but the formatting used
the Disk types.
Signed-off-by: John Ferlan <jferlan@redhat.com>
ACKed-by: Michal Privoznik <mprivozn@redhat.com>
Rather than moving the XPath root node in the caller and then still
passing it down, make sure that the callees move the node themselves.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Remove logic necessary to figure out whether to format the 'features'
element by using virXMLFormatElement.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Use virXMLFormatElement for the formatting which allows us to avoid
looking through the array to see if any feature is enabled.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
If none of the 'capabilities' features are enabled we'd still format the
opening and closing tag for the <capabilities element.
The implementation is suboptimal but will be refactored for a better
approach. This is done prior to the refactor to show that tests are not
impacted.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Use VIR_AUTOCLEAN to avoid leaking the buffer on error path and get rid
of resetting mid loop since virXMLFormatElement does the reset
internally.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
'i' is always in range of the enum, thus the name is always populated by
virDomainFeatureTypeToString.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
These buffers are used temporarily for some of the partial formatters
but not globally. Prefix the name with 'tmp' to be explicit.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Pure code motion of code for formatting domain features to a function
called virDomainDefFormatFeatures. Best viewed with the '--patience'
option for git show.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Split out the code into a separate function named
virDomainDefFormatBlkiotune and use virXMLFormatElement.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Use virXMLFormatElement to format the internals along with simplifying
cleanup code paths.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Use virXMLFormatElement to format the internals along with simplifying
cleanup code paths.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Use virXMLFormatElement to format the internals along with simplifying
cleanup code paths.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Use virXMLFormatElement to format the internals along with simplifying
cleanup code paths.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Use virXMLFormatElement to format the internals along with simplifying
cleanup code paths.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Use virXMLFormatElement to format the internals along with simplifying
cleanup code paths.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Refactor the function to use the XML formatting aid and use automatic
cleaning to simplify the control flow.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The function does not transfer errors from 'attrBuf' and 'childBuf'
arguments into 'buf', but rather reports them right away, thus we need
to make sure that it's always checked.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The qemuMigrationParamsApply internal API was designed to apply all
migration parameters and capabilities before we start to migrate a
domain. While migration parameters are only passed to QEMU when we
explicitly want to set a specific value, capabilities are always either
enabled or disabled.
Thus when this API is called outside migration job, e.g., via a call to
qemuDomainMigrateSetMaxSpeed with VIR_DOMAIN_MIGRATE_MAX_SPEED_POSTCOPY
flag, we would call migrate-set-capabilities and disable all
capabilities. However, changing capabilities while migration is already
running does not make sense and our code should never be trying to do
so. In fact QEMU even reports an error if migrate-set-capabilities is
called during migration and qemuDomainMigrateSetMaxSpeed would fail
with:
internal error: unable to execute QEMU command
migrate-set-capabilities: There's a migration process in progress
With this patch qemuMigrationParamsApply never tries to call
migrate-set-capabilities outside of migration job. When the capabilities
bitmap is all zeros (which is its initial value after
qemuMigrationParamsNew), we just skip the command. But when any
capability bit is set to 1 by a non-migration job, we report an error to
highlight a bug in our code.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Acked-by: Peter Krempa <pkrempa@redhat.com>
Further testing with more devices showed that we sometimes have a
different depth of pci device paths when accessing sysfs for device
attributes.
But since the access is limited to a set of filenames and read only it
is safe to use a wildcard for that.
Related apparmor denies - while we formerly had only considered:
apparmor="DENIED" operation="open"
name="/sys/devices/pci0000:00/0000:00:02.1/uevent"
requested_mask="r"
We now also know of cases like:
apparmor="DENIED" operation="open"
name="/sys/devices/pci0000:00/0000:00:03.1/0000:1c:00.0/uevent"
requested_mask="r"
Fixes: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1817943
Acked-by: Jamie Strandboge <jamie@canonical.com>
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
Further testing with different devices showed that we need more rules
to drive gl backends with nvidia cards. Related denies look like:
apparmor="DENIED" operation="open"
name="/usr/share/egl/egl_external_platform.d/"
requested_mask="r"
apparmor="DENIED" operation="open"
name="/proc/modules"
requested_mask="r"
apparmor="DENIED" operation="open"
name="/proc/driver/nvidia/params"
requested_mask="r"
apparmor="DENIED" operation="mknod"
name="/dev/nvidiactl"
requested_mask="c"
Fixes: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1817943
Acked-by: Jamie Strandboge <jamie@canonical.com>
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
https://bugzilla.redhat.com/show_bug.cgi?id=1685151
This reverts commit e4a969092b.
Now that drivers may call virConnectOpen() on secondary drivers, it
doesn't make much sense to have autostart separated from driver
initialization callback. In fact, it creates a problem because one
driver during its initialization might try to fetch an object from
another driver but since the object is yet to be autostarted the fetch
fails. This has been observed in reality: qemu driver performs
qemuProcessReconnect() during qemu's stateInitialize phase which may
call virDomainDiskTranslateSourcePool() which connects to the storage
driver to look up the volume. But the storage driver did not autostart
its pools yet therefore volume lookup fails and the domain is killed.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
https://bugzilla.redhat.com/show_bug.cgi?id=1685151
This reverts commit cefb97fb81.
The stateAutoStart callback will be removed in the next commit.
Therefore move autostarting of domains, networks and storage
pools back into stateInitialize callbacks.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The stateAutoStart callback will go away shortly. Therefore, move
the autostart call into state initialize callback.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The order in which drivers are registered is important because
their stateInitialize and stateAutoStart callback are called in
that order. Well, stateAutoStart is going away and therefore if
there is some dependency between two drivers (e.g. when
initializing storage driver expects secret driver to be available
already), the registration of such drivers must happen in correct
order.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
This fixes several CPUs which were incorrectly detected as
Skylake-Client.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
This fixes several CPUs which were incorrectly detected as a different
CPU model.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The family/model numbers are nice for humans or for comparing with
/proc/cpuinfo, but sometimes there's a need to see the CPUID
representation of the signature. Let's add it into a comment for each
signature in out cpu_map XMLs as the conversion is not exactly
straightforward.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The function exports the functionality of x86DataToSignatureFull and
x86MakeSignature to the test suite.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Most places in qemu_capabilities.c which call virQEMUCapsGetHostCPUData
actually need qemuMonitorCPUModelInfoPtr from QEMU caps. Let's use the
wrapper introduced in the previous commit instead.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
This is a simple wrapper around virQEMUCapsGetHostCPUData usable in
tests for getting qemuMonitorCPUModelInfoPtr from QEMU caps.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The code for transforming qemuMonitorCPUModelInfo data from QEMU into
virCPUDefPtr consumable by virCPU* APIs was hidden inside
virQEMUCapsInitCPUModelX86. This patch moves it into a new function to
make it usable in tests.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The log message may be useful when debugging why a specific CPU model
was selected for a given set of CPUID data.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
CPU signatures in the cpu_map serve as a hint for CPUID to CPU model
matching algorithm. If the CPU signatures matches any CPU model in the
cpu_map, this model will be the preferred one.
This works out well and solved several mismatches, but in real world
CPUs which should match a single CPU model may be produced with several
different signatures. For example, low voltage Broadwell CPUs for
laptops and Broadwell CPUs for servers differ in CPU model numbers while
we should detect them all as Broadwell CPU model.
This patch adds support for storing several signatures for a single CPU
model to make this hint useful for more CPUs. Later commits will provide
additional signatures for existing CPU models, which will correct some
results in our CPU test suite.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
In preparation for storing several CPU signatures in a single CPU model,
we need to turn virCPUx86Model's signature into an array of signatures.
The parser still hardcodes the number of signatures to 1, but the
following patch will drop this limit.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Introduce a helper for copying CPU signature between two CPU models.
It's not very useful until the way we store signatures is changed in the
next patch.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Having multiple CPU model definitions with the same name could result in
unexpected behavior.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The code is separated into a new x86ModelParseFeatures function.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The code is separated into a new x86ModelParseVendor function.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The code is separated into a new x86ModelParseSignature function.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The code is separated into a new x86ModelParseAncestor function.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
After commits e2087c2 and ec0793de older GCC started act very smart and
complain about potentially uninitialized variable, which existed prior
to these patches + even if the affected vars were left uninitialized the
function responsible for filling them in would have failed with NULL
being returned which the caller has always handled carefully.
Although GCC complained only about a single variable, let's initialize
all of them so as to prevent any further potential breakages.
Signed-off-by: Erik Skultety <eskultet@redhat.com>
Add <controller type='scsi' model handling for virtio transitional
devices. Ex:
<controller type='scsi' model='virtio-transitional'/>
* "virtio-transitional" maps to qemu "virtio-scsi-pci-transitional"
* "virtio-non-transitional" maps to qemu "virtio-scsi-non-transitional"
The naming here doesn't match the pre-existing model=virtio-scsi.
The prescence of '-scsi' there seems kind of redundant as we have
type='scsi' already, so I decided to follow the pattern of other
patches and use virtio-transitional etc.
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
<input> devices lack the model= attribute which is used by
most other device types. To eventually support
virtio-input-host-pci-{non-}traditional in qemu, let's add
a standard model= attribute. This just adds the domain_conf
wiring
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
<filesystem> devices lack the model= attribute which is used by
most other device types. To eventually support
virtio-9p-pci-{non-}traditional in qemu, let's add a standard
model= attribute. The accepted values are:
- virtio
- virtio-transitional
- virtio-non-transitional
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
qemu vhost-scsi devices map to XML roughly like:
<hostdev mode='subsystem' type='scsi_host'>
<source protocol='vhost' wwpn=X/>
</hostdev>
To support vhost-scsi-pci-{non-}traditional in qemu, we
need to to extend the SCSI Host hostdev XML to handle
model= value. This matches the XML model= format used
for mediated devices. This is just the domain_conf bits
and some XML test cases.
Use of virtio-X naming here does not match the hostdev
protocol=vhost nor does it match the qemu vhost-X device
naming, however it's more consistent with all other
model= names in this area, and also matches the
inconsistency of <vsock> devices which use model=virtio
but map to vhost-vsock on the qemu commandline
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
Add new <disk> model values for virtio transitional devices. When
combined with bus='virtio':
* "virtio-transitional" maps to qemu "virtio-blk-pci-transitional"
* "virtio-non-transitional" maps to qemu "virtio-blk-pci-non-transitional"
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
<disk> devices lack the model= attribute which is used by
most other device types. bus= mostly acts as one, but it
serves other purposes too like determing what target=
prefix to use, and for matching against controller type=
values.
Extending bus= to handle additional virtio transitional
devices will complicate apps lives, and it isn't a clean
mapping anyways. So let's bite the bullet and add a new
<disk model=X/> attribute, and wire up common handling
for virtio and virtio-{non-}transitional
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
Add a single QEMU_CAPS_VIRTIO_PCI_TRANSITIONAL that
will be set if any of the following qemu devices are found:
virtio-blk-pci-transitional
virtio-blk-pci-non-transitional
virtio-net-pci-transitional
virtio-net-pci-non-transitional
vhost-scsi-pci-transitional
vhost-scsi-pci-non-transitional
virtio-rng-pci-transitional
virtio-rng-pci-non-transitional
virtio-9p-pci-transitional
virtio-9p-pci-non-transitional
virtio-balloon-pci-transitional
virtio-balloon-pci-non-transitional
vhost-vsock-pci-transitional
vhost-vsock-pci-non-transitional
virtio-input-host-pci-transitional
virtio-input-host-pci-non-transitional
virtio-scsi-pci-transitional
virtio-scsi-pci-non-transitional
virtio-serial-pci-transitional
virtio-serial-pci-non-transitional
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
https://bugzilla.redhat.com/show_bug.cgi?id=1658504
This function is called when a domain is starting up (in qemu
driver that is when qemu cmd line is generated). It is used to
translate <disk type='volume'/> to something usable by filling in
virStorageSource (e.g. fetching disk path, or some connection URI
for a network FS). But some of these info are not stored in
status XML and thus the function is called on
qemuProcessReconnect too to reconstruct runtime data. But this
poses a problem because after the first run the mode is set to
'direct', but in the second run this triggers a failure because
mode is valid only for 'iscsi' volumes and not 'iscsi-direct'
ones.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Rewrite the code to make usage of some VIR_AUTOFREE logic.
Signed-off-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Now that we're using VIR_AUTOFREE there's quite a bit of clean up
possible for now unnecessary goto paths.
Signed-off-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Let's make use of the auto __cleanup capabilities for VIR_FREE consumers.
In some cases adding or removing blank lines for readability.
Signed-off-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
In preparation for VIR_AUTOFREE usage, let's remove a couple
of unused variables so that clang compilations won't fail.
Signed-off-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Now that we're using VIR_AUTOPTR(virBitmap) there's a couple of methods
that we can clean up some now unnecessary goto paths.
Signed-off-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Let's make use of the auto __cleanup capabilities for virBitmapPtr.
Signed-off-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
In preparation for using auto free mechanism, change to using the
VIR_STEAL_PTR on @def to @ret and of course be sure to properly clean
up @def in cleanup.
Signed-off-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Use the new helper when moving around the current node of the XPath
context.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Quite a few parts modify the XPath context current node to shift the
scope and allow easier queries. This also means that the node needs
to be restored afterwards.
Introduce a macro based on 'VIR_AUTOCLEAN' which adds a local structure
on the stack remembering the original node along with a function which
will make sure that the node is reset when the local structure leaves
scope.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The helper function is used by the VIR_AUTOUNREF macro. Prior art is to
clear the pointer even if the variable goes out of scope.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
We'd free only the first element of the vector leaking the rest.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
We don't need it as there's a separate macro for auto-freeing of string
lists.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Use of VIR_AUTOPTR and virString is confusing as it's a list and not a
single pointer. Replace it by VIR_AUTOSTRINGLIST as string lists are
basically the only sane NULL-terminated list we can have.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Similar to VIR_AUTOPTR, VIR_AUTOSTRINGLIST defines a list of strings
which will be freed if the pointer is leaving scope.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Let's make use of the auto __cleanup capabilities cleaning up any
now unnecessary goto paths.
Signed-off-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
The existing qemu snapshot code has a slight bug: if the domain
is currently pmsuspended, you can't use the _REDEFINE flag even
though the current domain state should have no bearing on being
able to recreate metadata state; and conversely, you can use the
_REDEFINE flag to create snapshot metadata claiming to be
pmsuspended as a bypass to the normal restrictions that you can't
create an original qemu snapshot in that state (the restriction
against pmsuspend is specific to qemu, rather than part of the
driver-agnostic snapshot_conf code).
Fix this by checking the snapshot state (when redefining) instead
of the domain state (which is a subset of snapshot states).
Fixes the second problem mentioned in https://bugzilla.redhat.com/1680304
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Both block_size and nb_block are unit32_t and multiplying them overflows
at 4GiB.
Moreover, the iscsi_*10_* APIs use 32bit number of blocks and thus they
can only address images up to 2TiB with 512B blocks. Let's use 64b
iscsi_*16_* APIs instead.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
When fetching LUNs from iscsi server the
virISCSIDirectReportLuns() is called. This function does some
libiscsi calls and then calls virISCSIDirectRefreshVol() over
each LUN found. It's unfortunate that the latter calls
virStoragePoolObjClearVols() as we lose all LUNs processed
in previous iterations.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
Jirka reported a bug that with every 'virsh pool-refresh' an
iscsi-direct pool would grow and grow. The problem is that
virISCSIDirectRefreshVol() only adds to def->capacity and
def->allocation but nothing clears it out to begin with.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
For consistency with other error messages, and the fact that
the object is always called a virDomainSnapshot rather than
a mere virSnapshot, include the word "domain" in the error
message.
Suggested-by: John Ferlan <jferlan@redhat.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Commit 28f8dfdc (1.0.0) added a flag to virDomainGetXMLDesc, but
failed to document its effects. And considering that the
MIGRATABLE flag has been the source of past bugs (CVE-2014-7823,
fixed in commit b1674ad5 (1.2.11), or even cf2d4c60 (1.2.13) where
flag mismatch broke virsh edit), make the wording wishy-washy
enough to discourage using the flag casually, by mentioning that
the resulting XML is more for internal use than for validation
against the schema.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Due to historical back-compat, bare 'virsh snapshot-create-as'
favors internal snapshots (but can't be used on domains with raw
storage), while 'virsh snapshot-create-as --disk-only' favors
external snapshots. What's more, snapshots created with
--disk-only while the domain was running are marked as snapshot
state 'disk-snapshot', while snapshots created while the domain
was offline are marked as snapshot state 'shutdown' (a
'disk-snapshot' image might not be quiescent, while a 'shutdown'
snapshot always is).
But this leads to some interesting problems: if we create a
--disk-only snapshot of an offline guest, and then immediately try
to 'virsh snapshot-create --redefine' using the resulting XML to
overwrite the existing snapashot in place, things silently succeed,
but 'virsh snapshot-create --redefine --disk-only' fails with an
error message that the snapshot state is not 'disk-only'. Worse,
if we delete the snapshot metadata first and then try to recreate
things, omitting --disk-only fails because the verification code
wants to force the default of an internal snapshot (which doesn't
work with raw disks), and using --disk-only still fails because the
snapshot XML is not 'disk-only' - making it impossible to recreate
the snapshot metadata (or to transfer it from one libvirtd host to
another). Ideally, the presence or absence of the --disk-only
flag, and the presence or absence of an existing snapshot being
overwritten, shouldn't matter; if the XML is valid for one
situation, it should always be valid to redefine the metadata for
that snapshot.
Fix things by uniformly using virDomainSnapshotDefIsExternal()
(caching the results up front, and eliminating other 'if' clauses
now rendered redundant) when deciding whether the XML being
requested for redefinition should permit external or force internal
state capture (we got it right in only one out of three places in
the function).
See also https://bugzilla.redhat.com/1680304; this fixes the
domain-agnostic problems mentioned there, but another patch is
needed to fix further oddities with the qemu driver. I did not
check for sure when the problems were introduced (git blame puts
some affected hunks as far back as 1.0.0), but it was definitely
been broken even before when commit 670e86bf (1.1.4) factored
redefine prep out of qemu code into the common snapshot_conf code.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Upcoming patches plan to introduce virDomainCheckpointPtr as a new
object for use in incremental backups, along with documentation on
how incremental backups differ from snapshots. But first, we need
to rename any existing mention of a 'system checkpoint' to instead
be a 'full system snapshot', so that we aren't overloading
the term checkpoint.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
vcpupin will fail when maxvcpus is larger than current
vcpu:
virsh vcpupin win7 --vcpu 0 --cpulist 5-6
error: Requested operation is not valid: cpu affinity is not supported
win7 xml in the command above is like below:
...
<vcpu current="3" placement="static">8</vcpu>
...
The reason is vcpu[3] and vcpu[4] have zero tids and should not been
compared as valid situation in qemuDomainRefreshVcpuInfo().
This issue is introduced by commit 34f7743, which fix recording of vCPU
pids for MTTCG.
Signed-off-by: Yi Wang <wang.yi59@zte.com.cn>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Added GPFS as shared file system recognized during live migration
security checks.
GPFS is 'IBM General Parallel File System' also called
'IBM Spectrum Scale'
BUG: https://bugzilla.redhat.com/show_bug.cgi?id=1679528
Signed-off-by: Diego Michelotto <diego.michelotto@cnaf.infn.it>
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
The structure used to handle network entries was based on 'if,else'
conditions. This commit converts this ugly structure into a switch to
clearify each option of the handler.
Signed-off-by: Julio Faracco <jcfaracco@gmail.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Extract out the network "type" processing into it's own method
rather than inline within lxcNetworkParseDataSuffix.
Signed-off-by: Julio Faracco <jcfaracco@gmail.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
This commit removes the full network entry setting: "lxc.network.X" to
type only. Like "type", "name", "flags", etc. This will handle entries
regardless of whether they are prefixed by "lxc.network." (today) or
"lxc.net.X." (the future).
Signed-off-by: Julio Faracco <jcfaracco@gmail.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Refactor lxcNetworkWalkCallback to be a simple method to handle
both possible network settings with indexes or the simple one. It is
better the decouple the whole algorithm to parse data to only parse
which entry type libvirt is handling.
The new method is responsible to verify is the settings correspond to
network entry. Right now, it is only verifying "lxc.network.", but in
the future, it can be used to verify "lxc.net.X." too. Any other case
would be rejected.
On the other hand, the idea here is working only with types. If we know
that entry is part of network settings, after we just need to know which
type is. It keeps the handler simple.
Signed-off-by: Julio Faracco <jcfaracco@gmail.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
The new method called lxcNetworkParseDataIPs() is responsible to handle
IPv{4,6} settings now. The idea is let lxcNetworkWalkCallback() method
handle all entries related to network definition only.
Signed-off-by: Julio Faracco <jcfaracco@gmail.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
libvirt_iohelper is used internally by the virFileWrapperFd APIs;
more specifically, in the QEMU driver we have the doCoreDump() and
qemuDomainSaveMemory() helper functions as users, and those in turn
end up being called by the implementation of several driver APIs.
By calling virReportError() if libvirt_iohelper has failed, we
overwrite whatever generic error message QEMU might have raised
with the more useful one generated by the helper program.
After this commit, the user will be able to see the error directly
instead of having to dig in the journal or libvirtd log.
https://bugzilla.redhat.com/show_bug.cgi?id=1578741
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
virFileWrapperFdFree(), like all free functions, is supposed
to only release allocated resources, so error reporting is
better suited for virFileWrapperFdClose().
This reverts commit b0c3e93180.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Right now we're reporting errors in virFileWrapperFdFree(),
but that's hardly the appropriate place to do so, as free
functions are supposed to do nothing more than release
allocated resources.
We want to move that code back into virFileWrapperFdClose(),
but before we can do that we need to make sure the function
is actually called every time we're done processing the
wrapped file. The cleanup path is the obvious candidate.
In a couple of cases we can just move the call, but for the
remaining ones we need to duplicate it instead in order not
to alter the existing behavior. We do, however, make sure
that in all cases a failure to properly close the wrapper
results in the overall operation being reported as failed.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
We'll want to use this function in the cleanup path soon,
and in order to be able to do that we need to make sure we
can call it multiple times on the same virFileWrapperFd
without side effects.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Replace virDomainChrSourceDefFree with virObjectUnref.
Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Use refcounting for priv->monConfig instead of asymmetric freeing.
Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Change fb01e1a44 "virt-aa-helper: generate rules for gl enabled
graphics devices" implemented the detection for gl enabled
devices in virt-aa-helper. But further testing showed
that it will need much more access for the full gl stack
to work.
Upstream apparmor just recently split those things out and now
has two related abstractions at
https://gitlab.com/apparmor/apparmor/blob/master:
- dri-common at /profiles/apparmor.d/abstractions/dri-common
- mesa: at /profiles/apparmor.d/abstractions/mesa
If would be great to just include that for the majority of
rules, but they are not yet in any distribution so we need
to add rules inspired by them based on the testing that we
can do.
Furthermore qemu with opengl will also probe the backing device
of the rendernode for attributes which should be safe as
read-only wildcard rules.
Fixes: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1815452
Acked-by: Jamie Strandboge <jamie@canonical.com>
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
Change fb01e1a44 "virt-aa-helper: generate rules for gl enabled
graphics devices" implemented the detection for gl enabled
devices in virt-aa-helper. But it will in certain cases e.g. if
no rendernode was explicitly specified need to read /dev/dri
which it currently isn't allowed.
Add a rule to the apparmor profile of virt-aa-helper itself to
be able to do that.
Acked-by: Jamie Strandboge <jamie@canonical.com>
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
Add a bhyveDomainDefNeedsISAController() helper function
which by domain configuration determines whether LPC controller is
required or not.
Signed-off-by: Roman Bogorodskiy <bogorodskiy@gmail.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Implement the MSRs ignore unknown reads and writes feature
that's specified using:
<features>
...
<msrs unknown='ignore'>
...
</features>
in the domain XML.
In bhyve, it's just passing '-w' command line argument to the bhyve(8)
executable.
Signed-off-by: Roman Bogorodskiy <bogorodskiy@gmail.com>
Reviewed-by: Cole Robinson <crobinso@redhat.com>
Introduce the 'msrs' feature element that controls Model Specific
Registers related behaviour. At this moment it allows only
single tunable attribute "unknown":
<msrs unknown='ignore|fault'/>
Which tells hypervisor to ignore accesses to unimplemented
Model Specific Registers. The only user of that for now is going
to be the bhyve driver.
Signed-off-by: Roman Bogorodskiy <bogorodskiy@gmail.com>
Reviewed-by: Cole Robinson <crobinso@redhat.com>
Replace all uses where virBuffer would need clearing on the cleanup
path.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
virBuffer is almost always stack-allocated, but requires freeing of the
internals on error. Introduce a VIR_AUTOCLEAN function to deal with
this.
Along with the addition add a test which would leak the buffer contents
if it weren't autocleaned.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
The new utility macros are useful for variables we put on the stack but
require some cleanup. The most prominent of those is virBuffer which is
used almost exclusively in that way.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
The conversion to VIR_AUTOFREE of 'escapeList' introduced memory leak of
the copied item to be escaped:
==17517== 2 bytes in 1 blocks are definitely lost in loss record 1 of 32
==17517== at 0x483880B: malloc (vg_replace_malloc.c:309)
==17517== by 0x54D666D: strdup (in /usr/lib64/libc-2.28.so)
==17517== by 0x497663E: virStrdup (virstring.c:956)
==17517== by 0x497663E: virStrdup (virstring.c:945)
==17517== by 0x48F8853: virBufferEscapeN (virbuffer.c:707)
==17517== by 0x403C9D: testBufEscapeN (virbuftest.c:383)
==17517== by 0x405FA8: virTestRun (testutils.c:174)
==17517== by 0x403A70: mymain (virbuftest.c:517)
==17517== by 0x406BC9: virTestMain (testutils.c:1097)
==17517== by 0x5470412: (below main) (in /usr/lib64/libc-2.28.so)
[...] (all other have same backtrace as it happens in a loop)
Fix it by reverting all the VIR_AUTO nonsense in this function as there
is exactly one place where it's handled.
This effectively reverts commits:
d0a92a037196fbf6df90d261ed2fb1
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
'virBufferFreeAndReset' does not free the top level structure itself.
Additionally we almost exclusively use stack'd buffers rather than
pointers.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
dnsmasq documentation says that the *IPv4* prefix/network
address/broadcast address sent to dhcp clients will be automatically
determined by dnsmasq by looking at the interface it's listening on,
so the original libvirt code did not add a netmask to the dnsmasq
commandline (or later, the dnsmasq conf file).
For *IPv6* however, dnsmasq apparently cannot automatically determine
the prefix (functionally the same as a netmask), and it must be
explicitly provided in the conf file (as a part of the dhcp-range
option). So many years after IPv4 DHCP support had been added, when
IPv6 dhcp support was added the prefix was included at the end of the
dhcp-range setting, but only for IPv6.
A user had reported a bug on a host where one of the interfaces was a
superset of the libvirt network where dhcp is needed (e.g., the host's
ethernet is 10.0.0.20/8, and the libvirt network is 10.10.0.1/24). For
some reason dnsmasq was supplying the netmask for the /8 network to
clients requesting an address on the /24 interface.
This seems like a bug in dnsmasq, but even if/when it gets fixed
there, it looks like there is no harm in just always adding the
netmask to all IPv4 dhcp-range options similar to how prefix is added
to all IPv6 dhcp-range options.
Signed-off-by: Laine Stump <laine@laine.org>
Reviewed-by: John Ferlan <jferlan@redhat.com>
This fixes a bug that has been present since the original version of
the function was pushed in commit 1ab80f3 on Nov. 26 2010 (by me). The
virSocketAddr::len was not being set.
Apparently until now we were always calling
virSocketAddrPrefixToNetmask with virSocketAddr object that was
already (coincidentally) initialized for the proper address family,
but the bug became apparent when trying to use it to fill in an
otherwise uninitialized object.
Signed-off-by: Laine Stump <laine@laine.org>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Most of the code base is fairly consistent about using the name
'uuidstr' when dealing with a formatted human-readable form, and
'uuid' when dealing with the smaller raw bytes form. Fix
snapshot_conf to comply, as well as reducing the scope of a human
string to only the error message that needs it.
Signed-off-by: Eric Blake <eblake@redhat.com>
Signal the udev thread the change of `priv->threadQuit` by using the
thread condition.
Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
If the udev thread is stopped, it must be ensured that the watch
handle is also removed from the main loop.
Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
The iohelper is an internal program that's only supposed to
be called by libvirt, and whatever output it might produce
will ultimately be passed to virReportError() or similar.
Since we do not want strings passed to those functions to
contain newlines, we can simply not output them in the first
place.
This is what happens in pretty much all cases already, but
in a couple instances newlines have managed to slip in.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Laine Stump <laine@laine.org>
Commit f609cb85 (0.9.5) introduced virDomainSnapshotGetXMLDesc()'s use
of @flags as a subset of virDomainXMLFlags, documenting that 2 of the
3 flags defined at the time would never be valid. Later, commit
28f8dfdc (1.0.0) introduced a new flag, VIR_DOMAIN_XML_MIGRATABLE, but
did not adjust the snapshot documentation to declare it as invalid.
However, since the flag is not accepted as valid by any of the
drivers (remote is just passthrough; esx and vbox don't support flags;
qemu, test, and vz only support VIR_DOMAIN_XML_SECURE), and it is
unlikely that the domain state saved off during a snapshot creation
needs to be migration-friendly (as the snapshot is not the source of
a migration), it is easier to just define an explicit set of supported
flags directly related to the snapshot API rather than trying to
borrow from domain API, and risking confusion if even more domain
flags are added later (in fact, I have an upcoming patch that plans to
add a new flag to virDomainGetXMLDesc that makes no sense for
snapshots).
There is no API or ABI impact (since we purposefully used unsigned int
rather than an enum type in public API, and since the new flag name
carries the same value as the reused name).
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Commit d2a929d4 (0.9.4) defined virDomainSaveImageGetXMLDesc()'s use
of @flags as a subset of virDomainXMLFlags, documenting that 2 of the
3 flags defined at the time would never be valid. Later, commit
28f8dfdc (1.0.0) introduced a new flag, VIR_DOMAIN_XML_MIGRATABLE, but
did not adjust the save image documentation to declare it as invalid.
Later, commit a67e3872 (3.7.0) blindly copied and pasted the same text
into virDomainManagedSaveGetXMLDesc.
However, since the flag is not accepted as valid by any of the
drivers (remote is just passthrough; and qemu is the only supporting
driver for either API, with support for just VIR_DOMAIN_XML_SECURE),
it is easier to just define an explicit set of supported flags
directly related to the save image API rather than trying to borrow
from live domain API, and risking confusion if even more domain flags
are added later (in fact, I have an upcoming patch that plans to add
a new flag to virDomainGetXMLDesc that makes no sense for saved
images). We may someday decide that saved images need to support the
_MIGRATABLE flag, as it is possible to load a saved image with a
different version of libvirt than the one that created it, but that
can be a separate patch if it is ever needed. Meanwhile, it DOES make
sense to reuse the same flags for SaveImage and for ManagedSave (since
ManagedSave is really just sugar for creating a normal SaveImage in a
location controlled by libvirt instead of by the user).
There is no API or ABI impact (since we purposefully used unsigned int
rather than an enum type in public API, and since the new flag name
carries the same value as the old reused name).
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Although VIR_DOMAIN_DEF_FORMAT_INACTIVE and VIR_DOMAIN_XML_INACTIVE
happen to have the same value (1<<1), they come from different enums;
and it is nicer to reason about a 'flags' variable if all uses of
that variable are compared against the same enum type. Messed up in
commit 06f75ff2 (3.8.0).
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Many drivers had a comment that they did not validate the incoming
'flags' to virDomainGetXMLDesc() because they were relying on
virDomainDefFormat() to do it instead. This used to be the case
(at least since 461e0f1a and friends in 0.9.4 added unknown flag
checking in general), but regressed in commit 0ecd6851 (1.2.12),
when all of the drivers were changed to pass 'flags' through the
new helper virDomainDefFormatConvertXMLFlags(). Since this helper
silently ignores unknown flags, we need to implement flag checking
in each driver instead.
Annoyingly, this means that any new flag values added will silently
be ignored when targeting an older libvirt, rather than our usual
practice of loudly diagnosing an unsupported flag. Add comments
in domain_conf.[ch] to remind us to be extra vigilant about the
impact when adding flags (a new flag to add data is safe if the
older server omitting the requested data doesn't break things in
the newer client; a new flag to suppress data rather than enhancing
the existing VIR_DOMAIN_XML_SECURE may form a data leak or even a
security hole).
In the qemu driver, there are multiple callers all funnelling to
qemuDomainDefFormatBufInternal(); many of them already validated
flags (and often only a subset of the full set of possible flags),
but for ease of maintenance, we can also check flags at the common
helper function.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
qemuProcessQMPStart starts a QEMU process and monitor connection that
can be used by multiple functions possibly for multiple QMP commands.
The QMP exchange to exit capabilities negotiation mode and enter command
mode can only be performed once after the monitor connection is
established.
Move responsibility for entering QMP command mode into the
qemuProcessQMP code so multiple functions can issue QMP commands in
arbitrary orders.
This also simplifies the functions using the connection provided by
qemuProcessQMPStart to issue QMP commands.
Test code now needs to call qemuMonitorSetCapabilities to send the
message to switch to command mode because the test code does not use the
qemuProcessQMP command that internally calls qemuMonitorSetCapabilities.
Signed-off-by: Chris Venteicher <cventeic@redhat.com>
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Multiple QEMU processes for QMP commands can operate concurrently.
Use a unique directory under libDir for each QEMU process to avoid
pidfile and unix socket collision between processes.
The pid file name is changed from "capabilities.pidfile" to "qmp.pid"
because we no longer need to avoid a possible clash with a qemu domain
called "capabilities" now that the processes artifacts are stored in
their own unique temporary directories.
"Capabilities" was changed to "qmp" in the pid file name because these
processes are no longer specific to the capabilities usecase and are
more generic in terms of being used for any general purpose QMP message
exchanges with a QEMU process that is not associated with a domain.
Signed-off-by: Chris Venteicher <cventeic@redhat.com>
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Users qemuProcessQMP struct were always forced to call both
qemuProcessQMPStop and qemuProcessQMPFree when they are done with the
process. We can just call qemuProcessQMPStop from qemuProcessQMPFree and
let users call qemuProcessQMPFree only.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
qemuProcessQMPNew is one of the public functions used to create and
manage a QEMU process for QMP command exchanges outside of domain
operations.
Add descriptive comment block, debug statement and make source
consistent with the cleanup / VIR_STEAL_PTR format used elsewhere.
Signed-off-by: Chris Venteicher <cventeic@redhat.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The monitor config data is removed from the qemuProcessQMP struct.
The monitor config data can be initialized immediately before call to
qemuMonitorOpen and does not need to be maintained after the call
because qemuMonitorOpen copies any strings it needs.
Signed-off-by: Chris Venteicher <cventeic@redhat.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Move code for setting paths and prepping file system from
qemuProcessQMPNew to qemuProcessQMPInit.
This keeps qemuProcessQMPNew limited to data structures and path
initialization is done in qemuProcessQMPInit.
The patch is a non-functional, cut / paste change, however goto is now
"cleanup" rather than "error".
Signed-off-by: Chris Venteicher <cventeic@redhat.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Store libDir path in the qemuProcessQMP struct in anticipation of moving
path construction code into qemuProcessQMPInit function.
Signed-off-by: Chris Venteicher <cventeic@redhat.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
All code related to QEMU monitor is moved from qemuProcessQMPNew and
qemuProcessQMPInit into qemuProcessQMPConnectMonitor.
Signed-off-by: Chris Venteicher <cventeic@redhat.com>
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
This is a replacement for qemuProcessQMPRun to make the name consistent
with qemuProcessStart. The original qemuProcessQMPRun function is
renamed as qemuProcessQMPLaunch and becomes one of the simpler functions
called from the main qemuProcessQMPStart entry point. The following
patches will move parts of the code in qemuProcessQMPLaunch to the other
functions (qemuProcessQMPInit and qemuProcessQMPConnectMonitor).
Signed-off-by: Chris Venteicher <cventeic@redhat.com>
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Keep the pointer to QEMU stderr output in qemuProcessQMP struct instead
of requiring the caller to provide it (and free it).
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Let's push the call to virQEMUCapsLogProbeFailure down the stack to
where the probing failure is detected.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
While qemuProcessQMPRun and virQEMUCapsInitQMPMonitor* functions called
from virQEMUCapsInit ignore some errors, the caller of virQEMUCapsInit
would report an error unless usedQMP is true anyway. And since usedQMP
can only be true if the probing code really succeeded (i.e., no errors
were ignored), we can just simplify the logic by not ignoring the errors
in the first place.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The function contains two almost identical parts. Let's consolidate them
into a single helper function and call it twice.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
In new process code, move from model where qemuProcessQMP struct can be
used to activate a series of Qemu processes to model where one
qemuProcessQMP struct is used for one and only one Qemu process.
By allowing only one process activation per qemuProcessQMP struct, the
struct can safely store process outputs like status and stderr, without
being overwritten, until qemuProcessQMPFree is called.
By doing this, process outputs like status and stderr can remain stored
in the qemuProcessQMP struct without being overwritten by subsequent
process activations.
The forceTCG parameter (use / don't use KVM) will be passed when the
qemuProcessQMP struct is initialized since the qemuProcessQMP struct
won't be reused.
Signed-off-by: Chris Venteicher <cventeic@redhat.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
virQEMUCapsInitQMP now stops QEMU process in all execution paths,
before freeing the process structure.
The qemuProcessQMPStop function can be called multiple times without
problems... Won't attempt to stop processes and free resources multiple
times.
Follow the convention established in qemu_process of
1) alloc process structure
2) start process
3) use process
4) stop process
5) free process data structure
The process data structure persists after the process activation fails
or the process dies or is killed so stderr strings can be retrieved
until the process data structure is freed.
Signed-off-by: Chris Venteicher <cventeic@redhat.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
s/qemuProcessQMPAbort/qemuProcessQMPStop/ applied to change function
name used to stop QEMU processes in process code moved from
qemu_capabilities.
No functionality change.
The new name, qemuProcessQMPStop, is consistent with the existing
function qemuProcessStop used to stop Domain processes.
Signed-off-by: Chris Venteicher <cventeic@redhat.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
s/cmd/proc/ in process code imported from qemu_capabilities.
Signed-off-by: Chris Venteicher <cventeic@redhat.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Add the const qualifier on non modified strings
(string only copied inside qemuProcessQMPNew)
so that const strings can be used directly in calls to
qemuProcessQMPNew in future patches.
Signed-off-by: Chris Venteicher <cventeic@redhat.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
QEMU process code in qemu_capabilities.c is moved to qemu_process.c in
order to make the code usable outside the original capabilities use
cases.
The moved code activates and manages QEMU processes without establishing
a guest domain.
This patch is a straight cut/paste move between files.
Signed-off-by: Chris Venteicher <cventeic@redhat.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Only one of the three callers of virPCIDeviceAddressFormat correctly
handles an error return status. Fortunately it can't fail so can be
made void.
Reviewed-by: Laine Stump <laine@laine.org>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
The newline was pretty arbitrary, and we're better off
without it.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
The memory allocated by VIR_REALLOC_N() is uninitialized,
which means it's not possible to figure out whether any
output was produced at all after the fact.
Since we don't care about the previous contents of buffers,
if any, use VIR_FREE() followed by VIR_ALLOC_N() instead.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Remove unused variable. Fix for [1]
[1] 821dd6d8: storage: Use VIR_AUTOFREE for storage backends
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
We dropped support in commit 8e91a40 (November 2015), but some
occurrences still remained, even in live code.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reported-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Now that virStorageSource is a subclass of virObject we can use
virObjectUnref and remove virStorageSourceFree which was a thin wrapper.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Since virStorageSource is now a subclass of virObject, we can use
VIR_AUTOUNREF instead.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Add helper for utilizing __attribute__(cleanup())) for unref-ing
instances of sublasses of virObject.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
To allow tracking a single virStorageSource in multiple structures
without extra hassle allow refcounting by turining it into an object.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Add virStorageSourceNew and refactor places allocating that structure to
use the helper.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Use the proper function to allocate a disk definition.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Now that we've moved all the actual code into helper
functions, we can turn it into a switch statement.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Minor tweaks to ensure compliance with our coding style.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Minor tweaks to ensure compliance with our coding style.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Minor tweaks to ensure compliance with our coding style.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The libvirt zonefile for firewalld (added in commit 3b71f2e4) does the
following:
1) lists specific services it wants to allow, then
2) uses a lower priority <reject/> rule to block all other services to
the host, and then finally,
3) relies on the zone's default "accept" policy to, accept all
forwarded traffic (since forwarded traffic is ignored by the
slightly higher priority <reject/> rule in (2)).
I had assumed that icmp traffic was either being allowed at the top of
the rules, or that it would be ignored by the <reject/> rule and
passed by the default accept policy (similar to forwarded traffic),
but this assumption was incorrect; the <reject/> rule does block icmp
traffic. This became apparent when DHCPv6 which requires ICMPv6 in
addition to udp/dhcpv6) failed to work.
This all means that in order to achieve our original goal of "similar
behavior to a default reject policy, but also allowing forwarded
traffic", we need to add rules to allow all icmp and icmpv6 traffic to
the libvirt zone, and that's what this patch does.
This is a further refinement of the resolution to
https://bugzilla.redhat.com/1650320
Signed-off-by: Laine Stump <laine@laine.org>
Acked-by: Eric Garver <eric@garver.life>
My change in 112f3a8d0f was too drastic. The @charAlias
variable is initialized only if @monitor == true. However, it is
used even outside of that condition, at which point it's just
uninitialized pointer.
Reported-by: John Ferlan <jferlan@redhat.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Even if an error is reported by `udev_enumerate_scan_devices`,
e.g. because a driver of a device has an bug, we can still enumerate
all other devices. Additionally the documentation of
udev_enumerate_scan_devices says that on success an integer >= 0 is
returned (see man udev_enumerate_scan_devices(3)).
Reviewed-by: Bjoern Walk <bwalk@linux.ibm.com>
Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Another misleadingly named macro.
Deprecate in favor of NULLSTR_STAR.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
This macro neither takes nor produces an empty string.
Remove it in favor of NULLSTR_MINUS.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
NULLSTR_EMPTY, the quiet child,
NULLSTR_STAR, the famous one and
NULLSTR_MINUS, the grumpy one.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
The @tmpChr is looked up in domain definition based on user
provided chardev XML. Therefore, the alias must have been
allocated already when domain was started up.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
This is basically an old artefact from 24b0821926 when the idea
was:
1) Build device string only to see if chardev has any -device
associated with it and thus if device_del is needed
2) Detach chardev using chardev_del
Now, that DEVICE and DEVICE_DELETED capabilities are assumed for
every domain 1) does not make sense anymore.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
https://bugzilla.redhat.com/show_bug.cgi?id=1624204
The guestfwd channels are -netdevs really. Hotunplug them as
such. Also, DEVICE_DELETED event is not triggered (surprisingly,
since we're not issuing device_del rather than netdev_del) and
associated chardev is removed automagically too. This means that
we need to do qemuDomainRemoveChrDevice() minus monitor call to
remove the chardev.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
https://bugzilla.redhat.com/show_bug.cgi?id=1624204
The guestfwd channels are -netdevs really. Hotplug them as such.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Introduced by d86c876a66.
There is no real need to have "user-" prefix for chardev.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
So far we are passing @chr to qemuBuildChrDeviceStr. This is
suboptimal (in fact wrong) because @chr is just parsed XML
definition provided by user which by definition may lack some
information. On the other hand, @tmpChr is the one that was found
using @chr in domain definition so it contains the same amount of
information or more.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
The code for creating external snapshots for an offline domain
called out to qemu-img without escaping commas in the manner
that qemu-img expects. This also fixes a typo in the comment.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
commit 3bba4825 added the new function virFirewallDInterfaceSetZone()
which calledsends virDBUSCallMethod a DBusMessage** for the reply
message, but doesn't use the reply, and also doesn't free it. Since
this arg is allowed to be NULL, this patch simply sets it to NULL so
we don't have to deal with it.
Signed-off-by: Laine Stump <laine@laine.org>
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Let's make use of the auto __cleanup capabilities cleaning up any
now unnecessary goto paths.
Signed-off-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
If we find multiple "id=" strings during processing, then we need
to force an error since we cannot have multiple <auth>'s defined
for a single source volume.
Signed-off-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
If virDomainHostdevSubsysSCSIiSCSIDefParseXML processing finds a
duplicated <auth> structure, we should error out rather than continue.
Signed-off-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Modify code to use the VIR_AUTOCLOSE logic cleaning up any
now unnecessary goto paths.
Signed-off-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Rather than overload @ret with trying serve multiple purposes,
let's initialize @ret to -1 and introduce an @rc function return
value that can be used for functions that may return -1 or -2
and only override @ret when rc < 0.
Signed-off-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Let's make use of the auto __cleanup capabilities. This also allows
for the cleanup of some goto paths.
Signed-off-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Rather than having two exit paths, let's use a @retval value
and VIR_STEAL_PTR in order to unite the exit path through the
error label.
Signed-off-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
To prepare for subsequent change to use VIR_AUTOPTR logic rename
the @ret to @def.
Signed-off-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
To prepare for subsequent change to use VIR_AUTOPTR logic rename
the @ret to @def.
Signed-off-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
To prepare for subsequent change to use VIR_AUTOPTR logic rename
the @ret to @def.
Signed-off-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
To prepare for subsequent change to use VIR_AUTOPTR logic rename
the @ret to @def.
Signed-off-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
To prepare for subsequent change to use VIR_AUTOPTR logic rename
the @ret to @def.
Signed-off-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Let's make use of the auto __cleanup capabilities cleaning up any
now unnecessary goto paths.
Signed-off-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Commit 390c06b67 added @xml, but it was never used.
Signed-off-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Rather than have a need for old_dom_name, let's just VIR_FREE
the old name first, then use VIR_STEAL_PTR to handle the swap
from the old name to the new name.
Signed-off-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Let's make use of the auto __cleanup capabilities cleaning up any
now unnecessary goto paths.
Signed-off-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Rather than open coding virStorageFileGetRelativeBackingPath
and virStorageFileGetMetadataRecurse, let's make use of the
VIR_STEAL_PTR macro.
Signed-off-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Let's make use of the auto __cleanup capabilities cleaning up any
now unnecessary goto paths.
Signed-off-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Remove the need for the @name variable by directly assigning
into source->hosts[i].name.
Signed-off-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Let's make use of the auto __cleanup capabilities cleaning up any
now unnecessary goto paths.
Signed-off-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
On error from virAsprintf we would erroneously return 0 with
the @*type not being set. Change to a return -1 on error like
we should have been doing.
Signed-off-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Commit a523770c3 added @retval return processing for
virStorageBackendUpdateVolInfo in order to allow a -2
to be return; however, upon successful completion
@retval = 0 and if either the virStorageBackendSCSISerial
or the virStoragePoolObjAddVol failed, the method would
return 0, but not add the @vol to the pool. So let's
just reset retval = -1 and continue processing.
Signed-off-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Let's make use of the auto __cleanup capabilities cleaning up any
now unnecessary goto paths.
Signed-off-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Rather than initialize to 0 and change to -1 on error, let's do the
normal operation of initializing to -1 and set to 0 on success.
Signed-off-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Let's initialize @path to NULL, then rather than use two labels
free_path and out labels, let's use the cleanup: label to call
VIR_FREE(path); and VIR_FORCE_CLOSE(fd);
Signed-off-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Let's make use of the auto __cleanup capabilities cleaning up any
now unnecessary goto paths.
Signed-off-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Rather than have two error paths, let's use a @retval value and
VIR_STEAL_PTR on @vgname and @pvname to unity the exit path through
the error label.
Signed-off-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
If the virAsprintf of the vol->key fails, then we would erroneously
return the '0' from the @ret from virStorageBackendSheepdogParseVdiList.
So in this error path case, let's set ret = -1.
Signed-off-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Rework the logic to remove the need for the @ok_to_mklabel boolean.
Signed-off-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Let's make use of the auto __cleanup capabilities cleaning up any
now unnecessary goto paths.
Signed-off-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Let's make use of the auto __cleanup capabilities cleaning up any
now unnecessary goto paths.
Signed-off-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Rather than having an error path, let's rework the code to allocate
and fill into an @def variable and then steal that into @ret when we
are successful leaving just a cleanup: path.
Signed-off-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Let's make use of the auto __cleanup capabilities cleaning up any
now unnecessary goto paths.
Signed-off-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Rather than having an error path, let's rework the code to allocate
and fill into an @def variable and then steal that into @ret when we
are successful leaving just a cleanup: path.
Signed-off-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Let's make use of the auto __cleanup capabilities cleaning up any
now unnecessary goto paths.
Signed-off-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Let's make use of the auto __cleanup capabilities cleaning up any
now unnecessary goto paths.
Signed-off-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Rather than having an error path, let's rework the code to allocate
and fill into an @authdef variable and then steal that into @ret when
we are successful leaving just a cleanup: path.
Signed-off-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Since commit a7424faff QMP is always used.
Also, commit 932534e8 removed the last use of this apart from:
* parsing/formatting this in the caps cache
* using it as a temporary variable to know when to report an error
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
No functional change, but this will allow us to mock out the function
in the test suite
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
If 2 threads call abort for example then one of them
will hang because client will send 2 abort messages and
server will reply only on first of them, the second will be
ignored. And on server reply client changes the state only
one of abort message to complete, the second will hang forever.
There are other similar issues.
We should complete all messages waiting reply if we got
error or expected abort/finish reply from server. Also if one
thread send finish and another abort one of them will win
the race and server will either abort or finish stream. If
stream is aborted then thread requested finishing should report
error. In order to archive this let's keep stream closing reason
in @closed field. If we receive VIR_NET_OK message for stream
then stream is finished if oldest (closest to queue end) message
in stream queue is finish message and stream is aborted if oldest
message is abort message. Otherwise it is protocol error.
By the way we need to fix case of receiving VIR_NET_CONTINUE
message. Now we take oldest message in queue and check if
this is dummy message. If one thread first sends abort and
second thread then receives data then oldest message is abort
message and second thread won't be notified when data arrives.
Let's find oldest dummy message instead.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
If we call virStreamFinish and virStreamAbort from 2 distinct
threads for example we can have access to freed memory.
Because when virStreamFinish finishes for example virStreamAbort
yet to be finished and it access virNetClientStreamPtr object
in stream->privateData.
Also it does not make sense to clear @driver field. After
stream is finished/aborted it is better to have appropriate
error message instead of "unsupported error".
This commit reverts [1] or virNetClientStreamPtr and
virStreamPtr will never be unrefed due to cyclic dependency.
Before this patch we don't have leaks because all execution
paths we call virStreamFinish or virStreamAbort.
[1] 8b6ffe40 : virNetClientStreamNew: Track origin stream
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
This mixing errors and EOF condition in one flag is odd.
Instead let's check st->err.code where appropriate.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
Checking virNetClientStreamRaiseError without client lock
is racy which is fixed in [1] for example. Thus let's remove such checks
when we are sending message to server. And in other cases
(like virNetClientStreamRecvHole for example) let's move the check
into client stream code.
virNetClientStreamRecvPacket already have stream lock so we could
introduce another error checking function like virNetClientStreamRaiseErrorLocked
but as error is set when both client and stream lock are hold we
can remove locking from virNetClientStreamRaiseError because all
callers hold either client or stream lock.
Also let's split virNetClientStreamRaiseErrorLocked into checking
state function and checking message send status function. They are
same yet.
[1] 1b6a29c21: rpc: fix race on stream abort/finish and server side abort
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Stream server error is not propagated if thread does not have the buck.
In case we have the buck we are ok due to the code added in [1].
Let's check for stream error on all paths. Now we don't need
to raise error in virNetClientCallDispatchStream.
Old code reported error only if the first message in wait
queue awaits reply. It is odd as depends on wait queue
situation. For example if we have only TX
message in queue and in one iteration loop both send the
message and receive error then thread sending TX message did
not receive the error. Next if we have RX message (first)
and TX message (second) in queue and in one iteration
loop both send the TX message and receive error then
thread sending TX message received error. In short
it was inconsistent. Let's report error whenever
we received it and for every type of message as it makes
sense to report errors as early as possible.
[1] 16c6e2b41: Fix propagation of RPC errors from streams
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
In next patches we'll add stream state checks to this
function that applicable to all call paths. This is handy
place because we hold client lock here.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
Stream abort/finish can hang because we can receive abort message
from server and yet sent abort/finish message to server. The latter
will not be answered ever because after server sends abort message
it forgets the stream and messages for unknown stream are simply ignored.
We check for stream error at the very beginning of remoteStreamFinish/remoteStreamAbort
but stream error can be set after the check in another thread operating
on stream. Let's check for stream error under client lock similar
to what's done in [1].
[1] 833b901cb: stream: Check for stream EOF
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
These functions do mostly the same things, and it would be
preferrable if they did them in mostly the same ways. This
also fixes a few violations to our code style guidelines.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
The function operates on a virDomainDef and is not tied to
device address assignment in any way, so it makes more sense
for it to live along with qemuDomainIs*() and the like.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Ideally we'd make all of them static, but there are a few
cases where we don't have a virDomainDef instance handy and
so they are the only option.
For the few ones we're forced to keep exporting, document
through comments that the alternative is preferred.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Now that we have added architecture checks to all
qemuDomainIs*() functions, we no longer need to perform the
same checks separately.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
There is very little overlap in the machine types available
on different architectures, so broadly speaking checking the
machine type is usually enough; regardless, it's better to
check the architecture as well.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
We want the signatures to be consistent, and also we're
going to start using the additional parameter next.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Make sure related functions, eg. all qemuDomainIs*(), are
close together instead of being sprinkled throughout both
the header and implementation file, and also that all
qemuDomainMachine*() functions are declared first since
we're going to make a bunch of them static later on.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
While the chances of the current checks resulting in false
positives are basically zero, it's still nicer to check for
the full prefix instead of the prefix's prefix.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
For consistency, let's use the semicolon for all definitions.
Signed-off-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
QEMU plans to deprecate 'query-events' as it's non-extensible. Events
are also described by 'query-qmp-schema' so we can use that one instead.
This patch adds detection of events to
virQEMUCapsProbeQMPSchemaCapabilities using the same structure declaring
them for the old approach (virQEMUCapsEvents). This is possible as the
name is the same in the QMP schema and our detector supports that
trivially.
For any complex queries virQEMUCapsQMPSchemaQueries can be used in the
future.
For now we still call 'query-events' and discard the result so that it's
obvious that the tests pass. This will be cleaned up later.
https://bugzilla.redhat.com/show_bug.cgi?id=1673320
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
QEMU accidentally exposed the id of -drive (or same value as disk
serial, if provided) in one of the identifiers visible from the guest.
To avoid regression in case when -blockdev will be used we need to
always specify it ourselves.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
The property allows to control the guest-visible content of the vendor
specific designator of the 'Device Identification' page of a SCSI
device's VPD (vital product data).
QEMU was leaking the id string of -drive as the value if the 'serial' of
the disk was not specified. Switching to -blockdev would impose an ABI
change.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
For SCSI, IDE, and AHCI cdroms the appropriate device types which select
the correct media are used. In qemu there's one other code path that
looks at -drive media=cdrom in the XEN pv code. Thankfully we don't
support it with qemu (see qemuBuildDiskDeviceStr). All other devices
ignore it as the comment states, thus we can drop that code.
The test fallout is expectedly only in the test added for uncommon cdrom
types.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Attempting to create an empty virtio-blk drive results into:
-device virtio-blk-pci,scsi=off,bus=pci.0,addr=0xc,drive=drive-virtio-disk1,id=virtio-disk1: Device needs media, but drive is empty
Attempting to eject media from virtio-blk based drive results into:
error: internal error: unable to execute QEMU command 'eject': Device 'drive-virtio-disk0' is not removable
Forbid configurations where users would attempt to use cdroms in virtio
bus.
Fix few wrong examples which are not really relevant to the tested code.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Cast disk->bus to proper type and add missing values to the enum so it's
more obvious what types are supported.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
The split of ide-disk into the two separate devices was introduced by
qemu commit 1f56e32a7f4b3 released in qemu v0.15.
Note that when compared to the previous commit which made sure that no
disk related tests were touched, in this case it's not as careful.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
The split of scsi-disk into the two separate devices was introduced by
qemu commit b443ae67 released in qemu v0.15.
All changes to test files are not really related to disk testing thanks
to previous refactors.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Since commit a4cda054e7 we are using 'ide-hd' and 'ide-cd' instead of
'ide-drive'. We also should probe capabilities for 'ide-hd' instead of
'ide-drive'. It is safe to do as 'ide-drive' is the common denominator
of both 'ide-hd' and 'ide-cd' so all the properties were common.
For now the test data are modified by just changing the appropriate type
when probing for caps.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Since commit 02e8d0cfdf we are using 'scsi-hd' and 'scsi-cd' instead of
'scsi-disk'. We also should probe capabilities for 'scsi-hd' instead of
'scsi-disk'. It is safe to do as 'scsi-disk' is the common denominator
of both 'scsi-hd' and 'scsi-cd' so all the properties were common.
For now the test data are modified by just changing the appropriate type
when probing for caps.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
This flag tells virDomainMigrateSetMaxSpeed and
virDomainMigrateGetMaxSpeed APIs to work on post-copy migration
bandwidth.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
This typed parameter for virDomainMigrate3 and virDomainMigrateToURI3
APIs may be used for setting maximum post-copy migration bandwidth.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
This patch adds a new VIR_MIGRATE_PARAM_BANDWIDTH_POSTCOPY typed
parameter for virDomainMigrate3 and virDomainMigrateToURI3 for setting
maximum post-copy migration bandwidth.
In case the initial VIR_MIGRATE_PARAM_BANDWIDTH_POSTCOPY value turns out
to be suboptimal a new VIR_DOMAIN_MIGRATE_MAX_SPEED_POSTCOPY flag for
virDomainMigrateSetMaxSpeed and virDomainMigrateGetMaxSpeed may be used
to set/get the maximum post-copy migration bandwidth while migration is
already running.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
So far migration parameters were changed only at the beginning of
migration mostly via an automatic translation from flags and typed
parameters. We need to export a few more functions to support APIs which
may set migration parameters while migration is already running.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Let's make the code flow easier to follow and get rid of the ugly endjob
label inside if branch.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Some migration parameters supported by libvirt may use units that differ
from the units used by QEMU for the corresponding parameters. For
example, libvirt defines migration bandwidth in MiB/s while QEMU expects
B/s. Let's add a unit field to qemuMigrationParamsTPMapItem for
automatic conversion when translating between libvirt's migration typed
parameters and QEMU's migration paramteres.
This patch is a preparation for future parameters as the existing
VIR_MIGRATE_PARAM_BANDWIDTH parameter is set using "migrate_set_speed"
QMP command rather than "migrate-set-parameters" for backward
compatibility.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
qemuDomainBlockPivot and qemuDomainBlockJobAbort need the job name for
cancelling or pivoting but were generating it locally instead of
accessing the existing copy in the job data structure.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The writing to an image actually starts when the copy job is initiated,
so checking this at the time of the pivot operation is too late.
Move the check to qemuDomainBlockCopyCommon. Note that modern qemu would
have prevented two writers with qcow2 so the slim possibility of a job
started with libvirtd without this patch missing the check is not really
worth worrying about.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
For copy and active commit jobs we record the state of the mirror so
that we can recover. The status XML was not saved in case of
qemuDomainBlockPivot due to an oversight.
Save the XML always when invoking qemuDomainBlockJobAbort even if
the job is not currently tracking any state. This will change later and
also this is not a particularly hot code path.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
If the container is really a simple one (init is just bash and
the whole root is passed through) then virDomainReboot and
virDomainShutdown will talk to the actual init within the host.
Therefore, 'virsh shutdown $dom' will result in shutting down the
host. True, at that point the container is shut down too but
looks a bit harsh to me.
The solution is to check if the init inside the container is or
is not the same as the init running on the host.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
So far the virInitctlSetRunLevel() is fully automatic. It finds
the correct fifo to use to talk to the init and it will set the
desired runlevel. Well, callers (so far there is just one) will
need to inspect the fifo a bit just before the runlevel is set.
Therefore, expose the internal list of fifos and also allow
caller to explicitly use one.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Due to a bug the seclabels are restored before any PID in the
container is killed. This should be done afterwards in
virLXCProcessCleanup.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Prior to rewrite of cgroup code we only had one backend to try.
After the rewrite the virCgroupBackendGetAll() returns both
backends (for v1 and v2). However, not both have to really be
present on the system which results in killRecursive callback
failing which in turn might mean we won't try the other backend.
At the same time, this function reports no error as it should.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Not that it would matter because LXC driver doesn't differentiate
the job types so far, but nevertheless the Destroy() should grab
LXC_JOB_DESTROY.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
The number of iothreads is not part of the vm state sent during
migration, nor exposed to the guest ABI, so this restriction is
a mistake in libvirt. Let's remove that bit of code.
Reviewed-by: Cole Robinson <crobinso@redhat.com>
Signed-off-by: Jie Wang <wangjie88@huawei.com>
The checks and error messages are mostly the same across
all virtio-input devices, so we can avoid having multiple
copies of the same code.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
It will not work. This breaks qemu capabilities probing as a user.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
For normal starts (no incoming migration) the refresh of the QEMU
state must be done before the VCPUs getting started since otherwise
there might be a race condition between a possible shutdown of the
guest OS and the QEMU monitor queries.
This fixes "qemu: migration: Refresh device information after
transferring state" (93db7eea1b).
Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
If a domain has a disk that is type='network' we require specific
cache mode to allow migration with it (either 'directsync' or
'none'). This doesn't make much sense since network disks are
supposed to be safe to migrate by default.
At the same time, we should be checking for the actual source
type, not apparent type set in the domain XML.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Storage pools might want to specify format of the image when translating
the volume thus we can't add any default format when parsing the XML.
Add a explicit format when starting the VM and format is not present
neither by user specifying it nor by the storage pool translation
function.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Post parse callback adds the 'raw' type only for local files. Remote
files can also have backing store (even local) so we should do this also
for network backed storage.
Note that virStorageFileGetMetadata always considers files with no type
as raw so we will not accidentally traverse the backing chain and allow
unexpected files being labelled with svirt labels.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
In commit f80eae8c2a I was too agresive in removing properties of
-drive for empty drives. It turns out that qemu actually persists the
state of 'readonly' and the throttling information even for the empty
drive.
Removing 'readonly' thus made qemu open any subsequent images added via
the 'change' command as RW which was forbidden by selinux thanks to the
restrictive sVirt label for readonly media.
Fix this by formating the property again and bump the tests and leave a
note detailing why the rest of the properties needs to be skipped.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Missing semicolon at the end of macros can confuse some analyzers
(like cppcheck <filename>). VIR_ONCE_GLOBAL_INIT is almost
exclusively called without an ending semicolon, but let's
standardize on using one like the other macros.
Add a dummy struct definition at the end of the macro, so
the compiler will require callers to add a semicolon.
Reviewed-by: John Ferlan <jferlan@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
Missing semicolon at the end of macros can confuse some analyzers
(like cppcheck <filename>), and we have a mix of semicolon and
non-semicolon usage through the code. Let's standardize on using
a semicolon for VIR_LOG_INIT calls.
Drop the semicolon from the final statement of the macro, so
the compiler will require callers to add a semicolon.
Reviewed-by: John Ferlan <jferlan@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
Missing semicolon at the end of macros can confuse some analyzers
(like cppcheck <filename>), and we have a mix of semicolon and
non-semicolon usage through the code. Let's standardize on using
a semicolon for VIR_ENUM_IMPL calls.
Move the verify() statement to the end of the macro and drop
the semicolon, so the compiler will require callers to add a
semicolon.
While we are touching these call sites, standardize on putting
the closing parenth on its own line, as discussed here:
https://www.redhat.com/archives/libvir-list/2019-January/msg00750.html
Reviewed-by: John Ferlan <jferlan@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
Missing semicolon at the end of macros can confuse some analyzers
(like cppcheck <filename>), and we have a mix of semicolon and
non-semicolon usage through the code. Let's standardize on using
a semicolon for VIR_ENUM_DECL calls.
Drop the semicolon from the final statement of the macro, so
the compiler will require callers to add a semicolon.
Reviewed-by: John Ferlan <jferlan@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
Just before pushing the series containing commit 3bba4825 I had added
a "return true" to the top of virFirewallDZoneExists() to measure the
impact of calling that function once per network during startup. I
found that the effect was minimal, but forgot to remove the "return
true" before pushing. This unfortunately causes a failure to start
networks on systems that have a firewalld version that doesn't support
our libvirt zone file (i.e. pretty much everyone).
This patch removes the unintended line.
Signed-off-by: Laine Stump <laine@laine.org>
When using custom command line arguments, warn that
this configuration is not fully supported.
Signed-off-by: Roman Bogorodskiy <bogorodskiy@gmail.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
- Remove ATTRIBUTE_UNUSED for the "buf" argument, it's
not unused
- Indent fix
Signed-off-by: Roman Bogorodskiy <bogorodskiy@gmail.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Since we're setting the zone anyway, it will be useful to allow
setting a different (custom) zone for each network. This will be done
by adding a "zone" attribute to the "bridge" element, e.g.:
...
<bridge name='virbr0' zone='myzone'/>
...
If a zone is specified in the config and it can't be honored, this
will be an error.
Signed-off-by: Laine Stump <laine@laine.org>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
This patch restores broken guest network connectivity after a host
firewalld is switched to using an nftables backend. It does this by
adding libvirt networks' bridge interfaces to the new "libvirt" zone
in firewalld.
After this patch, the bridge interface of any network created by
libvirt (when firewalld is active) will be added to the firewalld
zone called "libvirt" if it exists (regardless of the firewalld
backend setting). This behavior does *not* depend on whether or not
libvirt has installed the libvirt zone file (set with
"--with[out]-firewalld-zone" during the configure phase of the package
build).
If the libvirt zone doesn't exist (either because the package was
configured to not install it, or possibly it was installed, but
firewalld doesn't support rule priorities, resulting in a parse
error), the bridge will remain in firewalld's default zone, which
could be innocuous (in the case that the firewalld backend is
iptables, guest networking will still function properly with the
bridge in the default zone), or it could be disastrous (if the
firewalld backend is nftables, we can be assured that guest networking
will fail). In order to be unobtrusive in the former case, and
informative in the latter, when the libvirt zone doesn't exist we
then check the firewalld version to see if it's new enough to support
the nftables backend, and then if the backend is actually set to
nftables, before logging an error (and failing the net-start
operation, since the network couldn't possibly work anyway).
When the libvirt zone is used, network behavior is *slightly*
different from behavior of previous libvirt. In the past, libvirt
network behavior would be affected by the configuration of firewalld's
default zone (usually "public"), but now it is affected only by the
"libvirt" zone), and thus almost surely warrants a release note for
any distro upgrading to libvirt 5.1 or above. Although it's
unfortunate that we have to deal with a mandatory behavior change, the
architecture of multiple hooks makes it impossible to *not* change
behavior in some way, and the new behavior is arguably better (since
it will now be possible to manage access to the host from virtual
machines vs from public interfaces separately).
Creates-and-Resolves: https://bugzilla.redhat.com/1650320
Resolves: https://bugzilla.redhat.com/1638342
Signed-off-by: Laine Stump <laine@laine.org>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
In the past (when both libvirt and firewalld used iptables), if either
libvirt's rules *OR* firewalld's rules accepted a packet, it would
be accepted. This was because libvirt and firewalld rules were
processed during the same kernel hook, and a single ACCEPT result
would terminate the rule traversal and cause the packet to be
accepted.
But now firewalld can use nftables for its backend, while libvirt's
firewall rules are still using iptables; iptables rules are still
processed, but at a different time during packet processing
(i.e. during a different hook) than the firewalld nftables rules. The
result is that a packet must be accepted by *BOTH* the libvirt
iptables rules *AND* the firewalld nftable rules in order to be
accepted.
This causes pain because
1) libvirt always adds rules to permit DNS and DHCP (and sometimes
TFTP) from guests to the host network's bridge interface. But
libvirt's bridges are in firewalld's "default" zone (which is usually
the zone called "public"). The public zone allows ssh, but doesn't
allow DNS, DHCP, or TFTP. So even though libvirt's rules allow the
DHCP and DNS traffic, the firewalld rules (now processed during a
different hook) dont, thus guests connected to libvirt's bridges can't
acquire an IP address from DHCP, nor can they make DNS queries to the
DNS server libvirt has setup on the host. (This could be solved by
modifying the default firewalld zone to allow DNS and DHCP, but that
would open *all* interfaces in the default zone to those services,
which is most likely not what the host's admin wants.)
2) Even though libvirt adds iptables rules to allow forwarded traffic
to pass the iptables hook, firewalld's higher level "rich rules" don't
yet have the ability to configure the acceptance of forwarded traffic
(traffic that is going somewhere beyond the host), so any traffic that
needs to be forwarded from guests to the network beyond the host is
rejected during the nftables hook by the default zone's "default
reject" policy (which rejects all traffic in the zone not specifically
allowed by the rules in the zone, whether that traffic is destined to
be forwarded or locally received by the host).
libvirt can't send "direct" nftables rules (firewalld only supports
direct/passthrough rules for iptables), so we can't solve this problem
by just sending explicit nftables rules instead of explicit iptables
rules (which, if it could be done, would place libvirt's rules in the
same hook as firewalld's native rules, and thus eliminate the need for
packets to be accepted by both libvirt's and firewalld's own rules).
However, we can take advantage of a quirk in firewalld zones that have
a default policy of "accept" (meaning any packet that doesn't match a
specific rule in the zone will be *accepted*) - this default accept will
also accept forwarded traffic (not just traffic destined for the host).
Of course we don't want to modify firewalld's default zone in that
way, because that would affect the filtering of traffic coming into
the host from other interfaces using that zone. Instead, we will
create a new zone called "libvirt". The libvirt zone will have a
default policy of accept so that forwarded traffic can pass and list
specific services that will be allowed into the host from guests (DNS,
DHCP, SSH, and TFTP).
But the same default accept policy that fixes forwarded traffic also
causes *all* traffic from guest to host to be accepted. To close this
new hole, the libvirt zone can take advantage of a new feature in
firewalld (currently slated for firewalld-0.7.0) - priorities for rich
rules - to add a low priority rule that rejects all local traffic (but
leaves alone all forwarded traffic).
So, our new zone will start with a list of services that are allowed
(dhcp, dns, tftp, and ssh to start, but configurable via any firewalld
management application, or direct editing of the zone file in
/etc/firewalld/zones/libvirt.xml), followed by a low priority
<reject/> rule (to reject all other traffic from guest to host), and
finally with a default policy of accept (to allow forwarded traffic).
This patch only creates the zonefile for the new zone, and implements
a configure.ac option to selectively enable/disable installation of
the new zone. A separate patch contains the necessary code to actually
place bridge interfaces in the libvirt zone.
Why do we need a configure option to disable installation of the new
libvirt zone? It uses a new firewalld attribute that sets the priority
of a rich rule; this feature first appears in firewalld-0.7.0 (unless
it has been backported to am earlier firewalld by a downstream
maintainer). If the file were installed on a system with firewalld
that didn't support rule priorities, firewalld would log an error
every time it restarted, causing confusion and lots of extra bug
reports.
So we add two new configure.ac switches to avoid polluting the system
logs with this error on systems that don't support rule priorities -
"--with-firewalld-zone" and "--without-firewalld-zone". A package
builder can use these to include/exclude the libvirt zone file in the
installation. If firewalld is enabled (--with-firewalld), the default
is --with-firewalld-zone, but it can be disabled during configure
(using --without-firewalld-zone). Targets that are using a firewalld
version too old to support the rule priority setting in the libvirt
zone file can simply add --without-firewalld-zone to their configure
commandline.
These switches only affect whether or not the libvirt zone file is
*installed* in /usr/lib/firewalld/zones, but have no effect on whether
or not libvirt looks for a zone called libvirt and tries to use it.
NB: firewalld zones can only be added to the permanent config of
firewalld, and won't be loaded/enabled until firewalld is restarted,
so at package install/upgrade time we have to restart firewalld. For
rpm-based distros, this is done in the libvirt.spec file by calling
the %firewalld_restart rpm macro, which is a part of the
firewalld-filesystem package. (For distros that don't use rpm
packages, the command "firewalld-cmd --reload" will have the same
effect).
Signed-off-by: Laine Stump <laine@laine.org>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
virFirewallDGetBackend() reports whether firewalld is currently using
an iptables or an nftables backend.
virFirewallDGetVersion() learns the version of the firewalld running
on this system and returns it as 1000000*major + 1000*minor + micro.
virFirewallDGetZones() gets a list of all currently active firewalld
zones.
virFirewallDInterfaceSetZone() sets the firewalld zone of the given
interface.
virFirewallDZoneExists() can be used to learn whether or not a
particular zone is present and active in firewalld.
Signed-off-by: Laine Stump <laine@laine.org>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
In preparation for adding several other firewalld-specific functions,
separate the code that's unique to firewalld from the more-generic
"firewall" file.
Signed-off-by: Laine Stump <laine@laine.org>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Support for firewalld is a feature that can be selectively enabled or
disabled (using --with-firewalld/--without-firewalld), not merely
something that must be accounted for in the code if it is present with
no exceptions. It is more consistent with other usage in libvirt to
use WITH_FIREWALLD rather than HAVE_FIREWALLD.
Signed-off-by: Laine Stump <laine@laine.org>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
https://bugzilla.redhat.com/show_bug.cgi?id=1657468
Commit be1bb6c95 changed the way volumes were stored from a forward
linked list to a hash table. In doing so, it required that each vol
object would have 3 unique values as keys into tables - key, name,
and path. Due to how vHBA/NPIV LUNs are created/used this resulted
in a failure to utilize all the LUN's found during processing.
During virStorageBackendSCSINewLun processing fetch the key (or
serial value) for NPIV LUN's using virStorageFileGetNPIVKey which
will formulate a more unique key based on the serial value and
the port for the LUN.
Signed-off-by: John Ferlan <jferlan@redhat.com>
ACKed-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The vHBA/NPIV LUNs created via the udev processing of the
VPORT_CREATE command end up using the same serial value
as seen/generated by the /lib/udev/scsi_id as returned
during virStorageFileGetSCSIKey. Therefore, in order to
generate a unique enough key to be used when adding the
LUN as a volume during virStoragePoolObjAddVol a more
unique key needs to be generated for an NPIV volume.
The problem is illustrated by the following example, where
scsi_host5 is a vHBA used with the following LUNs:
$ lsscsi -tg
...
[5:0:4:0] disk fc:0x5006016844602198,0x101f00 /dev/sdh /dev/sg23
[5:0:5:0] disk fc:0x5006016044602198,0x102000 /dev/sdi /dev/sg24
...
Calling virStorageFileGetSCSIKey would return:
/lib/udev/scsi_id --device /dev/sdh --whitelisted --replace-whitespace /dev/sdh
350060160c460219850060160c4602198
/lib/udev/scsi_id --device /dev/sdh --whitelisted --replace-whitespace /dev/sdi
350060160c460219850060160c4602198
Note that althrough /dev/sdh and /dev/sdi are separate LUNs, they
end up with the same serial number used for the vol->key value.
When virStoragePoolFCRefreshThread calls virStoragePoolObjAddVol
the second LUN fails to be added with the following message
getting logged:
virHashAddOrUpdateEntry:341 : internal error: Duplicate key
To resolve this, virStorageFileGetNPIVKey will use a similar call
sequence as virStorageFileGetSCSIKey, except that it will add the
"--export" option to the call. This results in more detailed output
which needs to be parsed in order to formulate a unique enough key
to be used. In order to be unique enough, the returned value will
concatenate the target port as returned in the "ID_TARGET_PORT"
field from the command to the "ID_SERIAL" value.
Signed-off-by: John Ferlan <jferlan@redhat.com>
ACKed-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Alter the code to use the virStorageFileGetSCSIKey helper
to fetch the unique key for the SCSI disk. Alter the logic
to follow the former code which would return a duplicate
of @dev when either the virCommandRun succeeded, but returned
an empty string or when WITH_UDEV was not true.
Signed-off-by: John Ferlan <jferlan@redhat.com>
ACKed-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Alter the "real" code to return -2 on virCommandRun failure.
Alter the comments and function header to describe the function
and its returns.
Signed-off-by: John Ferlan <jferlan@redhat.com>
ACKed-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
https://bugzilla.redhat.com/show_bug.cgi?id=1503284
The way we currently start qemu from CPU affinity POV is as
follows:
1) the child process is set affinity to all online CPUs (unless
some vcpu pinning was given in the domain XML)
2) Once qemu is running, cpuset cgroup is configured taking
memory pinning into account
Problem is that we let qemu allocate its memory just anywhere in
1) and then rely in 2) to be able to move the memory to
configured NUMA nodes. This might not be always possible (e.g.
qemu might lock some parts of its memory) and is very suboptimal
(copying large memory between NUMA nodes takes significant amount
of time).
The solution is to set affinity to one of (in priority order):
- The CPUs associated with NUMA memory affinity mask
- The CPUs associated with emulator pinning
- All online host CPUs
Later (once QEMU has allocated its memory) we then change this
again to (again in priority order):
- The CPUs associated with emulator pinning
- The CPUs returned by numad
- The CPUs associated with vCPU pinning
- All online host CPUs
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
This is mainly about /dev/sev and its default permissions 0600. Of
course, rule of 'tinfoil' would be that we can't trust anything, but the
probing code in QEMU is considered safe from security's perspective + we
can't create an udev rule for this at the moment, because ioctls and
file system permissions aren't cross-checked in kernel and therefore a
user with read permissions could issue a 'privileged' operation on SEV
which is currently only limited to root.
https://bugzilla.redhat.com/show_bug.cgi?id=1665400
Signed-off-by: Erik Skultety <eskultet@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
The default permissions (0600 root:root) are of no use to the qemu
process so we need to change the owner to qemu iff running with
namespaces.
Signed-off-by: Erik Skultety <eskultet@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Instead of exposing /dev/sev to every domain, do it selectively.
Signed-off-by: Erik Skultety <eskultet@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
SEV has a limit on number of concurrent guests. From security POV we
should only expose resources (any resources for that matter) to domains
that truly need them.
Signed-off-by: Erik Skultety <eskultet@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
We should not give domains access to something they don't necessarily
need by default. Remove it from the qemu driver docs too.
Signed-off-by: Erik Skultety <eskultet@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
virtio-mmio is still used by default, so if PCI is desired
it's necessary to explicitly opt-in by adding an appropriate
<address type='pci' domain='0x0000' ... />
element to the corresponding device.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
This adds an additional directive to the dnsmasq configuration file that
notifies clients via dhcp about the link's MTU. Guests can then choose
adjust their link accordingly.
Signed-off-by: Casey Callendrello <cdc@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Signed-off-by: Ján Tomko <jtomko@redhat.com>
The 'qemu' binary used to provide the i386 emulator until it was renamed
to qemu-system-i386 in QEMU 1.0. Since we don't support such old
versions we don't need to check for 'qemu' when probing capabilities.
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
The custom namespaces were originally registered against the storage
pool source struct, but during review this was changed to the top level
storage pool struct. The namespace URIs were not updated to match, so
had a redundant '/source' component.
Reviewed-by: John Ferlan <jferlan@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Some clients poll virDomainGetBlockJobInfo rather than wait for the
VIR_DOMAIN_BLOCK_JOB_READY event. In some cases qemu can get to 100% and
still not reach the synchronised phase. Initiating a pivot in that case
will fail.
Given that computers are interacting here, the error that the job
can't be finalized yet is not handled very well by those specific
implementations.
Our docs now correctly state to use the event. We already do a similar
output adjustment in case when the progress is not available from qemu
as in that case we'd report 0 out of 0, which some apps also incorrectly
considered as 100% complete.
In this case we subtract 1 from the progress if the ready state is not
signalled by qemu if the progress was at 100% otherwise.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
A copy+paste mistaken meant the wrong enum -> string convertor
function was used for the error when an incorrect feature capability was
used.
Reviewed-by: John Ferlan <jferlan@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
PEP 8 says:
"Comparisons to singletons like None should always be done
with 'is' or 'is not', never the equality operators."
There are potentially semantics differences, though in the case of this
libvirt code its merely a style change:
http://jaredgrubb.blogspot.com/2009/04/python-is-none-vs-none.html
Reviewed-by: John Ferlan <jferlan@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
The virDomainDeviceInfo parameter is a large struct so it is preferrable
to pass it by reference instead of by value.
Reviewed-by: John Ferlan <jferlan@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
