Signed-off-by: NMartin Kletzander <mkletzan@redhat.com>

Just remove useless "else".  Best viewed with '-w'.
Signed-off-by: NMartin Kletzander <mkletzan@redhat.com>

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:

  https://bugzilla.redhat.com/show_bug.cgi?id=811967

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.
Signed-off-by: NEric Blake <eblake@redhat.com>

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).

* src/qemu/qemu_monitor.h (qemuMonitorDriveMirror): Add
parameters.
* src/qemu/qemu_monitor.c (qemuMonitorDriveMirror): Likewise.
* src/qemu/qemu_monitor_json.h (qemuMonitorJSONDriveMirror):
Likewise.
* src/qemu/qemu_monitor_json.c (qemuMonitorJSONDriveMirror):
Likewise.
* src/qemu/qemu_driver.c (qemuDomainBlockCopyCommon): Likewise.
(qemuDomainBlockRebase, qemuDomainBlockCopy): Adjust callers.
* src/qemu/qemu_migration.c (qemuMigrationDriveMirror): Likewise.
* tests/qemumonitorjsontest.c (qemuMonitorJSONDriveMirror): Likewise.
Signed-off-by: NEric Blake <eblake@redhat.com>

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.
Signed-off-by: NEric Blake <eblake@redhat.com>

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.

* src/qemu/qemu_driver.c (qemuDomainBlockCopy): Rename...
(qemuDomainBlockCopyCommon): ...and adjust parameters.
(qemuDomainBlockRebase): Adjust caller.
Signed-off-by: NEric Blake <eblake@redhat.com>

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: NLaszlo Ersek <lersek@redhat.com>
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>

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: NMichal Privoznik <mprivozn@redhat.com>

Add a check of the return for virDomainHostdevInsert() like every
other call.
Signed-off-by: NJohn Ferlan <jferlan@redhat.com>

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.
Signed-off-by: NJohn Ferlan <jferlan@redhat.com>

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
Signed-off-by: NJohn Ferlan <jferlan@redhat.com>

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.
Signed-off-by: NJohn Ferlan <jferlan@redhat.com>

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.
Signed-off-by: NJohn Ferlan <jferlan@redhat.com>

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
Signed-off-by: NJohn Ferlan <jferlan@redhat.com>

Because of similar contexts, git rebase I did just before pushing the
series which added --completed option patched the wrong command.

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.
Reported-by: NPavel Hrdina <phrdina@redhat.com>

To express empty drive we historically use storage source with empty
path. Unfortunately NBD disks may be declared without a path.

Add a helper to wrap this logic.

In my previous patch (37d8c75f) 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: NLaszlo Ersek <lersek@redhat.com>
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>

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.
Signed-off-by: NJim Fehlig <jfehlig@suse.com>

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: NDaniel P. Berrange <berrange@redhat.com>
Signed-off-by: NJim 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: NDaniel P. Berrange <berrange@redhat.com>
Signed-off-by: NJim Fehlig <jfehlig@suse.com>

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: NMichal Privoznik <mprivozn@redhat.com>

The build failed because of missing "sys/syscall.h".
Signed-off-by: NPavel Hrdina <phrdina@redhat.com>

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.
Signed-off-by: NJohn Ferlan <jferlan@redhat.com>

Coverity notes that if the virConnectListAllDomains returns a negative
value then the loop at the cleanup label that ends on numDomains will
have issues.
Signed-off-by: NJohn Ferlan <jferlan@redhat.com>

Coverity notes that if qemuMonitorGetMachines() returns a negative
nmachines value, then the code at the cleanup label will have issues.
Signed-off-by: NJohn Ferlan <jferlan@redhat.com>

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
Signed-off-by: NJohn Ferlan <jferlan@redhat.com>

If the virNumaGetNodeCPUs() call fails with -1, then jumping to cleanup
with 'cpus == NULL' and calling virCapabilitiesClearHostNUMACellCPUTopology
will cause issues.
Signed-off-by: NJohn Ferlan <jferlan@redhat.com>

In qemuProcessInitPCIAddresses() if qemuMonitorGetAllPCIAddresses()
returns a negative (or zero) value, then no need to call the
qemuProcessDetectPCIAddresses().
Signed-off-by: NJohn Ferlan <jferlan@redhat.com>

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
Signed-off-by: NJohn Ferlan <jferlan@redhat.com>

If the qemuMigrationEatCookie() fails to set mig, we jump to cleanup:
which will call qemuMigrationCancelDriveMirror() without first checking
if mig == NULL
Signed-off-by: NJohn Ferlan <jferlan@redhat.com>

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
Signed-off-by: NJohn Ferlan <jferlan@redhat.com>

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().
Signed-off-by: NJohn Ferlan <jferlan@redhat.com>

If we jump to cleanup before allocating the 'result', then the call
to virBlkioDeviceArrayClear will deref result causing a problem.
Signed-off-by: NJohn Ferlan <jferlan@redhat.com>

If we jump to cleanup before allocating 'result', then the call to
virBlkioDeviceArrayClear() could dereference result
Signed-off-by: NJohn Ferlan <jferlan@redhat.com>

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
Signed-off-by: NJohn Ferlan <jferlan@redhat.com>

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)
Signed-off-by: NJohn Ferlan <jferlan@redhat.com>

Coverity complains that the various checks for autoincrement and changed
variables are DEADCODE - seems to me to be a false positive - so mark it.
Signed-off-by: NJohn Ferlan <jferlan@redhat.com>

Add another 'dead_code_begin' - victims of our own coding practices
Signed-off-by: NJohn Ferlan <jferlan@redhat.com>