John Ferlan [Fri, 12 Sep 2014 12:16:07 +0000 (08:16 -0400)]
Resolve Coverity CHECKED_RETURN
Coverity complained that checking the return of virDomainCreate()
was not consistent amongst the callers - so added the return check
to the objecteventtest.c and adjust the virt-login-shell to compare
< 0 rather than just non zero for the failure condition.
John Ferlan [Fri, 12 Sep 2014 12:04:28 +0000 (08:04 -0400)]
virsh: Resolve Coverity DEADCODE
Coverity complains that on the first pass through the for loop that
'params' cannot be true, thus the ternary setting to "&" cannot be
done. Since we can only ever get to this point once, drop the ternary
John Ferlan [Fri, 12 Sep 2014 11:52:39 +0000 (07:52 -0400)]
domain_conf: Resolve Coverity COPY_PASTE_ERROR
Seems when commit id 'ea130e3b' added the checks to ensure each of
the hard_limit, soft_limit, and swap_hard_limit wasn't set at
VIR_DOMAIN_MEMORY_PARAM_UNLIMITED - a copy/paste error of using
the 'hard_limit' for each comparison was done. Adjust the code.
John Ferlan [Thu, 11 Sep 2014 21:29:18 +0000 (17:29 -0400)]
virtime: Resolve Coverity DEADCODE
Coverity complains that because of how 'offset' is initialized to
0 (zero), the resulting math and comparison on rem is pointless.
According to the origin commit id '3ec128989', the code is a
replacement for gmtime(), but without the localtime() or GMT
calculations - so just remove this code and add a comment
indicating the removal
This was a false positive where Coverity was complaining that the
remoteDeserializeTypedParameters() could allocate 'params', but
none of the callers could return the allocated memory back to their
caller since on input the param was passed by value. Additionally,
the flow of the code was that if params was NULL on entry, then each
function would return 'nparams' as the number of params entries the
caller would need to allocate in order to call the function again
with 'nparams' and 'params' being set. By the time the deserialize
routine was called params would have something. For other callers
where the 'params' was passed by reference as NULL since it's expected
that the deserialize allocates the memory and then have that passed
back to the original caller to dispose there was no Coverity issue.
As it turns out Coverity didn't quite seem to understand the
relationship between 'nparams' and 'params'; however, if the
!userAllocated path of the deserialize code compared against
limit in any manner, then the Coverity error went away which
was quite strange, but useful.
As it turns out one code path remoteDomainGetJobStats had a
comparison against 'limit' while another remoteConnectGetAllDomainStats
did not assuming that limit would be checked. So I refactored the
code a bit to cause the limit check to occur in deserialize for
both conditions and then only made the check of current returned
size against the incoming *nparams fail the non allocation case.
This means the job code doesn't need to check the limit any more,
while the stats code now does check the limit.
Additionally, to help perhaps decipher which of the various
callers to the deserialize code caused the failure - I used
a #define to pass the __FUNCNAME__ of the caller along so that
error messages could have something like:
error: remoteConnectGetAllDomainStats: too many parameters '2' for nparams '0'
error: Reconnected to the hypervisor
(it's a contrived error just to show the funcname in the error)
node_device_udev: Try harder to get human readable vendor:product
The manufacurer and product from USB device itself are usually not particularly
useful -- they tend to be missing, or ugly (all-uppercase, padded with spaces,
etc.). Prefer what's in the usb id database and fall back to descriptors only
if the device is too new to be in database.
Laine Stump [Wed, 10 Sep 2014 17:10:45 +0000 (13:10 -0400)]
network: try to eliminate default network conflict during package install
Sometimes libvirt is installed on a host that is already using the
network 192.168.122.0/24. If the libvirt-daemon-config-network package
is installed, this creates a conflict, since that package has been
hard-coded to create a virtual network that also uses
192.168.122.0/24. In the past libvirt has attempted to warn of /
remediate this situation by checking for conflicting routes when the
network is started, but it turns out that isn't always useful (for
example in the case that the *other* interface/network creating the
conflict hasn't yet been started at the time libvirtd start its own
networks).
This patch attempts to catch the problem earlier - at install
time. During the %post install script for
libvirt-daemon-config-network, we use a case statement to look through
the output of "ip route show" for a route that exactly matches
192.168.122.0/24, and if found we search for a similar route that
*doesn't* match (e.g. 192.168.124.0/24) (note that the search starts
with "124" instead of 123 because of reports of people already
modifying their L1 host's network to 192.168.123.0/24 in an attempt to
solve exactly the problem we are also trying to solve). When we find
an available route, we just replace all occurrences of "122" in the
default.xml that is being created with the newly found 192.168
subnet. This could obviously be made more complicated - examine the
template defaul.xml to automatically determine the existing network
address and mask rather than hard coding it in the specfile, etc, but
this scripting is simpler and gets the job done as long as we continue
to use 192.168.122.0/24 in the template. (If anyone with mad bash
skillz wants to suggest something to do that, by all means please do).
This is intended to at least "further reduce" occurrence of the
problems detailed in:
Eric Blake [Sun, 31 Aug 2014 03:56:19 +0000 (21:56 -0600)]
blockjob: allow finer bandwidth tuning for set speed
We stupidly modeled block job bandwidth after migration
bandwidth, which in turn was an 'unsigned long' and therefore
subject to 32-bit vs. 64-bit interpretations. To work around
the fact that 10-gigabit interfaces are possible but don't fit
within 32 bits, the original interface took the number scaled
as MiB/sec. But this scaling is rather coarse, and it might
be nice to tune bandwidth finer than in megabyte chunks.
Several of the block job calls that can set speed are fed
through a common interface, so it was easier to adjust them all
at once. Note that there is intentionally no flag for the new
virDomainBlockCopy; there, since the API already uses a 64-bit
type always, instead of a possible 32-bit type, and is brand
new, it was easier to just avoid scaling issues. As with the
previous patch that adjusted the query side (commit db33cc24),
omitting the new flag preserves old behavior, and the
documentation now mentions limits of what happens when a 32-bit
machine is on either client or server side.
* include/libvirt/libvirt.h.in (virDomainBlockJobSetSpeedFlags)
(virDomainBlockPullFlags)
(VIR_DOMAIN_BLOCK_REBASE_BANDWIDTH_BYTES)
(VIR_DOMAIN_BLOCK_COMMIT_BANDWIDTH_BYTES): New enums.
* src/libvirt.c (virDomainBlockJobSetSpeed, virDomainBlockPull)
(virDomainBlockRebase, virDomainBlockCommit): Document them.
* src/qemu/qemu_driver.c (qemuDomainBlockJobSetSpeed)
(qemuDomainBlockPull, qemuDomainBlockRebase)
(qemuDomainBlockCommit, qemuDomainBlockJobImpl): Support new flag.
Eric Blake [Mon, 8 Sep 2014 20:53:12 +0000 (14:53 -0600)]
blockcopy: add qemu implementation of new tunables
Upstream qemu 1.4 added some drive-mirror tunables not present
when it was first introduced in 1.3. Management apps may want
to set these in some cases (for example, without tuning
granularity down to sector size, a copy may end up occupying
more bytes than the original because an entire cluster is
copied even when only a sector within the cluster is dirty,
although tuning it down results in more CPU time to do the
copy). I haven't personally needed to use the parameters, but
since they exist, and since the new API supports virTypedParams,
we might as well expose them.
Since the tuning parameters aren't often used, and omitted from
the QMP command when unspecified, I think it is safe to rely on
qemu 1.3 to issue an error about them being unsupported, rather
than trying to create a new capability bit in libvirt.
Meanwhile, all versions of qemu from 1.4 to 2.1 have a bug where
a bad granularity (such as non-power-of-2) gives a poor message:
error: internal error: unable to execute QEMU command 'drive-mirror': Invalid parameter 'drive-virtio-disk0'
because of abuse of QERR_INVALID_PARAMETER (which is supposed to
name the parameter that was given a bad value, rather than the
value passed to some other parameter). I don't see that a
capability check will help, so we'll just live with it (and it
has since been improved in upstream qemu).
Eric Blake [Mon, 25 Aug 2014 21:11:05 +0000 (15:11 -0600)]
blockcopy: add qemu implementation of new API
The hard part of managing the disk copy is already coded; all
this had to do was convert the XML and virTypedParameters into
the internal representation.
With this patch, all blockcopy operations that used the old
API should also work via the new API. Additional extensions,
such as supporting the granularity tunable or a network rather
than file destination, will be added as later patches.
* src/qemu/qemu_driver.c (qemuDomainBlockCopy): New function.
Eric Blake [Fri, 29 Aug 2014 22:30:46 +0000 (16:30 -0600)]
blockcopy: tweak how rebase calls into copy
In order to implement the new virDomainBlockCopy, the existing
block copy internal implementation needs to be adjusted. The
new function will parse XML into a storage source, and parse
typed parameters into integers, then call into the same common
backend. For now, it's easier to keep the same implementation
limits that only local file destinations are suported, but now
the check needs to be explicit. Similar to qemuDomainBlockJobImpl
consuming 'vm', this code also consumes the caller's 'mirror'
description of the destination.
Michal Privoznik [Fri, 12 Sep 2014 11:18:32 +0000 (13:18 +0200)]
formatdomain: Update <loader/> example to match the rest
At the beginning when I was inventing <loader/> attributes and
<nvram/> I've introduced this @readonly attribute to the loader
element. It accepted values 'on' and 'off'. However, later, during the
review process, that has changed to 'yes' and 'no', but the example
XML snippet wasn't updated, so while the description is correct, the
example isn't.
Reported-by: Laszlo Ersek <lersek@redhat.com> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Michal Privoznik [Thu, 11 Sep 2014 11:17:11 +0000 (13:17 +0200)]
virDomainUndefineFlags: Allow NVRAM unlinking
When a domain is undefined, there are options to remove it's
managed save state or snapshots. However, there's another file
that libvirt creates per domain: the NVRAM variable store file.
Make sure that the file is not left behind if the domain is
undefined.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
John Ferlan [Thu, 11 Sep 2014 21:45:04 +0000 (17:45 -0400)]
qemu: Resolve Coverity FORWARD_NULL
If we end up at the cleanup lable before we've VIR_EXPAND_N the list,
then calling virQEMUCapsFreeStringList() with a NULL proplist could
theoretically deref proplist if nproplist was set. Coverity doesn't
seem to acknowledge the relationship between proplist and nproplist
assuming in virQEMUCapsFreeStringList that nproplist could be at
least 1 and thus have a null deref. It only seems to follow the
NULL proplist.
John Ferlan [Thu, 11 Sep 2014 21:05:34 +0000 (17:05 -0400)]
virfile: Resolve Coverity RESOURCE_LEAK
With the virGetGroupList() change in place - Coverity further complains
that if we fail to virFork(), the groups will be leaked - which aha seems
to be the case. Adjust the logic to save off the -errno, free the groups,
and then return the value we saved
John Ferlan [Thu, 11 Sep 2014 21:01:12 +0000 (17:01 -0400)]
virutil: Resolve Coverity RESOURCE_LEAK
This ends up being a very bizarre false positive. With an assist from
eblake, the claim is that mgetgroups() could return a -1 value, but yet
still have a groups buffer allocated, yet the example shown doesn't
seem to prove that.
Rather than fret about it, by adding a well placed sa_assert() on the
returned *list value we can "assure" ourselves that the mgetgroups()
failure path won't signal this condition.
John Ferlan [Thu, 11 Sep 2014 20:58:40 +0000 (16:58 -0400)]
daemon: Resolve Coverity RESOURCE_LEAK
With eblake's help - adjust the checks for stdinfd/stdoutfd to ensure the
values are within the range we expect; otherwise the dup2()'s and subsequent
VIR_CLOSE() calls cause Coverity to believe there's a resource leak.
John Ferlan [Thu, 4 Sep 2014 21:08:48 +0000 (17:08 -0400)]
virsh: Resolve Coverity NEGATIVE_RETURNS
Coverity notes that after we VIR_ALLOC_N(params, nparams) a failed call to
virDomainGetCPUStats could result in nparams being set to -1. In that case,
the subsequent virTypedParamsFree in cleanup will pass -1 which isn't good.
Use the returned value as the number of stats to display in the loop as
it will be the value reported from the hypervisor and may be less than
nparams which is OK
If a (floppy) drive isn't selected for snapshot explicitly and is empty
don't try to snapshot it. For external snapshots this would fail as we
can't generate a name for the snapshot from an empty drive.
Michal Privoznik [Fri, 12 Sep 2014 07:01:46 +0000 (09:01 +0200)]
libvirt.spec: Fix permission even for libvirt-driver-qemu
In my previous patch (37d8c75fad) I've tried to fix permissions
for nvram store path. The aim was to give the nvram directory
execute permission so that domain running under other users
than qemu:qemu can access their nvram file. However, my fix
was incomplete as the path belongs to libvirt-driver-qemu
package too and I've fixed it only for the libvirt-daemon
package.
Reported-by: Laszlo Ersek <lersek@redhat.com> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Jim Fehlig [Wed, 3 Sep 2014 20:14:50 +0000 (14:14 -0600)]
libxl: fix mapping of libvirt and libxl lifecycle actions
The libxl driver was blindly assigning libvirt's
virDomainLifecycleAction to libxl's libxl_action_on_shutdown, when
in fact the various actions take on different values in these enums.
Introduce helpers to properly map the enum values.
Rename the VIR_MOCK_IMPL* macros to VIR_MOCK_WRAP*
and add new VIR_MOCK_IMPL macros which let you directly
implement overrides in the preloaded source.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> Signed-off-by: Jim Fehlig <jfehlig@suse.com>
Test suites using the port allocator don't want to have different
behaviour depending on whether a port is in use on the host. Add
a VIR_PORT_ALLOCATOR_SKIP_BIND_CHECK which test suites can use
to skip the bind() test. The port allocator will thus only track
ports in use by the test suite process itself. This is fine when
using the port allocator to generate guest configs which won't
actually be launched
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> Signed-off-by: Jim Fehlig <jfehlig@suse.com>
Michal Privoznik [Thu, 11 Sep 2014 10:09:04 +0000 (12:09 +0200)]
nvram: Fix permissions
I've noticed two problem with the automatically created NVRAM varstore
file. The first, even though I run qemu as root:root for some reason I
get Permission denied when trying to open the _VARS.fd file. The
problem is, the upper directory misses execute permissions, which in
combination with us dropping some capabilities result in EPERM.
The next thing is, that if I switch SELinux to enforcing mode, I get
another EPERM because the vars file is not labeled correctly. It is
passed to qemu as disk and hence should be labelled as disk. QEMU may
write to it eventually, so this is different to kernel or initrd.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
John Ferlan [Thu, 4 Sep 2014 21:21:37 +0000 (17:21 -0400)]
libxl: Resolve Coverity NULL_RETURNS
With all the changes in my previous foray into this code, I forgot to
remove the libxlDomainEventQueue(driver, event); call inside the
dom == NULL condition.
John Ferlan [Thu, 4 Sep 2014 21:18:48 +0000 (17:18 -0400)]
qemu: Resolve Coverity NEGATIVE_RETURNS
Coverity notes that if the virConnectListAllDomains returns a negative
value then the loop at the cleanup label that ends on numDomains will
have issues.
John Ferlan [Thu, 4 Sep 2014 21:11:57 +0000 (17:11 -0400)]
xen: Resolve Coverity NEGATIVE_RETURNS
Coverity notes that if the call to virBitmapParse() returns a negative
value, then when we jump to the error label, the call to
virCapabilitiesClearHostNUMACellCPUTopology() will have issues
with the negative nb_cpus
John Ferlan [Thu, 4 Sep 2014 21:00:30 +0000 (17:00 -0400)]
nodeinfo: Resolve Coverity NEGATIVE_RETURNS
If the virNumaGetNodeCPUs() call fails with -1, then jumping to cleanup
with 'cpus == NULL' and calling virCapabilitiesClearHostNUMACellCPUTopology
will cause issues.
John Ferlan [Thu, 4 Sep 2014 20:50:15 +0000 (16:50 -0400)]
qemu: Resolve Coverity NEGATIVE_RETURNS
In qemuProcessInitPCIAddresses() if qemuMonitorGetAllPCIAddresses()
returns a negative (or zero) value, then no need to call the
qemuProcessDetectPCIAddresses().
John Ferlan [Thu, 4 Sep 2014 21:59:20 +0000 (17:59 -0400)]
network_conf: Resolve Coverity FORWARD_NULL
The code compares def->forwarders when deciding to return 0 at a
couple of points, then uses "def->nfwds" as a way to index into
the def->forwarders array. That reference results in Coverity
complaining that def->forwarders being NULL was checked as part
of an arithmetic OR operation where failure could be any one 5
conditions, but that is not checked when entering the loop to
dereference the array. Changing the comparisons to use nfwds
will clear the warnings
John Ferlan [Thu, 4 Sep 2014 20:40:34 +0000 (16:40 -0400)]
qemu: Resolve Coverity FORWARD_NULL
If the qemuMigrationEatCookie() fails to set mig, we jump to cleanup:
which will call qemuMigrationCancelDriveMirror() without first checking
if mig == NULL
John Ferlan [Thu, 4 Sep 2014 20:37:11 +0000 (16:37 -0400)]
virstring: Resolve Coverity FORWARD_NULL
Perhaps a false positive, but since Coverity doesn't understand the
relationship between the 'count' and the 'strings', rather than leave
the chance the on input 'strings' is NULL and causes a deref - just
check for it and return
John Ferlan [Thu, 4 Sep 2014 20:27:58 +0000 (16:27 -0400)]
network: Resolve Coverity FORWARD_NULL
If the VIR_STRDUP(exptime,...) fails, then we will jump to cleanup,
no need to check if exptime is set which causes Coverity to issue
a complaint in the virStrToLong_ll call because there wasn't a check
for a NULL value while there was one for the reference right after
the VIR_STRDUP().
John Ferlan [Thu, 4 Sep 2014 20:12:44 +0000 (16:12 -0400)]
qemu: Resolve Coverity FORWARD_NULL
If the virJSONValueNewObject() fails, then rather than going to error
and getting a Coverity false positive since it doesn't seem to understand
the relationship between nkeywords, keywords, and values and seems to
believe calling qemuFreeKeywords will cause a NULL deref - just return NULL
John Ferlan [Thu, 4 Sep 2014 19:42:08 +0000 (15:42 -0400)]
virsh: Resolve Coverity DEADCODE
Coverity points out that if 'dom' isn't returned from virDomainQemuAttach,
then the code already jumps to cleanup, so there was no need for the
subsequent if (dom != NULL) check.
I moved the error message about failure into the goto cleanup on failure
and then removed the if (dom != NULL)
John Ferlan [Thu, 4 Sep 2014 19:08:49 +0000 (15:08 -0400)]
virsh: Resolve Coverity DEADCODE
Coverity points out that by using EMPTYSTR(type) we are guarding against
the possibility that it could be NULL; however, based on how 'type' was
initialized to NULL, then using nested ternary if-then-else's (?:?:)
setting either "ipv4", "ipv6", or "" - there is no way it could be NULL.
Since "-" is supposed to mean something empty in a field - modify the
nested ternary to an easier to read/process if-then-else leaving the
initialization to NULL to mean "-" in the formatted output.
John Ferlan [Thu, 4 Sep 2014 18:46:34 +0000 (14:46 -0400)]
virfile: Resolve Coverity DEADCODE
Adjust the parentheses in/for the waitpid loops; otherwise, Coverity
points out:
(1) Event assignment: Assigning: "waitret" = "waitpid(pid, &status, 0) == -1"
(2) Event between: At condition "waitret == -1", the value of "waitret"
must be between 0 and 1.
(3) Event dead_error_condition: The condition "waitret == -1" cannot
be true.
(4) Event dead_error_begin: Execution cannot reach this statement:
"ret = -*__errno_location();".
However, foo_edited could never be set in the code path - even with
all the goto's since the only way for it to be set is if vir*DefineXML()
succeeds in which case the code to allow a retry (and thus all the goto's)
never leaves foo_edited set
All error paths lead to "cleanup:" which causes both foo and foo_edited
to call the respective vir*Free() routines if set.
John Ferlan [Thu, 4 Sep 2014 14:34:43 +0000 (10:34 -0400)]
storage: Resolve Coverity OVERFLOW_BEFORE_WIDEN
Coverity complains that when multiplying to 32 bit values that eventually
will be stored in a 64 bit value that it's possible the math could
overflow unless one of the values being multiplied is type cast to
the proper size.
John Ferlan [Thu, 4 Sep 2014 14:12:13 +0000 (10:12 -0400)]
qemu: Resolve Coverity REVERSE_INULL
Coverity complains that checking for !domlist after setting doms = domlist
and making a deref of doms just above
It seems the call in question was intended to me made in the case that
'doms' was passed in and not when the virDomainObjListExport() call
allocated domlist and already called virConnectGetAllDomainStatsCheckACL().
Thus rather than check for !domlist - check that "doms != domlist" in
order to avoid the Coverity message.
John Ferlan [Thu, 4 Sep 2014 13:56:06 +0000 (09:56 -0400)]
vbox: Resolve Coverity UNUSED_VALUE
Handle a few places where Coverity complains about the value being
unused. For two of them (Close cases) - the comments above the close
indicate there is no harm to ignore the error - so added an ignore_value.
For the other condition, added an rc check like other callers.
John Ferlan [Wed, 3 Sep 2014 19:10:56 +0000 (15:10 -0400)]
qemu_driver: Resolve Coverity COPY_PASTE_ERROR
In qemuDomainSetBlkioParameters(), Coverity points out that the calls
to qemuDomainParseBlkioDeviceStr() are slightly different and points
out there may be a cut-n-paste error.
In the first call (AFFECT_LIVE), the second parameter is "param->field";
however, for the second call (AFFECT_CONFIG), the second parameter is
"params->field". It seems the "param->field" is correct especially since
each path as a setting of "param" to "¶ms[i]". Furthermore, there
were a few more instances of using "params[i]" instead of "param->"
which I cleaned up.
Michal Privoznik [Thu, 11 Sep 2014 08:04:35 +0000 (10:04 +0200)]
selinux: Properly check TAP FD label
After a4431931 the TAP FDs ale labeled with image label instead
of the process label. On the other hand, the commit was
incomplete as a few lines above, there's still old check for the
process label presence while it should be check for the image
label instead.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Peter Krempa [Wed, 10 Sep 2014 08:46:41 +0000 (10:46 +0200)]
virsh: desc command in --title mode mentions description instead of title
Tweak the messages so that they mention "title" rather than
"description" when operating in title mode. Also fixes one missing "%s"
before non-formatted gettext message.
Before:
$ virsh desc --title dom
No description for domain: dom
After:
$ virsh desc --title dom
No title for domain: dom
Michal Privoznik [Wed, 10 Sep 2014 09:47:19 +0000 (11:47 +0200)]
virprocess: Introduce our own setns() wrapper
From time to time weird bugreports occur on the list, e.g [1].
Even though the kernel supports setns syscall, there's an older
glibc in the system that misses a wrapper over the syscall.
Hence, after the configure phase we think there's no setns
support in the system, which is obviously wrong. On the other
hand, we can't rely on linux distributions to provide newer glibc
soon. Therefore we need to introduce the wrapper on or own.
Peter Krempa [Tue, 9 Sep 2014 15:09:58 +0000 (17:09 +0200)]
util: process: Don't report OOM errors in helper
virProcessTranslateStatus is used on error paths that should not spoil
the returned error. As the errors are ignored, use the quiet versions of
virAsprintf to create the message.
When using split UEFI image, it may come handy if libvirt manages per
domain _VARS file automatically. While the _CODE file is RO and can be
shared among multiple domains, you certainly don't want to do that on
the _VARS file. This latter one needs to be per domain. So at the
domain startup process, if it's determined that domain needs _VARS
file it's copied from this master _VARS file. The location of the
master file is configurable in qemu.conf.
Temporary, on per domain basis the location of master NVRAM file can
be overridden by this @template attribute I'm inventing to the
<nvram/> element. All it does is holding path to the master NVRAM file
from which local copy is created. If that's the case, the map in
qemu.conf is not consulted.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Acked-by: Laszlo Ersek <lersek@redhat.com>
where the first line reflects <loader> and the second one <nvram>.
Moreover, these two lines obsolete the -bios argument.
Note that UEFI is unusable without ACPI. This is handled properly now.
Among with this extension, the variable file is expected to be
writable and hence we need security drivers to label it.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Acked-by: Laszlo Ersek <lersek@redhat.com>
Up to now, users can configure BIOS via the <loader/> element. With
the upcoming implementation of UEFI this is not enough as BIOS and
UEFI are conceptually different. For instance, while BIOS is ROM, UEFI
is programmable flash (although all writes to code section are
denied). Therefore we need new attribute @type which will
differentiate the two. Then, new attribute @readonly is introduced to
reflect the fact that some images are RO.
Moreover, the OVMF (which is going to be used mostly), works in two
modes:
1) Code and UEFI variable store is mixed in one file.
2) Code and UEFI variable store is separated in two files
The latter has advantage of updating the UEFI code without losing the
configuration. However, in order to represent the latter case we need
yet another XML element: <nvram/>. Currently, it has no additional
attributes, it's just a bare element containing path to the variable
store file.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Acked-by: Laszlo Ersek <lersek@redhat.com> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Jiri Denemark [Thu, 28 Aug 2014 14:39:58 +0000 (16:39 +0200)]
qemu: Transfer recomputed stats back to source
After the previous commit, migration statistics on the source and
destination hosts are not equal because the destination updated time
statistics. Let's send the result back so that the same data can be
queried on both sides of the migration.
Jiri Denemark [Thu, 28 Aug 2014 14:37:38 +0000 (16:37 +0200)]
qemu: Recompute downtime and total time when migration completes
Total time of a migration and total downtime transfered from a source to
a destination host do not count with the transfer time to the
destination host and with the time elapsed before guest CPUs are
resumed. Thus, source libvirtd remembers when migration started and when
guest CPUs were paused. Both timestamps are transferred to destination
libvirtd which uses them to compute total migration time and total
downtime. Obviously, this requires the time to be synchronized between
the two hosts. The reported times are useless otherwise but they would
be equally useless if we didn't do this recomputation so don't lose
anything by doing it.
Jiri Denemark [Thu, 28 Aug 2014 12:06:10 +0000 (14:06 +0200)]
qemu: Transfer migration statistics to destination
When migrating a transient domain or with VIR_MIGRATE_UNDEFINE_SOURCE
flag, the domain may disappear from source host. And so will migration
statistics associated with the domain. We need to transfer the
statistics at the end of a migration so that they can be queried at the
destination host.
Jiri Denemark [Thu, 28 Aug 2014 11:52:58 +0000 (13:52 +0200)]
Add support for fetching statistics of completed jobs
virDomainGetJobStats gains new VIR_DOMAIN_JOB_STATS_COMPLETED flag that
can be used to fetch statistics of a completed job rather than a
currently running job.
Jiri Denemark [Thu, 21 Aug 2014 13:08:39 +0000 (15:08 +0200)]
Refactor job statistics
Job statistics data were tracked in several structures and variables.
Let's make a new qemuDomainJobInfo structure which can be used as a
single source of statistics data as a preparation for storing data about
completed a job.
Eric Blake [Mon, 25 Aug 2014 18:58:49 +0000 (12:58 -0600)]
blockcopy: add a way to parse disk source
The new blockcopy API wants to reuse only a subset of the disk
hotplug parser - namely, we only care about the embedded
virStorageSourcePtr inside a <disk> XML. Strange as it may
seem, it was easier to just parse an entire disk definition,
then throw away everything but the embedded source, than it
was to disentangle the source parsing code from the rest of
the overall disk parsing function. All that I needed was a
couple of tweaks and a new internal flag that determines
whether the normally-mandatory target element can be
gracefully skipped, since everything else was already optional.
* src/conf/domain_conf.h (virDomainDiskSourceParse): New
prototype.
* src/conf/domain_conf.c (VIR_DOMAIN_XML_INTERNAL_DISK_SOURCE):
New flag.
(virDomainDiskDefParseXML): Honor flag to make target optional.
(virDomainDiskSourceParse): New function.
Eric Blake [Mon, 8 Sep 2014 14:50:48 +0000 (08:50 -0600)]
blockjob: avoid 32-bit compilation warning
Commit c1d75de caused this warning on 32-bit platforms (fatal when
-Werror is enabled):
virsh-domain.c: In function 'cmdBlockCopy':
virsh-domain.c:2003:17: error: comparison is always false due to limited range of data type [-Werror=type-limits]
Forcing the left side of the < to be ull instead of ul shuts up
the 32-bit compiler while still protecting 64-bit code from overflow.
* tools/virsh-domain.c (cmdBlockCopy): Add type coercion.
Erik Skultety [Mon, 8 Sep 2014 10:27:23 +0000 (12:27 +0200)]
qemu: panic device: check for invalid address type
qemu now checks for invalid address type for a panic device, which is
currently implemented only to use ISA address type, thus rejecting
any other options, except for leaving XML attributes blank, in that case,
defaults are used (this behaviour remains the same from earlier verions).
qemu: Propagate QEMU errors during incoming migrations
When QEMU fails during incoming migration after we successfully started
it (i.e., during Perform or Finish phase), we report a rather unhelpful
message
Unable to read from monitor: Connection reset by peer
We already have a code that takes error messages from QEMU's error
output but we disable it once QEMU successfully starts. This patch
postpones this until the end of Finish phase during incoming migration
so that we can report a much better error message:
internal error: early end of file from monitor: possible problem:
Unknown savevm section or instance '0000:00:05.0/virtio-balloon' 0
load of migration failed
Peter Krempa [Wed, 3 Sep 2014 13:43:31 +0000 (15:43 +0200)]
qemu: snapshot: Simplify error paths
Return failure right away when the domain object can't be looked up
instead of jumping to cleanup. This allows to remove the condition
before unlocking the domain object.
Jincheng Miao [Thu, 28 Aug 2014 11:27:04 +0000 (19:27 +0800)]
qemu: snapshot: Acquire job earlier on snapshot revert/delete
The code would lookup the snapshot object before acquiring the job. This
could lead to a crash as one thread could delete the snapshot object,
while a second thread already had the reference.
Signed-off-by: Jincheng Miao <jmiao@redhat.com> Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Peter Krempa [Wed, 3 Sep 2014 13:17:50 +0000 (15:17 +0200)]
qemu: snapshot: Fix job handling when creating snapshots
Creating snapshots modifies the domain state. Currently we wouldn't
enter the job for certain operations although they would modify the
state. Refactor job handling so that everything is covered by an async
job.
For security type='none' libvirt according to the docs should not
generate seclabel be it for selinux or any model. So, skip the
reservation of labels when type is none.
Signed-off-by: Shivaprasad G Bhat <sbhat@linux.vnet.ibm.com>
Eric Blake [Fri, 29 Aug 2014 21:47:28 +0000 (15:47 -0600)]
blockcopy: expose new API in virsh
Expose the new power of virDomainBlockCopy through virsh (well,
all but the finer-grained bandwidth, as that is its own can of
worms for a later patch). Continue to use the older API where
possible, for maximum compatibility.
The command now requires either --dest (with optional --format
and --blockdev), to directly describe the file destination, or
--xml, to name a file that contains an XML description such as:
[well, it may be a while before the qemu driver is actually patched
to act on that particular xml beyond just parsing it, but the virsh
interface won't need changing at that time]
Non-zero option parameters are converted into virTypedParameters,
and if anything requires the new API, the command can synthesize
appropriate XML even if the --dest option was used instead of --xml.
The existing --raw flag remains for back-compat, but the preferred
spelling is now --format=raw, since the new API now allows us
to specify all formats rather than just a boolean raw to suppress
probing.
I hope I did justice in describing the effects of granularity and
buf-size on how they get passed through to qemu.
* tools/virsh-domain.c (cmdBlockCopy): Add new options --xml,
--granularity, --buf-size, --format. Make --raw an alias for
--format=raw. Call new API if new parameters are in use.
* tools/virsh.pod (blockcopy): Document new options.
Eric Blake [Thu, 4 Sep 2014 12:39:54 +0000 (06:39 -0600)]
maint: update to latest gnulib
The usual portability fixes; and this includes a fix that adds
a new syntax check for double semicolons (commit 28de556 fixed
some, but gnulib found a better check).
* .gnulib: Update to latest.
* src/xenconfig/xen_common.c (xenFormatConfigCommon): Fix offender.
Eric Blake [Fri, 29 Aug 2014 21:20:30 +0000 (15:20 -0600)]
blockcopy: split out virsh implementation
I'm about to extend the capabilities of blockcopy. Hiding a few
common lines of implementation gets in the way of the new required
logic, and putting the new logic in the common implementation won't
benefit any of the other blockjob operations. Therefore, it is
simpler to just do the work inline. There should be no semantic
change in this patch.
Resolve a libvirtd crash in virStoragePoolSourceFindDuplicate()
when there is an existing SCSI pool defined with adapter type as
'scsi_host' and defining a new SCSI pool with adapter type as
'fc_host' and parent attribute missing or vice versa.
For example, if there is an existing SCSI pool with adapter type
as 'scsi_host' defined using the following XML
Same is true for the reverse case as well where there exists a SCSI pool
with adapter type as 'fc_host' and another SCSI pool is defined with
adapter type as 'scsi_host'.
This happens because for fc_host 'name' is optional attribute whereas for
scsi_host its mandatory. However the check in libvirt for finding duplicate
storage pools didn't take that into account while comparing
Signed-off-by: Pradipta Kr. Banerjee <bpradip@in.ibm.com>
Eric Blake [Thu, 28 Aug 2014 04:03:04 +0000 (22:03 -0600)]
blockcopy: allow block device destination
To date, anyone performing a block copy and pivot ends up with
the destination being treated as <disk type='file'>. While this
works for data access for a block device, it has at least one
noticeable shortcoming: virDomainGetBlockInfo() reports allocation
differently for block devices visited as files (the size of the
device) than for block devices visited as <disk type='block'>
(the maximum sector used, as reported by qemu); and this difference
is significant when trying to manage qcow2 format on block devices
that can be grown as needed.
Of course, the more powerful virDomainBlockCopy() API can already
express the ability to set the <disk> type. But a new API can't
be backported, while a new flag to an existing API can; and it is
also rather inconvenient to have to resort to the full power of
generating XML when just adding a flag to the older call will do
the trick. So this patch enhances blockcopy to let the user flag
when the resulting XML after the copy must list the device as
type='block'.
* include/libvirt/libvirt.h.in (VIR_DOMAIN_BLOCK_REBASE_COPY_DEV):
New flag.
* src/libvirt.c (virDomainBlockRebase): Document it.
* tools/virsh-domain.c (opts_block_copy, blockJobImpl): Add
--blockdev option.
* tools/virsh.pod (blockcopy): Document it.
* src/qemu/qemu_driver.c (qemuDomainBlockRebase): Allow new flag.
(qemuDomainBlockCopy): Remember the flag, and make sure it is only
used on actual block devices.
* tests/qemuxml2argvdata/qemuxml2argv-disk-mirror.xml: Test it.
Eric Blake [Thu, 28 Aug 2014 23:39:25 +0000 (17:39 -0600)]
blockjob: add new --bytes flag to virsh blockjob
Expose the new flag just added to virDomainGetBlockJobInfo.
With --raw, the presence or absence of --bytes determines which
flag to use in the single API call. Without --raw, the use of
--bytes forces an error if the server doesn't support it,
otherwise, the code tries to silently fall back to scaling the
MiB/s value.
My goal is to eventually also support --bytes in bandwidth mode;
but that's a bit further down the road (and needs a new API flag
added in libvirt.h first).
This changes the human output, but the previous patch added
raw output precisely so that we can have flexibility with the
human output. For this commit, I used qemu-monitor-command to
force an unusual bandwidth, but the same will be possible once
qemu implements virDomainBlockCopy:
The cache avoids having to repeatedly checking whether the flag
works when talking to an older server, when multiple blockjob
commands are issued during a batch session and the user is
manually polling for job completion.
Eric Blake [Thu, 28 Aug 2014 23:23:49 +0000 (17:23 -0600)]
blockjob: add new --raw flag to virsh blockjob
The current output of 'blockjob [--info]' is a single line
designed for human consumption; it's not very nice for machine
parsing. Furthermore, I have plans to modify the line in
response to the new flag for controlling bandwidth units.
Solve that by adding a --raw parameter, which outputs
information closer to the C struct.
The information is indented, because I'd like for a later patch
to add a mode that iterates over all the vm's disks with status
for each; in that mode, each block name would be listed unindented
before information (if any) about that block.
Now that we have a raw mode, we can guarantee that it won't change
format over time. Any app that cares about parsing the output can
try --raw, and if it fails, know that it was talking to an older
virsh and fall back to parsing the human-readable format which had
not changed until now; meanwhile, when not using --raw, we have
freed future virsh to change the output to whatever makes sense.
My first change to human mode: this command now guarantees a line
is printed on successful use of the API, even when the API did
not find a current block job (consistent with the rest of virsh).
Bonus: https://bugzilla.redhat.com/show_bug.cgi?id=1135441
complained that this message was confusing:
$ virsh blockjob test1 hda --async --bandwidth 10
error: conflict between --abort, --info, and --bandwidth modes
even though the man page already documents that --async implies
abort mode, all because '--abort' wasn't present in the command
line. Since I'm adding another case where options are tied
to or imply a mode, I changed that error to:
error: conflict between abort, info, and bandwidth modes
Eric Blake [Thu, 28 Aug 2014 19:18:22 +0000 (13:18 -0600)]
blockjob: split up virsh blockjob info
I have plans to make future enhancements to the job list mode,
which will be easier to do if the common blockJobImpl function
is not mixing a query command with multiple modify commands.
Besides, it just feels weird that all callers to blockJobImpl
had to supply both a bandwidth input argument (unused for info
mode) and an info output argument (unused for all other modes);
not to mention I just made similar cleanups on the libvirtd
side.
The only reason blockJobImpl returned int was because of info
mode returning -1/0/1 (all other job API are -1/0), so that
can also be cleaned up. No user-visible changes in this commit.
* tools/virsh-domain.c (blockJobImpl): Change signature and return
value. Drop info handling.
(cmdBlockJob): Handle info here.
(cmdBlockCommit, cmdBlockCopy, cmdBlockPull): Adjust callers.
Eric Blake [Wed, 27 Aug 2014 20:04:36 +0000 (14:04 -0600)]
blockjob: allow finer bandwidth tuning for query
While reviewing the new virDomainBlockCopy API, Peter Krempa
pointed out that our existing design of using MiB/s for block
job bandwidth is rather coarse, especially since qemu tracks
it in bytes/s; so virDomainBlockCopy only accepts bytes/s.
But once the new API is implemented for qemu, we will be in
the situation where it is possible to set a value that cannot
be accurately reflected back to the user, because the existing
virDomainGetBlockJobInfo defaults to the coarser units.
Fortunately, we have an escape hatch; and one that has already
served us well in the past: we can use the flags argument to
specify which scale to use (see virDomainBlockResize for prior
art). This patch fixes the query side of the API; made easier
by previous patches that split the query side out from the
modification code. Later patches will address the virsh
interface, as well retrofitting all other blockjob APIs to
also accept a flag for toggling bandwidth units.
projects / libvirt.git / log