Michal Privoznik [Mon, 22 Feb 2016 06:59:25 +0000 (07:59 +0100)]
virDomainDefFormatInternal: Drop useless check
There's a check if a domain definition has any graphics card and
if so, we iterate over each one of them. This makes no sense,
because even if it has none we can still iterate over.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
When getting a list of servers registered for a daemon, it's
returned as a dynamically allocated array filled in with pointers
to constant strings. Because the array is dynamic, it should be
freed when no longer needed (but not the strings!). Even the
function that creates the array suggests that.
==19446== 48 bytes in 3 blocks are definitely lost in loss record 821 of 1,034
==19446== at 0x4C2C28E: realloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==19446== by 0x54BAFC8: virReallocN (viralloc.c:245)
==19446== by 0x54BB0BE: virExpandN (viralloc.c:294)
==19446== by 0x54BB391: virInsertElementsN (viralloc.c:436)
==19446== by 0x164E3D: virNetDaemonGetServerNames (virnetdaemon.c:217)
==19446== by 0x15616F: adminDaemonListServers (admin_server.c:52)
==19446== by 0x155B8C: adminDispatchConnectListServers (admin.c:151)
==19446== by 0x155FD8: adminDispatchConnectListServersHelper (admin_dispatch.h:101)
==19446== by 0x568E862: virNetServerProgramDispatchCall (virnetserverprogram.c:437)
==19446== by 0x568E3C3: virNetServerProgramDispatch (virnetserverprogram.c:307)
==19446== by 0x5687B5B: virNetServerProcessMsg (virnetserver.c:135)
==19446== by 0x5687C1B: virNetServerHandleJob (virnetserver.c:156)
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Andrea Bolognani [Thu, 18 Feb 2016 15:13:42 +0000 (16:13 +0100)]
gic: Introduce VIR_GIC_VERSION_DEFAULT alias
GIC v2 is the default, but checking against that specific version when
we want to know whether the default has been selected is potentially
error prone; using an alias instead makes it safer.
Andrea Bolognani [Thu, 18 Feb 2016 14:44:20 +0000 (15:44 +0100)]
docs: List possible GIC versions
Recent changes to the handling of GIC version, specifically commit 2a7b11eafb67, have clearly defined what values are acceptable for the
version attribute of the <gic> element. Update the documentation
accordingly.
Peter Krempa [Fri, 19 Feb 2016 15:04:15 +0000 (16:04 +0100)]
qemu: iothreadpin: Always set affinity when pinning iothread
Similarly to VM startup always set the legacy affinity. Additionally we
don't need to report an explicit error since virProcessSetAffinity
reports them themselves.
Peter Krempa [Fri, 19 Feb 2016 15:04:15 +0000 (16:04 +0100)]
qemu: emulatorpin: Always set affinity when pinning emulator thread
Similarly to VM startup always set the legacy affinity. Additionally we
don't need to report an explicit error since virProcessSetAffinity
reports them themselves.
Cole Robinson [Wed, 17 Feb 2016 16:53:07 +0000 (11:53 -0500)]
qemu: parse: drop redundant AddImplicitControllers
PostParse handles it for us now.
This causes some test suite churn; qemu's custom PostParse could is
now invoked before the generic AddImplicitControllers, so PCI
controllers end up sequentially in the XML before the generically
added IDE controllers. So it's just some XML reordering
Cole Robinson [Fri, 8 Jan 2016 03:49:57 +0000 (22:49 -0500)]
domain: add implicit controllers from post parse
Seems like the natural fit, since we are already adding other XML bits
in the PostParse routine.
Previously AddImplicitControllers was only called at the end of XML
parsing, meaning code that builds a DomainDef by hand had to manually
call it. Now those PostParse callers get it for free.
There's some test churn here; xen xm and sexpr test suite bits weren't
calling this before, but now they are, so you'll see new IDE controllers.
I don't think this will cause problems in practice, since the code already
needs to handle these implicit controllers like in the case when a user
defines their own XML.
Jiri Denemark [Tue, 16 Feb 2016 09:49:26 +0000 (10:49 +0100)]
Check for active domain in virDomainObjWait
virDomainObjWait is designed to be called in a loop. Make sure we break
the loop in case the domain dies to avoid waiting for an event which
will never happen.
Jiri Denemark [Thu, 11 Feb 2016 10:20:28 +0000 (11:20 +0100)]
qemu: Avoid calling qemuProcessStop without a job
Calling qemuProcessStop without a job opens a way to race conditions
with qemuDomainObjExitMonitor called in another thread. A real world
example of such a race condition:
- migration thread (A) calls qemuMigrationWaitForSpice
- another thread (B) starts processing qemuDomainAbortJob API
- thread B signals thread A via qemuDomainObjAbortAsyncJob
- thread B enters monitor (qemuDomainObjEnterMonitor)
- thread B calls qemuMonitorSend
- thread A awakens and calls qemuProcessStop
- thread A calls qemuMonitorClose and sets priv->mon to NULL
- thread B calls qemuDomainObjExitMonitor with priv->mon == NULL
=> monitor stays ref'ed and locked
Depending on how lucky we are, the race may result in a memory leak or
it can even deadlock libvirtd's event loop if it tries to lock the
monitor to process an event received before qemuMonitorClose was called.
Jiri Denemark [Thu, 11 Feb 2016 14:32:48 +0000 (15:32 +0100)]
qemu: Process monitor EOF in a job
Stopping a domain without a job risks a race condition with another
thread which started a job a which does not expect anyone else to be
messing around with the same domain object.
Jiri Denemark [Thu, 11 Feb 2016 14:13:09 +0000 (15:13 +0100)]
qemu: Introduce qemuProcessBeginStopJob
When destroying a domain we need to make sure we will be able to start a
job no matter what other operations are running or even stuck in a job.
This is done by killing the domain before starting the destroy job.
Let's introduce qemuProcessBeginStopJob which combines killing a domain
and starting a job in a single API which can be called everywhere we
need a job to stop a domain.
Jiri Denemark [Thu, 28 Jan 2016 12:48:17 +0000 (13:48 +0100)]
qemu: End nested jobs properly
Ending a nested job is no different from ending any other (non-async)
job, after all the code in qemuDomainBeginJobInternal does not handle
them differently either. Thus we should call qemuDomainObjEndJob to stop
nested jobs.
Peter Krempa [Fri, 12 Feb 2016 07:24:07 +0000 (08:24 +0100)]
qemu: qemuDomainGetStatsVcpu: Fix output for possible sparse vCPU settings
qemuDomainHelperGetVcpus would correctly return an array of
virVcpuInfoPtr structs for online vcpus even for sparse topologies, but
the loop that fills the returned typed parameters would number the vcpus
incorrectly. Fortunately sparse topologies aren't supported yet.
Peter Krempa [Fri, 12 Feb 2016 05:15:47 +0000 (06:15 +0100)]
virsh: cmdVcpuPin: Simplify handling of API flags
Rather than setting flags to -1 if none were specified, move the logic
to use the old API to the place where we need to decide. It simplifies
the logic a bit.
Andrea Bolognani [Fri, 19 Feb 2016 12:25:38 +0000 (13:25 +0100)]
test: qemuxml2argv: Drop QEMU_CAPS_DEVICE uses
Since commit 51045df01b3c, the QEMU_CAPS_DEVICE capability is enabled
automatically and shouldn't be passed as an argument to DO_TEST();
however, commit 998a936c4c1a accidentally introduced few such uses.
Erik Skultety [Thu, 18 Feb 2016 13:39:18 +0000 (14:39 +0100)]
admin: Fix memory leak in remoteAdminConnectClose
When virt-admin is run with valgrind, this kind of output can be obtained:
HEAP SUMMARY:
in use at exit: 134,589 bytes in 1,031 blocks
total heap usage: 2,667 allocs, 1,636 frees, 496,755 bytes allocated
88 bytes in 1 blocks are definitely lost in loss record 82 of 128
at 0x4C2A9C7: calloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
by 0x52F6D1F: virAllocVar (viralloc.c:560)
by 0x5350268: virObjectNew (virobject.c:193)
by 0x53503E0: virObjectLockableNew (virobject.c:219)
by 0x4E3BBCB: virAdmConnectNew (datatypes.c:832)
by 0x4E38495: virAdmConnectOpen (libvirt-admin.c:209)
by 0x10C541: vshAdmConnect (virt-admin.c:107)
by 0x10C7B2: vshAdmReconnect (virt-admin.c:163)
by 0x10CC7C: cmdConnect (virt-admin.c:298)
by 0x110838: vshCommandRun (vsh.c:1224)
by 0x10DFD8: main (virt-admin.c:862)
LEAK SUMMARY:
definitely lost: 88 bytes in 1 blocks
indirectly lost: 0 bytes in 0 blocks
possibly lost: 0 bytes in 0 blocks
still reachable: 134,501 bytes in 1,030 blocks
suppressed: 0 bytes in 0 blocks
This is because virNetClientSetCloseCallback was being reinitialized
incorrectly. By resetting the callbacks in a proper way, the leak is fixed.
Matthias Bolte [Mon, 15 Feb 2016 20:17:49 +0000 (21:17 +0100)]
esx: Avoid using vSphere SessionIsActive function
A login session with the vSphere API might expire after some idle time.
The esxVI_EnsureSession function uses the SessionIsActive function to
check if the current session has expired and a relogin needs to be done.
But the SessionIsActive function needs the Sessions.ValidateSession
privilege that is considered as an admin level privilege.
Only vCenter actually provides the SessionIsActive function. This results
in requiring an admin level privilege even for read-only operations on
a vCenter server.
ESX and VMware Server don't provide the SessionIsActive function and
the code already works around that. Use the same workaround for vCenter
again.
Ján Tomko [Thu, 11 Feb 2016 09:55:07 +0000 (10:55 +0100)]
Error out on missing machine type in machine configs
Commit f1a89a8 allowed parsing configs from /etc/libvirt
without validating the emulator capabilities.
Check for the presence of os->type.machine even if the
VIR_DOMAIN_DEF_PARSE_SKIP_OSTYPE_CHECKS flag is set,
otherwise the daemon can crash on carelessly crafted input
in the config directory.
John Ferlan [Wed, 17 Feb 2016 21:23:46 +0000 (16:23 -0500)]
qemu: Rename qemuBuildSmpArgStr to qemuBuildSmpCommandLine
Rename function and move code in from qemuBuildCommandLine to
keep smp related code together. Also make a few style changes
for long lines, return value change, and 2 spaces between functions.
John Ferlan [Mon, 15 Feb 2016 21:55:08 +0000 (16:55 -0500)]
qemu: Make basic upfront checks before create command
Create qemuBuildCommandLineValidate to make some checks before trying
to build the command. This will move some logic from much later to much
earlier - we shouldn't be adjusting any data so that shouldn't matter.
Peter Krempa [Tue, 28 Jul 2015 13:34:58 +0000 (15:34 +0200)]
virsh: Remove <backingStore> when changing cdrom media source
Since the code is changing the source image path by modifying the
existing XML snippet the <backingStore> stays in place.
As <backingStore> is relevant to the <source> part of the image, the
update of that part makes the element invalid.
CD/floppy images usually don't have a backing chain and the element is
currently ignored though but it might start being used in the future so
let's start behaving correctly.
Drop the <backingStore> subtree once we want to update the XML.
Before this patch, you'd get:
$ virsh change-media --eject --print-xml 10 hdc
<disk type="file" device="cdrom">
<driver name="qemu" type="qcow2"/>
Cole Robinson [Wed, 17 Feb 2016 14:10:28 +0000 (09:10 -0500)]
tests: Remove unused virtTestClearLineRegex
This was only used for test 'xml blanking', which has now all
been removed, and isn't an ideal paradigm anyways since it
inhibits easy XML regeneration.
This is not valid XML, as either a UUID or usage must be specified in
the secret block. It's not clear though how the argv2xml code can do
anything correct here, since XML like this requires a libvirt secret
object to have already been defined.
The current test suite handles this by blanking out any <secret> block
in the XML. This avoids domainschematest failures.
Instead of blanking, let's hardcode a usage= name. This lets us test
the other bits of generated <secret> XML, and is a step towards wiring
up VIR_TEST_REGENERATE_OUTPUT
Peter Krempa [Wed, 17 Feb 2016 12:10:11 +0000 (13:10 +0100)]
qemu: Remove unnecessary calculations in qemuDomainSaveMemory
Now that the file migration doesn't require us to use 'dd' and other
legacy stuff for too old qemus we don't even have to calcuate the
offsets and other stuff.
Michal Privoznik [Wed, 17 Feb 2016 13:25:35 +0000 (14:25 +0100)]
vircgroup: Update virCgroupDenyDevicePath stub
In cf113e8d we changed the declaration of
virCgroupAllowDevicePath() and virCgroupDenyDevicePath().
However, while updating the stub for non-cgroup platforms for the
former we forgot to update the latter too causing a build
failure.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
According to the original commit message, this is dead code:
It is highly unlikely that a backend will know how to create a
volume from a different volume (buildVolFrom) and not know how to
create an empty volume (createVol).
Erik Skultety [Fri, 14 Aug 2015 07:17:01 +0000 (09:17 +0200)]
admin: Introduce adminDaemonConnectListServers API
This API is merely a convenience API, i.e. when managing clients connected to
daemon's servers, we should know (convenience) which server the specific client
is connected to. This implies a client-side representation of a server along
with a basic API to let the administrating client know what servers are actually
available on the daemon.
Signed-off-by: Erik Skultety <eskultet@redhat.com> Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
Erik Skultety [Thu, 13 Aug 2015 14:20:27 +0000 (16:20 +0200)]
admin: Introduce virAdmServer structure
This is the key structure of all management operations performed on the
daemon/clients. An admin client needs to be able to identify
another client (either admin or non-privileged client) to perform an
action on it. This identification includes a server the client is
connected to, thus a client-side representation of a server is needed.
Signed-off-by: Erik Skultety <eskultet@redhat.com>
Erik Skultety [Mon, 10 Aug 2015 10:39:33 +0000 (12:39 +0200)]
admin: Move admin_server.{h,c} to admin.{h,c}
This change is merely because admin_server would contain all the code
from dispatchers and helpers to the actual APIs. Admin should have
similar structure to the daemon-side remote driver - dispatchers and
helpers in a separate module, APIs in a separate module.
Best viewed with -M.
Signed-off-by: Erik Skultety <eskultet@redhat.com> Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
Erik Skultety [Mon, 10 Aug 2015 11:01:44 +0000 (13:01 +0200)]
virnetdaemon: Store servers in a hash table
Since the daemon can manage and add (at fresh start) multiple servers,
we also should be able to add them from a JSON state file in case of a
daemon restart, so post exec restart support for multiple servers is also
provided. Patch also updates virnetdaemontest accordingly.
Signed-off-by: Erik Skultety <eskultet@redhat.com> Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
Erik Skultety [Fri, 12 Feb 2016 09:15:57 +0000 (10:15 +0100)]
util: Refactor virHashForEach so it returns as soon as an iterator fails
The method will now return 0 on success and -1 on error, rather than number of
items which it iterated over before it returned back to the caller. Since the
only place where we actually check the number of elements iterated is in
virhashtest, return value of 0 and -1 can be a pretty accurate hint that it
iterated over all the items. However, if we really want to know the number of
items iterated over (like virhashtest does), a counter has to be provided
through opaque data to each iterator call. This patch adjusts return value of
virHashForEach, refactors the body, so it returns as soon as one of the
iterators fail and adjusts virhashtest to reflect these changes.
Signed-off-by: Erik Skultety <eskultet@redhat.com>
Erik Skultety [Fri, 12 Feb 2016 09:03:50 +0000 (10:03 +0100)]
util: Add a return value to void hash iterators
Our existing virHashForEach method iterates through all items disregarding the
fact, that some of the iterators might have actually failed. Errors are usually
dispatched through an error element in opaque data which then causes the
original caller of virHashForEach to return -1. In that case, virHashForEach
could return as soon as one of the iterators fail. This patch changes the
iterator return type and adjusts all of its instances accordingly, so the
actual refactor of virHashForEach method can be dealt with later.
Signed-off-by: Erik Skultety <eskultet@redhat.com>
Peter Krempa [Tue, 16 Feb 2016 13:43:41 +0000 (14:43 +0100)]
util: cgroup: Allow ignoring EACCES in virCgroup(Allow|Deny)DevicePath
When adding disk images to ACL we may call those functions on NFS
shares. In that case we might get an EACCES, which isn't really relevant
since NFS would not hold a block device. This patch adds a flag that
allows to stop reporting an error on EACCES to avoid spaming logs.
Peter Krempa [Tue, 16 Feb 2016 12:57:10 +0000 (13:57 +0100)]
util: cgroup: Drop virCgroup(Allow|Deny)DeviceMajor
Since commit 47e5b5ae virCgroupAllowDevice allows to pass -1 as either
the minor or major device number and it automatically uses '*' in place
of that. Reuse the new approach through the code and drop the duplicated
functions.
Peter Krempa [Mon, 15 Feb 2016 16:17:02 +0000 (17:17 +0100)]
qemu: migration: Refactor code now that we assume support for fd migration
After removing capability check for fd migration the code that was left
behind didn't make quite sense. The old exec migration would be used in
case when pipe() failed. Remove the old code and make failure of pipe()
a hard error.
This additionally removes usage of virCgroupAllowDevicePath outside of
qemu_cgroup.c.
Andrea Bolognani [Tue, 16 Feb 2016 16:41:47 +0000 (17:41 +0100)]
conf: Use a temporary int variable to store GIC version
Since no value in the virGICVersion enumeration is negative, a clever
enough compiler can report an error such as
src/conf/domain_conf.c:15337:75: error: comparison of unsigned enum
expression < 0 is always false [-Werror,-Wtautological-compare]
if ((def->gic_version = virGICVersionTypeFromString(tmp)) < 0 ||
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^ ~
virGICVersionTypeFromString() can, however, return a negative value if
the input string is not part of the enumeration, so we definitely need
that check.
Work around the problem by storing the return value in a temporary int
variable.
John Ferlan [Mon, 15 Feb 2016 18:08:02 +0000 (13:08 -0500)]
qemu: Move qemuDomain*Address* functions
Create new modules qemu_domain_address.c and qemu_domain_address.h to
contain all the new functions and header data. Additionally move any
supporting static functions.
Make qemuDomainSupportsPCI non static.
Also, move and rename the following:
qemuSetSCSIControllerModel to qemuDomainSetSCSIControllerModel
qemuCollectPCIAddress to qemuDomainCollectPCIAddress
qemuValidateDevicePCISlotsPIIX3 to qemuDomainValidateDevicePCISlotsPIIX3
qemuAssignDevicePCISlots to qemuDomainAssignDevicePCISlots
John Ferlan [Mon, 15 Feb 2016 15:52:50 +0000 (10:52 -0500)]
qemu: Move qemuNetworkIfaceConnect to qemu_interface.c and rename
Move the misplaced function from qemu_command.c to qemu_interface.c
since it's closer in functionality there and had less to do with building
the command line.
Rename function to qemuInterfaceBridgeConnect and modify callers.
John Ferlan [Mon, 15 Feb 2016 15:26:40 +0000 (10:26 -0500)]
qemu: Move qemuPhysIfaceConnect to qemu_interface.c and rename
Move the misplaced function from qemu_command.c to qemu_interface.c
since it's closer in functionality there and had less to do with building
the command line.
Rename function to qemuInterfaceDirectConnect and modify callers.
Test all kinds of scenarios, including guests asking for GIC but
failing to specify a version, guests specifying an invalid version
and guests trying to use GIC with non-virt or even non-ARM machines.
Unify the naming to prepare for new test cases that will be added
later on.
Convert a couple of output XML files for the qemuxml2xml test to
symlinks while at it, since they were identical to the corresponding
input XML files anyways.
Moreover, since we're only interested in testing GIC support here,
simplify XML files by getting rid of the unrelevant bits.
This change allows to use "host" as a GIC version in the domain XML.
Since we'll need to update the virGICVersion enumeration to support
new GIC versions anyway, it makes sense to be a bit more strict in
the schema as well and reject values that are not in the enumeration.
We currently blindly accept any numeric value as a GIC version, even
though only GIC v2 and GIC v3 actually exist; on the other hand, we
reject "host", which is a perfectly legitimate value for QEMU guests.
This new enumeration contains all GIC versions libvirt is aware of.
Currently, on hot unplug of PCI devices with VFIO driver for QEMU, libvirt is
trying to restore the host devices to it's previous value (basically a chown
on the previous user/group).
However for devices with VFIO driver, when the device is unbinded it is
removed from the /dev/vfio file system causing the restore label to fail.
The fix is to not restore the label for those PCI devices since they are going
to be teared down anyway.
Signed-off-by: Ludovic Beliveau <ludovic.beliveau@windriver.com> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Peter Krempa [Fri, 12 Feb 2016 13:09:02 +0000 (14:09 +0100)]
vsh: Replace vshPrint macro with function
The macro would eat the first parameter. In some cases the format string
for vshPrint was eaten. In other cases the calls referenced variables
which did not exist in the given context. Avoid errors by doing compile
time checking.
Peter Krempa [Fri, 12 Feb 2016 13:00:28 +0000 (14:00 +0100)]
vsh: Simplify bailing out on OOM conditions
When we hit OOM it doesn't really make sense to format the error message
by attempting to allocate it. Introduce a simple helper that prints a
static message and terminates the execution.
Laine Stump [Thu, 11 Feb 2016 20:24:17 +0000 (15:24 -0500)]
util: clean up and expand 802.1QbX negotiation logging
The existing log messages for this have several problems; there are
two lines of log when one will suffice, they duplicate the function
name in log message (when it's already included by VIR_DEBUG), they're
missing some useful bits, they get logged even when the call is a NOP.
This patch cleans up the problems with those existing logs, and also
adds a new VIR_INFO-level log down at the function that is actually
creating and sending the netlink message that logs *everything* going
into the netlink message (which turns out to be much more useful in
practice for me; I didn't want to eliminate the logs at the existing
location though, in case they are useful in some scenario I'm
unfamiliar with; anyway those logs are remaining at debug level, so it
shouldn't be a bother to anyone).
Laine Stump [Tue, 9 Feb 2016 17:28:48 +0000 (12:28 -0500)]
network: consolidated info log for all network allocate/free operations
There are three functions that deal with allocating and freeing
devices from a networks netdev/pci device pool:
network(Allocate|Notify|Release)ActualDevice(). These functions also
maintain a counter of the number of domains currently using a network
(regardless of whether or not that network uses a device pool). Each
of these functions had multiple log messages (output using VIR_DEBUG)
that were in slightly different formats and gave varying amounts of
information.
This patch creates a single function to log the pertinent information
in a consistent manner for all three of these functions. Along with
assuring that all the functions produce a consistent form of output
(and making it simpler to change), it adds the MAC address of the
domain interface involved in the operation, making it possible to
verify which interface of which domain the operation is being done for
(assuming that all MAC addresses are unique, of course).
All of these messages are raised from DEBUG to INFO, since they don't
happen that often (once per interface per domain/libvirtd start or
domain stop), and can be very informative and helpful - eliminating
the need to log debug level messages makes it much easier to sort
these out.