This patch just covers the simple functions without explicit return
values. There is more to be handled.
The generator collects the members of the XDR argument structs and uses
this information to generate the function bodies.
Exclude the generated files from offending syntax-checks.
Suggested by Richard W.M. Jones
Clang found three instances of uninitialized use of nparams in
the cleanup path. Unfortunately, one is a false positive: clang
couldn't see that ret->params.params_val is guaranteed to be
NULL unless allocated within a function, and that nparams is
guaranteed to be assigned prior to the allocation; hoisting the
assignment to nparams to be earlier in the function shuts up
that false positive. But two of the reports also happened to
highlight a real bug - the error path can dereference NULL.
Regression introduced in commit 158ba873.
* daemon/remote.c (remoteDispatchDomainGetMemoryParameters)
(remoteDispatchDomainGetBlkioParameters): Don't clear fields if
array was not allocated.
(remoteDispatchDomainGetSchedulerParameters): Initialize nparams
earlier.
Replace some occurrances of
virDomainPtr domain;
virNetworkPtr network;
With
virDomainPtr dom;
virNetworkPtr net;
* daemon/remote.c: Fix variable naming to follow standard
Replace cases of
type = virConnectGetType(conn);
if (type == NULL)
goto cleanup;
With
if (!(type = virConnectGetType(conn)))
goto cleanup;
* daemon/remote.c: Write error checks in compat form
Replace all occurrances of
if (....) {
goto cleanup;
}
With
if (.....)
goto cleanup;
to save one line of code
* daemon/remote.c: Remove curly braces on single line conditionals
The libvirt APIs reserve any negative value for indicating an
error. Thus checks
if (virXXXX() == -1)
Should instead be
if (virXXXX() < 0)
* daemon/remote.c: s/ == -1/ < 0/
The dispatcher functions have numerous places where they
return to the caller. This leads to duplicated cleanup
code, often resulting in memory leaks. It makes it harder
to ensure that errors are dispatched before freeing objects,
which may overwrite the original error.
The standard pattern is now
remoteDispatchXXX(...) {
int rv = -1;
....
if (XXX < 0)
goto cleanup;
...
if (XXXX < 0)
goto cleanup;
...
rv = 0;
cleanup:
if (rv < 0)
remoteDispatchError(rerr);
...free all other stuff..
return rv;
}
* daemon/remote.c: Centralize all cleanup paths
* daemon/stream.c: s/remoteDispatchConnError/remoteDispatchError/
* daemon/dispatch.c, daemon/dispatch.h: Replace
remoteDispatchConnError with remoteDispatchError
removing unused virConnectPtr
The daemon dispatcher code had an obsolete macro
#define REMOTE_DEBUG(fmt, ...) VIR_DEBUG(fmt, __VA_ARGS__)
This can be trivially removed
* daemon/remote.c: s/REMOTE_DEBUG/VIR_DEBUG/
Many functions did not check for whether a connection was
open. Replace the macro which obscures control flow, with
explicit checks, and ensure all dispatcher code has checks.
* daemon/remote.c: Add connection checks
A lot of code in libvirtd's dispatcher used the style
dom = get_nonnull_domain (conn, args->dom);
Instead of the normal libvirt style
dom = get_nonnull_domain(conn, args->dom);
* daemon/remote.c: Remove all whitelist before function brackets
Commit f44bfb7 was supposed to make sure no additional libvirt API (esp.
*Free) is called before remoteDispatchConnError() is called on error.
However, the patch missed two instances.
Child processes don't always reach _exit(); if they die from a
signal, then any messages should still be accurate. Most users
either expect a 0 status (thankfully, if status==0, then
WIFEXITED(status) is true and WEXITSTATUS(status)==0 for all
known platforms) or were filtering on WIFEXITED before printing
a status, but a few were missing this check. Additionally,
nwfilter_ebiptables_driver was making an assumption that works
on Linux (where WEXITSTATUS shifts and WTERMSIG just masks)
but fails on other platforms (where WEXITSTATUS just masks and
WTERMSIG shifts).
* src/util/command.h (virCommandTranslateStatus): New helper.
* src/libvirt_private.syms (command.h): Export it.
* src/util/command.c (virCommandTranslateStatus): New function.
(virCommandWait): Use it to also diagnose status from signals.
* src/security/security_apparmor.c (load_profile): Likewise.
* src/storage/storage_backend.c
(virStorageBackendQEMUImgBackingFormat): Likewise.
* src/util/util.c (virExecDaemonize, virRunWithHook)
(virFileOperation, virDirCreate): Likewise.
* daemon/remote.c (remoteDispatchAuthPolkit): Likewise.
* src/nwfilter/nwfilter_ebiptables_driver.c (ebiptablesExecCLI):
Likewise.
Bug https://bugzilla.redhat.com/show_bug.cgi?id=689374 reported libvirtd
crash during error dispatch.
The reason is that libvirtd uses remoteDispatchConnError() with non-NULL
conn parameter which means that virConnGetLastError() is used instead of
its thread safe replacement virGetLastError().
So when several libvirtd threads are reporting errors at the same time,
the errors can get mixed or corrupted or in case of bad luck libvirtd
itself crashes.
Since Daniel B. is going to rewrite this code from scratch on top of his
RPC infrastructure, I tried to come up with a minimal fix. Thus,
remoteDispatchConnError() now just ignores its conn argument and always
calls virGetLastError(). However, several callers had to be touched as
well, since no libvirt API is allowed to be called before dispatching
the error. Doing so would reset the error and we would have nothing to
dispatch. As a result of that, the code is not very nice but that
doesn't really make daemon/remote.c worse than it is now :-) And it will
all die soon, which is good.
The bug report also contains a reproducer in C which detects both mixed
up error messages and libvirtd crash. Before this patch, I was able to
crash libvirtd in about 20 seconds up to 3 minutes depending on number
of CPU cores (more is better) and luck.
Done mechanically with:
$ git grep -l '\bDEBUG0\? *(' | xargs -L1 sed -i 's/\bDEBUG0\? *(/VIR_&/'
followed by manual deletion of qemudDebug in daemon/libvirtd.c, along
with a single 'make syntax-check' fallout in the same file, and the
actual deletion in src/util/logging.h.
* src/util/logging.h (DEBUG, DEBUG0): Delete.
* daemon/libvirtd.h (qemudDebug): Likewise.
* global: Change remaining clients over to VIR_DEBUG counterpart.
To make it easier to investigate problems with async event
delivery, add two more debugging lines
* daemon/remote.c: Debug when an event is queued for dispatch
* src/remote/remote_driver.c: Debug when an event is received
for processing
This provides an implementation of the virDomainOpenConsole
API for the remote driver client and server.
* daemon/remote.c: Server side impl
* src/remote/remote_driver.c: Client impl
* src/remote/remote_protocol.x: Wire definition
Commit e8066d53 broke the build with polkit0:
remote.c: In function 'remoteDispatchAuthPolkit':
remote.c:4177: error: 'rv' undeclared (first use in this function)
Add missing identifier.
Revert most of commit a8b5f9bd27.
The audit hooks will be re-added directly in the QEMU driver code
in a future commit
* daemon/remote.c: Remove all audit logging hooks
* src/qemu/qemu_driver.c: Remove all audit logging hooks
With SystemTap 1.0 a part of the generated macros in probes.h
expands to:
volatile __typeof__(((name))) arg2 = (name);
GCC reports an 'invalid initialize' error when name has type
char[]. Therfore, add casts to char* to avoid this.
It is useful to know where the client is connecting from,
so include the socket address in probe data.
* daemon/libvirtd.h: Use virSocketAddr for storing client
address and keep printable address handy for logging
* daemon/libvirtd.c: Include socket address in client
connect/disconnect probes
* daemon/probes.d: Add socket address to probes
* examples/systemtap/client.stp: Print socket address
* src/util/network.h: Add sockaddr_un to virSocketAddr union
Adds initial support for dtrace static probes in libvirtd
daemon, assuming use of systemtap dtrace compat shim on
Linux. The probes are inserted for network client connect,
disconnect, TLS handshake states and authentication protocol
states.
This can be tested by running the xample program and then
attempting to connect with any libvirt client (virsh,
virt-manager, etc).
# stap examples/systemtap/client.stp
Client fd=44 connected readonly=0
Client fd=44 auth polkit deny pid:24997,uid:500
Client fd=44 disconnected
Client fd=46 connected readonly=1
Client fd=46 auth sasl allow test
Client fd=46 disconnected
The libvirtd.stp file should also really not be required,
since it is duplicated info that is already available in
the main probes.d definition file. A script to autogenerate
the .stp file is needed, either in libvirtd tree, or better
as part of systemtap itself.
* Makefile.am: Add examples/systemtap subdir
* autobuild.sh: Disable dtrace for mingw32
* configure.ac: Add check for dtrace
* daemon/.gitignore: Ignore generated dtrace probe file
* daemon/Makefile.am: Build dtrace probe header & object
files
* daemon/libvirtd.stp: SystemTAP convenience probeset
* daemon/libvirtd.c: Add connect/disconnect & TLS probes
* daemon/remote.c: Add SASL and PolicyKit auth probes
* daemon/probes.d: Master probe definition
* daemon/libvirtd.h: Add convenience macro for probes
so that compilation is a no-op when dtrace is not available
* examples/systemtap/Makefile.am, examples/systemtap/client.stp
Example systemtap script using dtrace probe markers
* libvirt.spec.in: Enable dtrace on F13/RHEL6
* mingw32-libvirt.spec.in: Force disable dtrace
The addrToString functionality is now available via the
virSocketFormatAddrFull method.
* daemon/remote.c, src/remote/remote_driver.c: Remove
addrToString methods
If getnameinfo() with NI_NUMERICHOST set fails, there are no
grounds to expect inet_ntop to succeed, since these calls
are functionally equivalent. Remove useless inet_ntop code
in the getnameinfo() error path.
* daemon/remote.c, src/remote/remote_driver.c: Remove
calls to inet_ntop
Most operations are audited at the libvirtd level; auditing in
src/libvirt.c would result in two audit entries per operation (one in
the client, one in libvirtd).
The only exception is a domain stopping of its own will (e.g. because
the user clicks on "shutdown" inside the interface). There can often be
no client connected at the time the domain stops, so libvirtd does not
have any virConnectPtr object on which to attach an event watch. This
patch therefore adds auditing directly inside the qemu driver (other
drivers are not supported).
The addrToString methods were not coping with UNIX domain sockets
which have no normal host+port address. Hardcode special handling
for these so that SASL routines can work over UNIX sockets. Also
fix up SSF logic in remote client so that it presumes that a UNIX
socket is secure
* daemon/remote.c: Fix addrToString for UNIX sockets.
* src/remote/remote_driver.c: Fix addrToString for UNIX sockets
and fix SSF logic to work for TLS + UNIX sockets in the same
manner
Refactor some daemon code to facilitate the introductioin of static
probes, sanitizing function exit paths in many places
* daemon/libvirtd.c: Pass the dname string into remoteCheckDN
to let caller deal with failure paths. Add separate exit paths
to remoteCheckCertificate for auth failure vs denial. Merge
all exit paths in qemudDispatchServer to one cleanup block
* daemon/remote.c: Add separate exit paths to SASL & PolicyKit
functions for auth failure vs denial
Since we are adding a new "per-hypervisor" protocol, we
make it so that the qemu remote protocol uses a new
PROTOCOL and PROGRAM number. This allows us to easily
distinguish it from the normal REMOTE protocol.
This necessitates changing the proc in remote_message_header
from a "remote_procedure" to an "unsigned", which should
be the same size (and thus preserve the on-wire protocol).
Changes since v1:
- Fixed up a couple of script problems in remote_generate_stubs.pl
- Switch an int flag to a bool in dispatch.c
Changes since v2:
- None
Changes since v3:
- Change unsigned proc to signed proc, to conform to spec
Signed-off-by: Chris Lalancette <clalance@redhat.com>
There were some major, and some minor bugs having to do with
the reference counting of node devices in daemon/remote.c.
Some functions were completely failing to unreference node devices;
this would lead to many open file descriptors, which would eventually
fail.
The minor bugs were along the same lines, but were in rarely
used error paths.
Signed-off-by: Chris Lalancette <clalance@redhat.com>
Signed-off-by: Matthias Bolte <matthias.bolte@googlemail.com>
Justin Clift reported a problem with adding virStoragePoolIsPersistent
to virsh's pool-info command, resulting in a strange problem. Here's
an example:
virsh # pool-create-as images_dir3 dir - - - - "/home/images2"
Pool images_dir3 created
virsh # pool-info images_dir3
Name: images_dir3
UUID: 90301885-94eb-4ca7-14c2-f30b25a29a36
State: running
Capacity: 395.20 GB
Allocation: 30.88 GB
Available: 364.33 GB
virsh # pool-destroy images_dir3
Pool images_dir3 destroyed
At this point the images_dir3 pool should be gone (because it was
transient) and we should be able to create a new pool with the same name:
virsh # pool-create-as images_dir3 dir - - - - "/home/images2"
Pool images_dir3 created
virsh # pool-info images_dir3
Name: images_dir3
UUID: 90301885-94eb-4ca7-14c2-f30b25a29a36
error: Storage pool not found
The new pool got the same UUID as the first one, but we didn't specify
one. libvirt should have picked a random UUID, but it didn't.
It turned out that virStoragePoolIsPersistent leaks a reference to the
storage pool object (actually remoteDispatchStoragePoolIsPersistent does).
As a result, pool-destroy doesn't remove the virStoragePool for the
"images_dir3" pool from the virConnectPtr's storagePools hash on libvirtd's
side. Then the second pool-create-as get's the stale virStoragePool object
associated with the "images_dir3" name. But this object has the old UUID.
This commit ensures that all get_nonnull_* and make_nonnull_* calls for
libvirt objects are matched properly with vir*Free calls. This fixes the
reference leaks and the reported problem.
All remoteDispatch*IsActive and remoteDispatch*IsPersistent functions were
affected. But also remoteDispatchDomainMigrateFinish2 was affected in the
success path. I wonder why that didn't surface earlier. Probably because
domainMigrateFinish2 is executed on the destination host and in the common
case this connection is opened especially for the migration and gets closed
after the migration is done. So there was no chance to run into a problem
because of the leaked reference.
Define the wire format for the new virDomainCreateWithFlags
API, and implement client and server side of marshaling code.
* daemon/remote.c (remoteDispatchDomainCreateWithFlags): Add
server side dispatch for virDomainCreateWithFlags.
* src/remote/remote_driver.c (remoteDomainCreateWithFlags)
(remote_driver): Client side serialization.
* src/remote/remote_protocol.x
(remote_domain_create_with_flags_args)
(remote_domain_create_with_flags_ret)
(REMOTE_PROC_DOMAIN_CREATE_WITH_FLAGS): Define wire format.
* daemon/remote_dispatch_args.h: Regenerate.
* daemon/remote_dispatch_prototypes.h: Likewise.
* daemon/remote_dispatch_table.h: Likewise.
* src/remote/remote_protocol.c: Likewise.
* src/remote/remote_protocol.h: Likewise.
* src/remote_protocol-structs: Likewise.
This introduces a new event type
VIR_DOMAIN_EVENT_ID_IO_ERROR_REASON
This event is the same as the previous VIR_DOMAIN_ID_IO_ERROR
event, but also includes a string describing the cause of
the event.
Thus there is a new callback definition for this event type
typedef void (*virConnectDomainEventIOErrorReasonCallback)(virConnectPtr conn,
virDomainPtr dom,
const char *srcPath,
const char *devAlias,
int action,
const char *reason,
void *opaque);
This is currently wired up to the QEMU block IO error events
* daemon/remote.c: Dispatch IO error events to client
* examples/domain-events/events-c/event-test.c: Watch for
IO error events
* include/libvirt/libvirt.h.in: Define new IO error event ID
and callback signature
* src/conf/domain_event.c, src/conf/domain_event.h,
src/libvirt_private.syms: Extend API to handle IO error events
* src/qemu/qemu_driver.c: Connect to the QEMU monitor event
for block IO errors and emit a libvirt IO error event
* src/remote/remote_driver.c: Receive and dispatch IO error
events to application
* src/remote/remote_protocol.x: Wire protocol definition for
IO error events
* src/qemu/qemu_monitor.c, src/qemu/qemu_monitor.h,
src/qemu/qemu_monitor_json.c: Watch for BLOCK_IO_ERROR event
from QEMU monitor
* daemon/remote.c: Server side dispatcher
* daemon/remote_dispatch_args.h, daemon/remote_dispatch_prototypes.h,
daemon/remote_dispatch_ret.h, daemon/remote_dispatch_table.h: Update
with new API
* src/remote/remote_driver.c: Client side dispatcher
* src/remote/remote_protocol.c, src/remote/remote_protocol.h: Update
* src/remote/remote_protocol.x: Define new wire protocol
While running libvirtd under valgrind and doing some
snapshot testing I noticed that we would always leak a
connection reference. The problem was actually that we
were leaking a domain reference in the libvirtd remote
snapshot code, which was in turn causing a leaked
connection reference. Fix the situation by explicitly
taking and dropping a domain reference where we need it.
Signed-off-by: Chris Lalancette <clalance@redhat.com>
The user probably doesn't care what the gai error numbers are, as
much as what the failed conversion IP address was.
* src/remote/remote_driver.c (addrToString): Mention which address
could not be converted.
* daemon/remote.c (addrToString): Likewise.
