In commit 540c339a the whole domain
reference counting was refactored in the qemu driver. Domain jobs now
don't need to reference the domain object as they now expect the
reference from the calling function.

However, the patch forgot to remove the unref call in case we exit the
monitor when we were acquiring a nested job. This caused the daemon to
crash on a subsequent access to the domain object once we've done an
operation requiring a nested job for a monitor access.

An easy reproducer case:

1) Start a vm with qcow disks
2) virsh snapshot-create-as DOMNAME
3) virsh dumpxml DOMNAME
4) daemon crashes in a semi-random spot while accessing a now-removed VM
object.

Fortunately, the commit wasn't released yet, so there are no security
implications.
Reported-by: NShanzi Yu <shyu@redhat.com>
Signed-off-by: NPeter Krempa <pkrempa@redhat.com>

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

When setting new bandwidth limits via
virDomainSetInterfaceParameters, the old ones are cleared first.
However, if setting the new ones fails, the old are already gone
and interface is left in inconsistent state.  Therefore, right
before failing we ought to try to restore the old bandwidth.
Signed-off-by: NLuyao Huang <lhuang@redhat.com>

This place have a wrong logic, maybe forget goto cleanup.
Also fix some small things.
Signed-off-by: NLuyao Huang <lhuang@redhat.com>

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

We will get a warning when we have a guest in paused
status (caused by kernel panic) and restart libvirtd,
warning message like this:

Qemu reported unknown VM status: 'guest-panicked'

and this seems because we set a wrong status name in
qemu_monitor.c, and from qemu qapi-schema.json file
we know this status should named 'guest-panicked'.
Signed-off-by: NLuyao Huang <lhuang@redhat.com>
Signed-off-by: NEric Blake <eblake@redhat.com>

Add the possibility to have more than one IP address configured for a
domain network interface. IP addresses can also have a prefix to define
the corresponding netmask.

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

There is one problem that causes various errors in the daemon.  When
domain is waiting for a job, it is unlocked while waiting on the
condition.  However, if that domain is for example transient and being
removed in another API (e.g. cancelling incoming migration), it get's
unref'd.  If the first call, that was waiting, fails to get the job, it
unref's the domain object, and because it was the last reference, it
causes clearing of the whole domain object.  However, when finishing the
call, the domain must be unlocked, but there is no way for the API to
know whether it was cleaned or not (unless there is some ugly temporary
variable, but let's scratch that).

The root cause is that our APIs don't ref the objects they are using and
all use the implicit reference that the object has when it is in the
domain list.  That reference can be removed when the API is waiting for
a job.  And because each domain doesn't do its ref'ing, it results in
the ugly checking of the return value of virObjectUnref() that we have
everywhere.

This patch changes qemuDomObjFromDomain() to ref the domain (using
virDomainObjListFindByUUIDRef()) and adds qemuDomObjEndAPI() which
should be the only function in which the return value of
virObjectUnref() is checked.  This makes all reference counting
deterministic and makes the code a bit clearer.
Signed-off-by: NMartin Kletzander <mkletzan@redhat.com>

Although QMP returns info about vCPU threads in TCG mode, the
data it returns is mostly lies. Only the first vCPU has a valid
thread_id returned. The thread_id given for the other vCPUs is
in fact the main emulator thread. All vCPUs actually run under
the same thread in TCG mode.

Our vCPU pinning code is not at all able to cope with this
so if you try to set CPU affinity per-vCPU you end up with
wierd errors

error: Failed to start domain instance-00000007
error: cannot set CPU affinity on process 24365: Invalid argument

Since few people will care about the performance of TCG with
strict CPU pinning, lets just disable that for now, so we get
a clear error message

error: Failed to start domain instance-00000007
error: Requested operation is not valid: cpu affinity is not supported

The code assumes that def->vcpus == nvcpupids, so when we setup
fake CPU pids for old QEMU with nvcpupids == 1, we cause the
later code to read off the end of the array. This has fun results
like sche_setaffinity(0, ...) which changes libvirtd's own CPU
affinity, or even better sched_setaffinity($RANDOM, ...) which
changes the affinity of a random OS process.

Libvirt BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1175397
QEMU BZ:    https://bugzilla.redhat.com/show_bug.cgi?id=1170093

In qemu there are two interesting arguments:

1) -numa to create a guest NUMA node
2) -object memory-backend-{ram,file} to tell qemu which memory
region on which host's NUMA node it should allocate the guest
memory from.

Combining these two together we can instruct qemu to create a
guest NUMA node that is tied to a host NUMA node. And it works
just fine. However, depending on machine type used, there might
be some issued during migration when OVMF is enabled (see QEMU
BZ). While this truly is a QEMU bug, we can help avoiding it. The
problem lies within the memory backend objects somewhere. Having
said that, fix on our side consists on putting those objects on
the command line if and only if needed. For instance, while
previously we would construct this (in all ways correct) command
line:

    -object memory-backend-ram,size=256M,id=ram-node0 \
    -numa node,nodeid=0,cpus=0,memdev=ram-node0

now we create just:

    -numa node,nodeid=0,cpus=0,mem=256

because the backend object is obviously not tied to any specific
host NUMA node.
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>

Commit ca91ba78 moved these into the qemuDomainPrepareDisk helper,
but forgot to remove them from here as well.

Commit ca91ba78 moved qemuSetupDiskCgroup into the qemuDomainPrepareDisk
helper, but failed to call it for usb disks.

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

Coverity flagged commit 0282ca45 as introducing a memory leak;
in all my refactoring to make capacity probing conditional on
whether the image is non-raw, I missed deleting the unconditional
probe.

* src/qemu/qemu_driver.c (qemuStorageLimitsRefresh): Drop
redundant assignment.
Signed-off-by: NEric Blake <eblake@redhat.com>

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

There's nothing we need to do for shared iSCSI devices in
qemuAddSharedHostdev and qemuRemoveSharedHostdev. The iSCSI layer
takes care about that for us.
Signed-off-by: NLuyao Huang <lhuang@redhat.com>
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>

Wire up backing chain recursion.  For the first time, it is now
possible to get libvirt to expose that qemu tracks read statistics
on backing files, as well as report maximum extent written on a
backing file during a block-commit operation.

For a running domain, where one of the two images has a backing
file, I see the traditional output:

$ virsh domstats --block testvm2
Domain: 'testvm2'
  block.count=2
  block.0.name=vda
  block.0.path=/tmp/wrapper.qcow2
  block.0.rd.reqs=1
  block.0.rd.bytes=512
  block.0.rd.times=28858
  block.0.wr.reqs=0
  block.0.wr.bytes=0
  block.0.wr.times=0
  block.0.fl.reqs=0
  block.0.fl.times=0
  block.0.allocation=0
  block.0.capacity=1310720000
  block.0.physical=200704
  block.1.name=vdb
  block.1.path=/dev/sda7
  block.1.rd.reqs=0
  block.1.rd.bytes=0
  block.1.rd.times=0
  block.1.wr.reqs=0
  block.1.wr.bytes=0
  block.1.wr.times=0
  block.1.fl.reqs=0
  block.1.fl.times=0
  block.1.allocation=0
  block.1.capacity=1310720000

vs. the new output:

$ virsh domstats --block --backing testvm2
Domain: 'testvm2'
  block.count=3
  block.0.name=vda
  block.0.path=/tmp/wrapper.qcow2
  block.0.rd.reqs=1
  block.0.rd.bytes=512
  block.0.rd.times=28858
  block.0.wr.reqs=0
  block.0.wr.bytes=0
  block.0.wr.times=0
  block.0.fl.reqs=0
  block.0.fl.times=0
  block.0.allocation=0
  block.0.capacity=1310720000
  block.0.physical=200704
  block.1.name=vda
  block.1.path=/dev/sda6
  block.1.backingIndex=1
  block.1.rd.reqs=0
  block.1.rd.bytes=0
  block.1.rd.times=0
  block.1.wr.reqs=0
  block.1.wr.bytes=0
  block.1.wr.times=0
  block.1.fl.reqs=0
  block.1.fl.times=0
  block.1.allocation=327680
  block.1.capacity=786432000
  block.2.name=vdb
  block.2.path=/dev/sda7
  block.2.rd.reqs=0
  block.2.rd.bytes=0
  block.2.rd.times=0
  block.2.wr.reqs=0
  block.2.wr.bytes=0
  block.2.wr.times=0
  block.2.fl.reqs=0
  block.2.fl.times=0
  block.2.allocation=0
  block.2.capacity=1310720000

I may later do a patch that trims the output to avoid 0 stats,
particularly for backing files (which are more likely to have
0 stats, at least for write statistics when no block-commit
is performed).  Also, I still plan to expose physical size
information (qemu doesn't expose it yet, so it requires a stat,
and for block devices, a further open/seek operation).  But
this patch is good enough without worrying about that yet.

* src/qemu/qemu_driver.c (QEMU_DOMAIN_STATS_BACKING): New internal
enum bit.
(qemuConnectGetAllDomainStats): Recognize new user flag, and pass
details to...
(qemuDomainGetStatsBlock): ...here, where we can do longer recursion.
(qemuDomainGetStatsOneBlock): Output new field.
Signed-off-by: NEric Blake <eblake@redhat.com>

In order to report stats on backing chains, we need to separate
the output of stats for one block from how we traverse blocks.

* src/qemu/qemu_driver.c (qemuDomainGetStatsBlock): Split...
(qemuDomainGetStatsOneBlock): ...into new helper.
Signed-off-by: NEric Blake <eblake@redhat.com>

A coming patch will make it optionally possible to list backing
chain block stats; in this mode of operation, block.counts is no
longer the number of <disks> in the domain, but the number of
blocks in the array being reported.  We still want block.count
listed first, but rather than iterate the tree twice (once to
count, and once to list stats), it's easier to just touch things
up after the fact.

* src/qemu/qemu_driver.c (qemuDomainGetStatsBlock): Compute count
after the fact.
Signed-off-by: NEric Blake <eblake@redhat.com>

The prior refactoring can now be put to use. With the same domain
as the earlier commit 7b499262 (one qcow2 disk and an empty
cdrom drive):
$ virsh domstats --block foo
Domain: 'foo'
  block.count=2
  block.0.name=hda
  block.0.path=/var/lib/libvirt/images/foo.qcow2
  block.0.allocation=1309614080
  block.0.capacity=42949672960
  block.0.physical=1309671424
  block.1.name=hdc

* src/qemu/qemu_driver.c (qemuDomainGetStatsBlock): Use
qemuStorageLimitsRefresh to report offline statistics.
Signed-off-by: NEric Blake <eblake@redhat.com>

Create a helper function that can be reused for gathering block
info from virDomainListGetStats.

* src/qemu/qemu_driver.c (qemuDomainGetBlockInfo): Split guts...
(qemuStorageLimitsRefresh): ...into new helper function.
Signed-off-by: NEric Blake <eblake@redhat.com>

The documentation for virDomainBlockInfo was confusing: it stated
that 'physical' was the size of the container, then gave an example
of it being the amount of storage used by a sparse file (that is,
for a sparse raw image on a regular file, the wording implied
capacity==physical, while allocation was smaller; but the example
instead claimed physical==allocation).  Since we use 'physical' for
the last offset of a block device, we should do likewise for
regular files.

Furthermore, the example claimed that for a qcow2 regular file,
allocation==physical.  At the time the code was first written,
this was true (qcow2 files were allocated sequentially, and were
never sparse, so the last sector written happened to also match
the disk space occupied); but modern qemu does much better and
can punch holes for a qcow2 with allocation < physical.

Basically, after this patch, the three fields are now reliably
mapped as:
 'capacity' - how much storage the guest can see (equal to
physical for raw images, determined by image metadata otherwise)
 'allocation' - how much storage the image occupies (similar to
what 'du' would report)
 'physical' - the last offset of the image (similar to what 'ls'
would report)

'capacity' can be larger than 'physical' (such as for a qcow2
image that does not vary much from a backing file) or smaller
(such as for a qcow2 file with lots of internal snapshots).
Likewise, 'allocation' can be (slightly) larger than 'physical'
(such as counting the tail of cluster allocations required to
round a file size up to filesystem granularity) or smaller
(for a sparse file).  A block-resize operation changes capacity
(which, for raw images, also changes physical); many non-raw
images automatically grow physical and allocation as necessary
when starting with an allocation smaller than capacity; and even
when capacity and physical stay unchanged, allocation can change
when converting sectors from holes to data or back.

Note that this does not change semantics for qcow2 images stored
on block devices; there, we still rely on qemu to report the
highest written extent for allocation.  So using this API to
track when to extend a block device because a qcow2 image is
about to exceed a threshold will not see any changes.

Also, note that virStorageVolInfo is unfortunately limited to
just 'capacity' and 'allocation' (we can't expand it to add
'physical', although we can expand the XML to add it there);
historically, that struct's 'allocation' value has reported
file size for qcow2 files (what this patch terms 'physical'
for a domain block device), but disk usage for raw files (what
this patch terms 'allocation').  So follow-up patches will be
needed to make storage volumes report the same allocation
values and get at physical values, where those differ.

* include/libvirt/libvirt-domain.h (_virDomainBlockInfo): Tweak
documentation to match saner definition.
* src/qemu/qemu_driver.c (qemuDomainGetBlockInfo): For regular
files, physical size is capacity, not allocation.
Signed-off-by: NEric Blake <eblake@redhat.com>

Ultimately, we want to avoid read()ing a file while qemu is running.
We still have to open() block devices to determine their physical
size, but that is safer.  This patch rearranges code to group
together all code that reads the image, to make it easier for later
patches to skip the metadata collection when possible.

* src/qemu/qemu_driver.c (qemuDomainGetBlockInfo): Check for empty
disk up front.  Place metadata reading next to use.
Signed-off-by: NEric Blake <eblake@redhat.com>

When requested in a later patch, the QMP command results are now
examined recursively.  As qemu_driver will eventually have to
read items out of the hash table as stored by this patch, the
computation of backing alias string is done in a shared location.

* src/qemu/qemu_domain.h (qemuDomainStorageAlias): New prototype.
* src/qemu/qemu_domain.c (qemuDomainStorageAlias): Implement it.
* src/qemu/qemu_monitor_json.c
(qemuMonitorJSONGetOneBlockStatsInfo)
(qemuMonitorJSONBlockStatsUpdateCapacityOne): Perform recursion.
(qemuMonitorJSONGetAllBlockStatsInfo)
(qemuMonitorJSONBlockStatsUpdateCapacity): Update callers.
Signed-off-by: NEric Blake <eblake@redhat.com>

A future patch will allow recursion into backing chains when
collecting block stats.  This patch should not change behavior,
but merely moves out the common code that will be reused once
recursion is enabled, and adds the parameter that will turn on
recursion.

* src/qemu/qemu_monitor.h (qemuMonitorGetAllBlockStatsInfo)
(qemuMonitorBlockStatsUpdateCapacity): Add recursion parameter,
although it is ignored for now.
* src/qemu/qemu_monitor.h (qemuMonitorGetAllBlockStatsInfo)
(qemuMonitorBlockStatsUpdateCapacity): Likewise.
* src/qemu/qemu_monitor_json.h
(qemuMonitorJSONGetAllBlockStatsInfo)
(qemuMonitorJSONBlockStatsUpdateCapacity): Likewise.
* src/qemu/qemu_monitor_json.c
(qemuMonitorJSONGetAllBlockStatsInfo)
(qemuMonitorJSONBlockStatsUpdateCapacity): Add parameter, and
split...
(qemuMonitorJSONGetOneBlockStatsInfo)
(qemuMonitorJSONBlockStatsUpdateCapacityOne): ...into helpers.
(qemuMonitorJSONGetBlockStatsInfo): Update caller.
* src/qemu/qemu_driver.c (qemuDomainGetStatsBlock): Update caller.
* src/qemu/qemu_migration.c (qemuMigrationCookieAddNBD): Likewise.
Signed-off-by: NEric Blake <eblake@redhat.com>

Right now, grabbing blockinfo always calls stat on the disk, then
opens the image to determine the capacity, using a throw-away
virStorageSourcePtr.  This has a couple of drawbacks:

1. We are calling stat and opening a file on every invocation of
the API.  However, there are cases where the stats should NOT be
changing between successive calls (if a domain is running, no
one should be changing the physical size of a block device or raw
image behind our backs; capacity of read-only files should not
be changing; and we are the gateway to the block-resize command
to know when the capacity of read-write files should be changing).
True, we still have to use stat in some cases (a sparse raw file
changes allocation if it is read-write and the amount of holes is
changing, and a read-write qcow2 image stored in a file changes
physical size if it was not fully pre-allocated).  But for
read-only images, even this should be something we can remember
from the previous time, rather than repeating every call.

2. We want to enhance the power of virDomainListGetStats, by
sharing code.  But we already have a virStorageSourcePtr for
each disk, and it would be easier to reuse the common structure
than to have to worry about the one-off virDomainBlockInfoPtr.

While this patch does not optimize reuse of information in point
1, it does get us closer to being able to do so; by updating a
structure that survives between consecutive calls.

* src/util/virstoragefile.h (_virStorageSource): Add physical, to
mirror virDomainBlockInfo; rearrange fields to match public struct.
(virStorageSourceCopy): Copy the new field.
* src/qemu/qemu_driver.c (qemuDomainGetBlockInfo): Store into
storage source, then copy to block info.
Signed-off-by: NEric Blake <eblake@redhat.com>

In order for a future patch to virDomainListGetStats to reuse
some code for determining disk usage of offline domains, we
need to make it easier to pull out part of the guts of grabbing
blockinfo.  The current implementation grabs a job fairly late
in the game, while getstats will already own a job; reordering
things so that the job is always grabbed up front in both
functions will make it easier to pull out the common code.
This patch results in grabbing a job in cases where one was not
previously needed, but as it is a query job, it should not be
noticeably slower.

This patch touches the same code as the fix for CVE-2014-6458
(commit b7992595); in that patch, we avoided hotplug changing
a disk reference during the time of obtaining a monitor lock
by copying all data we needed and no longer referencing disk;
this patch goes the other way and ensures that by holding the
job, the disk cannot be changed so we no longer need to worry
about the disk being invalidated across the monitor lock.

* src/qemu/qemu_driver.c (qemuDomainGetBlockInfo): Rearrange job
control to be outside of disk information.
Signed-off-by: NEric Blake <eblake@redhat.com>

Commit e3435caf added cleanup code to qemuDomainSetVcpusFlags() that was
not supposed to reset the error.  Usual procedure was done, saving the
error to temporary variable, but it was never free'd, but rather leaked.
Signed-off-by: NMartin Kletzander <mkletzan@redhat.com>

Commit af2a1f05 tried clearly separating each condition in
qemuRestoreCgroupState() for the sake of readability, however somehow
one condition body was missing.  That means that the body of the next
condition got executed only if both of there were true, which is
impossible, thus resulting in a dead code and a logic error.
Signed-off-by: NMartin Kletzander <mkletzan@redhat.com>

When hot-plugging a VCPU into the guest, kvm needs to allocate some data
from the DMA zone, which might be in a memory node that's not allowed in
cpuset.mems.  Basically the same problem as there was with starting the
domain and due to which commit 7e72ac78
exists.  This patch just extends it to hotplugging as well.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1161540Signed-off-by: NMartin Kletzander <mkletzan@redhat.com>

Instead of setting the value of cpuset.mems once when the domain starts
and then re-calculating the value every time we need to change the child
cgroup values, leave the cgroup alone and rather set the child data
every time there is new cgroup created.  We don't leave any task in the
parent group anyway.  This will ease both current and future code.
Signed-off-by: NMartin Kletzander <mkletzan@redhat.com>

Thanks to that we don't need to drag the pointer everywhere and future
code will get cleaner.
Signed-off-by: NMartin Kletzander <mkletzan@redhat.com>

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

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

When we use attach-device add a hostdev or chr device which have a
iscsi address or others (just like guest agent, subsys iscsi disk...),
we will find there is no basic controller for our new attached device.
Somtimes this will make guest cannot start after we add them (although
they can start at the second time).
Signed-off-by: NLuyao Huang <lhuang@redhat.com>

When libvirt is managing a bridge's forwarding database (FDB)
(macTableManager='libvirt'), if we add FDB entries for a new guest
interface even before the qemu process is created, then in the case of
a migration any other guest attached to the "destination" bridge will
have its traffic immediately sent to the destination of the migration
even while the source domain is still running (and the destination, of
course, isn't). To make sure that traffic from other guests on the new
host continues flowing to the old guest until the new one is ready, we
have to wait until the new guest CPUs are started to add the FDB
entries.

Conversely, we need to remove the FDB entries from the bridge any time
the guest CPUs are stopped; among other things, this will assure
proper operation during a post-copy migration (which is just the
opposite of the problem described in the previous paragraph).

We can change vnc password by using virDomainUpdateDeviceFlags API with
live flag. But it can't be changed with config flag. Error is reported as
below.

error: Operation not supported: persistent update of device 'graphics' is not supported

This patch supports the graphics arguments changed with config flag.
Signed-off-by: NWang Rui <moon.wangrui@huawei.com>

Signed-off-by: NWang Rui <moon.wangrui@huawei.com>

It's not supported to change some graphics arguments with '--live'.
Replace some error code VIR_ERR_INTERNAL_ERROR and VIR_ERR_INVALID_ARG
with VIR_ERR_OPERATION_UNSUPPORTED.
Signed-off-by: NWang Rui <moon.wangrui@huawei.com>

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

It occurred to me that OpenStack uses the following XML when not using
regular huge pages:

  <memoryBacking>
    <hugepages>
      <page size='4' unit='KiB'/>
    </hugepages>
  </memoryBacking>

However, since we are expecting to see huge pages only, we fail to
startup the domain with following error:

  libvirtError: internal error: Unable to find any usable hugetlbfs
  mount for 4 KiB

While regular system pages are not huge pages technically, our code is
prepared for that and if it helps OpenStack (or other management
applications) we should cope with that.
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>

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

In our config files users are expected to pass several integer values
for different configuration knobs. However, majority of them expect a
nonnegative number and only a few of them accept a negative number too
(notably keepalive_interval in libvirtd.conf).
Therefore, a new type to config value is introduced: VIR_CONF_ULONG
that is set whenever an integer is positive or zero. With this
approach knobs accepting VIR_CONF_LONG should accept VIR_CONF_ULONG
too.
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>

We now have a qemuInterfaceStartDevices() which does the final
activation needed for the host-side tap/macvtap devices that are used
for qemu network connections. It will soon make sense to have the
converse qemuInterfaceStopDevices() which will undo whatever was done
during qemuInterfaceStartDevices().

A function to "stop" a single device has also been added, and is
called from the appropriate place in qemuDomainDetachNetDevice(),
although this is currently unnecessary - the device is going to
immediately be deleted anyway, so any extra "deactivation" will be for
naught. The call is included for completeness, though, in anticipation
that in the future there may be some required action that *isn't*
nullified by deleting the device.

This patch is a part of a more complete fix for:

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