3 out of 4 uses of gl_WARN_ADD() were incorrectly adding "" around
the argument, which in turn resulted in the argument being used
unquoted (configure had gl_positive=""-fstack-protector-all"",
rather than the intended gl_positive="-fstack-protector-all").
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
We keep forgetting that older setups don't like 'index':
CC util/libvirt_util_la-virsysinfo.lo
cc1: warnings being treated as errors
util/virstoragefile.c: In function 'virStorageSourceFindByNodeName':
util/virstoragefile.c:3804: error: declaration of 'index' shadows a global declaration [-Wshadow]
/usr/include/string.h:489: error: shadowed declaration is here [-Wshadow]
Signed-off-by: Eric Blake <eblake@redhat.com>
Among other recent changes, this includes a workaround to avoid
Clang compiler bug https://llvm.org/bugs/show_bug.cgi?id=16404
having spurious link failures.
* .gnulib: Update.
* bootstrap: Synchronize to upstream.
Signed-off-by: Eric Blake <eblake@redhat.com>
In particular, pull in gnulib's fix for the broken SIZE_MAX on s390.
* .gnulib: Update to latest.
* bootstrap: Resync to upstream.
Signed-off-by: Eric Blake <eblake@redhat.com>
The build was failing with:
CCLD lockd.la
libtool: error: can't build i686-pc-cygwin shared library unless -no-undefined is specified
Rather than add yet another $(CYGWIN_EXTRA_LDFLAGS) to all the
impacted *_la_LDFLAGS, it was easier to just pull the extra
flags into ALL libraries via AM_LDFLAGS.
Then, fix lockd_la_LDFLAGS to include AM_LDFLAGS, like all other
libraries.
Signed-off-by: Eric Blake <eblake@redhat.com>
Without XDR_CFLAGS, compilation on Cygwin fails with:
CC libvirt_driver_la-libvirt-stream.lo
In file included from libvirt-stream.c:26:0:
rpc/virnetprotocol.h:9:21: fatal error: rpc/rpc.h: No such file or directory
Signed-off-by: Eric Blake <eblake@redhat.com>
The old 32-bit-only mingw project had a mismatch in its <sys/types.h>
that omitted ssize_t, but where size_t was a different rank than
int as picked by gnulib. But now that mingw64 (both 32- and 64-bit)
is the more popular platform (Fedora has dropped mingw in favor of
mingw64), we no longer need to carry a hack to gnulib to cater to
the old mingw. This reverts part of commit 1012dc2933.
Signed-off-by: Eric Blake <eblake@redhat.com>
We can't mock tests on Mingw, which lacks dlopen() and friends;
follow the paradigms used in other mock files of conditionally
compiling nothing when not building for Linux.
Signed-off-by: Eric Blake <eblake@redhat.com>
Fix a regression in checking for realpath (which caused link
failures regarding duplicate rpl_canonicalize_file_name), and
fix the mingw build regarding unsetenv.
* .gnulib: Update to latest.
Signed-off-by: Eric Blake <eblake@redhat.com>
Pulls in several portability fixes, including the fact that gnulib
now only works on platforms with two's complement signed integers.
Also makes for a smaller delta on the next update (we are waiting
on a license change to unsetenv for the sake of mingw).
* .gnulib: Update to latest.
* bootstrap: Resync from upstream.
* tests/virstringtest.c: Drop use of obsolete probes of integer
properties.
Signed-off-by: Eric Blake <eblake@redhat.com>
We can't guarantee which 64-bit type will be used in an RPC struct;
while %lu worked on 64-bit Linux, that won't always be the type
used on all 64-bit platforms; and certainly is not right for 32-bit:
admin.c: In function 'adminDispatchClientGetInfo':
admin.c:265:25: error: format '%lu' expects argument of type 'long unsigned int', but argument 7 has type 'uint64_t {aka long long unsigned int}' [-Werror=format=]
Signed-off-by: Eric Blake <eblake@redhat.com>
Yet again, selinux has been adding const-correctness; this change
is ABI-compatible, but breaks API, which affects us when we try to
override things in our testsuite:
../../tests/securityselinuxhelper.c:307:24: error: conflicting types for 'selabel_open'
struct selabel_handle *selabel_open(unsigned int backend,
^~~~~~~~~~~~
In file included from ../../tests/securityselinuxhelper.c:32:0:
/usr/include/selinux/label.h:73:24: note: previous declaration of 'selabel_open' was here
The problem is a new 'const' prior to the second parameter.
Fix it the same way we did in commit 292d3f2d: check for the new
const at configure time.
Signed-off-by: Eric Blake <eblake@redhat.com>
%zu is not always synonymous with uint64_t; on 32-bit machines,
size_t is only 32 bits. Prefer "%lld"/'unsigned long long' when
the variable is under our control, and "%"PRIu64 when we are
stuck with 'uint64_t' from RBD.
Fixes errors such as:
../../src/storage/storage_backend_rbd.c: In function 'virStorageBackendRBDVolWipe':
../../src/storage/storage_backend_rbd.c:1281:15: error: format '%zu' expects argument of type 'size_t', but argument 8 has type 'uint64_t {aka long long unsigned int}' [-Werror=format=]
VIR_DEBUG("Need to wipe %zu bytes from RBD image %s/%s",
^
../../src/util/virlog.h:90:73: note: in definition of macro 'VIR_DEBUG_INT'
virLogMessage(src, VIR_LOG_DEBUG, filename, linenr, funcname, NULL, __VA_ARGS__)
^
../../src/storage/storage_backend_rbd.c:1281:5: note: in expansion of macro 'VIR_DEBUG'
VIR_DEBUG("Need to wipe %zu bytes from RBD image %s/%s",
^
Signed-off-by: Eric Blake <eblake@redhat.com>
Ever since commit ace4aecd, running 'make check' on RHEL 6 produces:
./test-lib.sh: line 21: realpath: command not found
for every shell script test, because 'realpath' was not part of
coreutils back then.
* tests/test-lib.sh (_scriptdir): Compute with only portable shell.
Signed-off-by: Eric Blake <eblake@redhat.com>
Required for the copyright year bump to keep 'make syntax-check'
happy, and also pulls in several portability fixes.
* .gnulib: Update to latest.
* bootstrap: Resync from upstream.
* gnulib/local/m4/ssize_t.m4.diff: Regenerate.
Signed-off-by: Eric Blake <eblake@redhat.com>
Cygwin cannot build the vbox driver yet:
CC vbox/libvirt_driver_vbox_impl_la-vbox_glue.lo
In file included from vbox/vbox_glue.c:27:0:
vblox/vbox_XPCOMCGlue.c:63:3: error: #error "Port me"
# error "Port me"
^
In file included from vbox/vbox_XPCOMCGlue.c:45:0,
from vbox/vbox_glue.c:27:
vbox/vbox_XPCOMCGlue.c: In function 'tryLoadOne':
vbox/vbox_XPCOMCGlue.c:98:46: error: 'DYNLIB_NAME' undeclared (first use in this function)
if (virAsprintf(&name, "%s/%s", dir, DYNLIB_NAME) < 0)
^
./util/virstring.h:245:31: note: in definition of macro 'virAsprintf'
strp, __VA_ARGS__)
^
Rather than trying to figure out how to get dynamic loading of
vbox to work under cygwin (since I don't even have a working vbox
setup to test whether it works), I'm going to be lazy and just
default to not even trying vbox on cygwin.
The libvirt file system storage driver determines what file to
act on by concatenating the pool location with the volume name.
If a user is able to pick names like "../../../etc/passwd", then
they can escape the bounds of the pool. For that matter,
virStoragePoolListVolumes() doesn't descend into subdirectories,
so a user really shouldn't use a name with a slash.
Normally, only privileged users can coerce libvirt into creating
or opening existing files using the virStorageVol APIs; and such
users already have full privilege to create any domain XML (so it
is not an escalation of privilege). But in the case of
fine-grained ACLs, it is feasible that a user can be granted
storage_vol:create but not domain:write, and it violates
assumptions if such a user can abuse libvirt to access files
outside of the storage pool.
Therefore, prevent all use of volume names that contain "/",
whether or not such a name is actually attempting to escape the
pool.
This changes things from:
$ virsh vol-create-as default ../../../../../../etc/haha --capacity 128
Vol ../../../../../../etc/haha created
$ rm /etc/haha
to:
$ virsh vol-create-as default ../../../../../../etc/haha --capacity 128
error: Failed to create vol ../../../../../../etc/haha
error: Requested operation is not valid: volume name '../../../../../../etc/haha' cannot contain '/'
Signed-off-by: Eric Blake <eblake@redhat.com>
Without this, building on cygwin fails with:
CC libvirt_admin_la-libvirt-admin.lo
libvirt-admin.c:25:21: fatal error: rpc/rpc.h: No such file or directory
#include <rpc/rpc.h>
^
Reported-by: Yaakov Selkowitz <yselkowi@redhat.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
Ever since commit e44b0269, 64-bit mingw compilation fails with:
../../src/util/virprocess.c: In function 'virProcessGetPids':
../../src/util/virprocess.c:628:50: error: passing argument 4 of 'virStrToLong_i' from incompatible pointer type [-Werror=incompatible-pointer-types]
if (virStrToLong_i(ent->d_name, NULL, 10, &tmp_pid) < 0)
^
In file included from ../../src/util/virprocess.c:59:0:
../../src/util/virstring.h:53:5: note: expected 'int *' but argument is of type 'pid_t * {aka long long int *}'
int virStrToLong_i(char const *s,
^
cc1: all warnings being treated as errors
Although mingw won't be using this function, it does compile the
file, and the fix is relatively simple.
* src/util/virprocess.c (virProcessGetPids): Don't assume pid_t
fits in int.
Signed-off-by: Eric Blake <eblake@redhat.com>
We already enable the parser option to detect invalid UTF-8, but
didn't test it. Also, JSON states that behavior of an object
with a duplicated key is undefined; we chose to reject it, but
were not testing it.
With the enhanced tests in place, we can simplify yajl2
initialization by relying on parser defaults being sane.
* src/util/virjson.c (virJSONValueFromString): Simplify.
* tests/jsontest.c (mymain): Test more bad usage.
Signed-off-by: Eric Blake <eblake@redhat.com>
Since older yajl ignores trailing garbage, a client can cause
problems by intentionally ending the wrapper array early. Since
we already track nesting, it's not too much harder to reject
invalid nesting pops.
* src/util/virjson. (_virJSONParser): Add field.
(virJSONValueFromString): Set witness.
(virJSONParserHandleEndArray): Use it to catch abuse.
* tests/jsontest.c (mymain): Test it.
Signed-off-by: Eric Blake <eblake@redhat.com>
Yajl 2 has a nice feature that it can be configured whether to
allow multiple JSON objects parsed from a single stream, defaulting
to off. And yajl 1.0.12 at least provided a way to tell if all
input bytes were parsed, or if trailing bytes remained after a
valid JSON object was parsed. But we target RHEL 6 yajl 1.0.7,
which has neither of these. So fake it by always parsing '[...]'
instead, so that trailing garbage either trips up the array parse,
or is easily detected when unwrapping the result.
* src/util/virjson.c (virJSONValueFromString): With older json,
wrap text to avoid trailing garbage.
* tests/jsontest.c (mymain): Add tests for this.
Signed-off-by: Eric Blake <eblake@redhat.com>
We have been allowing javascript style comments in JSON ever
since commit 9428f2c (v0.7.5), but qemu doesn't send them, and
they are not strict JSON. Reject them for now; if we can later
prove that it is worthwhile, we can reinstate it at that point
(or even make it conditional, by adding a bool parameter to
the libvirt entry point).
* src/util/virjson.c (virJSONValueFromString): Don't enable
comment parsing.
* tests/jsontest.c (mymain): Test it.
Signed-off-by: Eric Blake <eblake@redhat.com>
Commit ceb496e5 fails on RHEL 6, with yajl 1.0.7, because that
version of yajl returns yajl_status_insufficient_data when the
parser is waiting for the rest of a token (this enum value was
dropped in yajl 2, so we have to wrap it). It also exposes a
problem where older yajl silently ignores trailing garbage after
a successful parse, so this patch works around that by changing
the testsuite. Another more invasive patch can add tighter
semantics to json parsing, but this is sufficient for a minimal
clean backport.
While touching this, fix up our error message cleanup. Yajl
documents that error messages produced by yajl_get_error()
MUST be cleaned with yajl_free_error(); this is certainly
true if we were to pass non-NULL allocator callbacks during
yajl_alloc(), but probably harmless in our usage of passing
NULL. But better safe than sorry.
* src/util/virjson.c (virJSONValueFromString): Allow different
error code. Use canonical cleanup of error message.
(VIR_YAJL_STATUS_OK): New helper macro.
* tests/jsontest.c (mymain): Wrap text to avoid difference in
trailing garbage handling
Signed-off-by: Eric Blake <eblake@redhat.com>
Rather than grabbing an arbitrary JSON value and then checking
if it has the right type, we might as well request the correct
type to begin with.
* src/qemu/qemu_monitor_json.c (qemuMonitorJSONIOProcessEvent)
(qemuMonitorJSONCommandWithFd, qemuMonitorJSONHandleGraphics)
(qemuMonitorJSONGetStatus, qemuMonitorJSONExtractCPUInfo)
(qemuMonitorJSONGetVirtType, qemuMonitorJSONGetBalloonInfo)
(qemuMonitorJSONGetMemoryStats)
(qemuMonitorJSONDevGetBlockExtent)
(qemuMonitorJSONGetOneBlockStatsInfo)
(qemuMonitorJSONGetAllBlockStatsInfo)
(qemuMonitorJSONBlockStatsUpdateCapacityOne)
(qemuMonitorJSONBlockStatsUpdateCapacity)
(qemuMonitorJSONGetBlockExtent)
(qemuMonitorJSONGetMigrationStatusReply)
(qemuMonitorJSONGetDumpGuestMemoryCapability)
(qemuMonitorJSONAddFd, qemuMonitorJSONQueryRxFilterParse)
(qemuMonitorJSONExtractChardevInfo)
(qemuMonitorJSONDiskNameLookupOne)
(qemuMonitorJSONDiskNameLookup)
(qemuMonitorJSONGetAllBlockJobInfo)
(qemuMonitorJSONBlockIoThrottleInfo, qemuMonitorJSONGetVersion)
(qemuMonitorJSONGetMachines, qemuMonitorJSONGetCPUDefinitions)
(qemuMonitorJSONGetCommands, qemuMonitorJSONGetEvents)
(qemuMonitorJSONGetKVMState, qemuMonitorJSONGetObjectTypes)
(qemuMonitorJSONGetObjectListPaths)
(qemuMonitorJSONGetObjectProps, qemuMonitorJSONGetTargetArch)
(qemuMonitorJSONGetMigrationCapabilities)
(qemuMonitorJSONGetStringArray, qemuMonitorJSONAttachCharDev)
(qemuMonitorJSONGetCPUx86Data, qemuMonitorJSONGetIOThreads)
(qemuMonitorJSONGetMemoryDeviceInfo): Use shorter idioms.
Signed-off-by: Eric Blake <eblake@redhat.com>
While working in qemu_monitor_json, I repeatedly found myself
getting a value then checking if it was an object. Add some
wrappers to make this task easier.
* src/util/virjson.c (virJSONValueObjectGetByType)
(virJSONValueObjectGetObject, virJSONValueObjectGetArray): New
functions.
(virJSONValueObjectGetString, virJSONValueObjectGetNumberInt)
(virJSONValueObjectGetNumberUint)
(virJSONValueObjectGetNumberLong)
(virJSONValueObjectGetNumberUlong)
(virJSONValueObjectGetNumberDouble)
(virJSONValueObjectGetBoolean): Simplify.
(virJSONValueIsNull): Change return type.
* src/util/virjson.h: Reflect changes.
* src/libvirt_private.syms (virjson.h): Export them.
* tests/jsontest.c (testJSONLookup): New test.
Signed-off-by: Eric Blake <eblake@redhat.com>
I was adding a JSON test, and was shocked to find out our parser
treated the input string of "1" as invalid JSON. It turns out
that YAJL specifically documents that it buffers input, and that
if the last input read could be a prefix to a longer token, then
you have to explicitly tell the parser that the buffer has ended
before that token will be processed.
It doesn't help that yajl 2 renamed the function from what it was
in yajl 1.
* src/util/virjson.c (virJSONValueFromString): Complete parse, in
case buffer ends in possible token prefix.
* tests/jsontest.c (mymain): Expose the problem.
Signed-off-by: Eric Blake <eblake@redhat.com>
Commit 1882c0bd accidentally used ',' instead of ';'; oddly
enough, the result was still syntactically valid (yes, C is
a fun language). But it made me do a double take; it's better
to use idiomatic syntax.
* daemon/remote.c (remoteRelayDomainEventDeviceAdded): Fix
harmless typo.
Signed-off-by: Eric Blake <eblake@redhat.com>
Newer binutils 'ar' has added an option 'D' for deterministic
builds, and at least on rawhide, this option is enabled by default.
But it conflicts with the 'u' optimization where the linker only
modifies libraries based on file timestamps, but can result in
different library ordering based on which files were touched last.
Thus, it results in some noisy compilation, for every CCLD line:
CCLD libvirt_driver_qemu_impl.la
ar: `u' modifier ignored since `D' is the default (see `U')
Upstream automake has decided that defaulting ARFLAGS to 'cru' is
no longer beneficial, and that switching the default to 'cr' will
both silence the noise and not penalize modern build systems.
https://lists.gnu.org/archive/html/automake-patches/2015-06/msg00000.html
But rather than wait for newer automake to propagate to all systems
that already have newer binutils, we might as well just use the new
default ourselves, even on older platforms.
* configure.ac: Default AR[_]FLAGS to 'cr', not 'cru'.
Signed-off-by: Eric Blake <eblake@redhat.com>
Incorporates fixes for cross-compiling to mingw on rawhide, where
gcc 5.1 changes detection of how to properly determine PRIdMAX.
Also picks up some improvements for compilation on Mac OS X.
* .gnulib: Update to latest, for at least mingw.
Signed-off-by: Eric Blake <eblake@redhat.com>
Time to update to new gnulib before a release.
gcc 5.1 introduced a new -Wformat-signedness, and new gnulib now
turns it on by default. However, it is still rather lame at the
moment, because it warns for enums, even though there is no way
to control the signeness of an enum which does not use any members
that are negative or larger than INT_MAX, and even though such an
enum would always print the same for both %d and %u:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66249
In file included from ../../src/util/virarch.c:26:0:
../../src/util/virarch.c: In function 'virArchFromHost':
../../src/util/virarch.c:180:15: error: format '%d' expects argument of type 'int', but argument 9 has type 'unsigned int' [-Werror=format=]
VIR_DEBUG("Mapped %s to %d (%s)",
So this patch turns off the new warning as part of enabling all
other new gcc 5.1 warnings that gnulib now enables.
* .gnulib: Update to latest, in part for gcc 5.1 interaction.
* m4/virt-compile-warnings.m4: Ignore -Wformat-signedness, for now.
Signed-off-by: Eric Blake <eblake@redhat.com>
gcc 4.1.2 (hello RHEL 5) on 32-bit platforms complains:
vircgrouptest.c: In function 'testCgroupGetPercpuStats':
vircgrouptest.c:627: warning: integer constant is too large for 'long' type
vircgrouptest.c:628: warning: this decimal constant is unsigned only in ISO C90
vircgrouptest.c:634: warning: integer constant is too large for 'long' type
vircgrouptest.c:635: warning: this decimal constant is unsigned only in ISO C90
vircgrouptest.c:636: warning: this decimal constant is unsigned only in ISO C90
vircgrouptest.c:644: warning: integer constant is too large for 'long' type
* tests/vircgrouptest.c (testCgroupGetPercpuStats): Use ULL suffix.
Signed-off-by: Eric Blake <eblake@redhat.com>
Commit a0670ae caused a regression in 'virsh event' and
'virsh qemu-monitor-event' - if a user tries to filter the
command to a specific domain, an error message is printed:
$ virsh event dom --loop
error: internal error: virsh qemu-monitor-event: no domain VSH_OT_DATA option
and then the command continues as though no domain had been
supplied (giving events for ALL domains, instead of the
requested one). This is because the code was incorrectly
assuming that all "domain" options would be supplied via a
mandatory VSH_OT_DATA, even though "domain" is optional for
these two commands, so we had changed them to VSH_OT_STRING
to quit failing for other reasons (ever since it was decided
that VSH_OT_DATA and VSH_OT_STRING should no longer be
synonyms).
In looking at the situation, though, the code for looking up
a domain was making a pointless check for whether the option
exists prior to finding the option's string value, as
vshCommandOptStringReq does just fine at reporting any errors
when looking up a string whether or not the option was present.
So this is a case of regression fixing by pure code deletion :)
* tools/virsh-domain.c (vshCommandOptDomainBy): Drop useless filter.
* tools/virsh-interface.c (vshCommandOptInterfaceBy): Likewise.
* tools/virsh-network.c (vshCommandOptNetworkBy): Likewise.
* tools/virsh-nwfilter.c (vshCommandOptNWFilterBy): Likewise.
* tools/virsh-secret.c (vshCommandOptSecret): Likewise.
* tools/virsh.h (vshCmdHasOption): Drop unused function.
* tools/virsh.c (vshCmdHasOption): Likewise.
Signed-off-by: Eric Blake <eblake@redhat.com>
Commit 49ed6cff is broken on mingw and other non-linux platforms:
CCLD libvirt.la
Cannot export virNetDevSysfsFile: symbol not defined
collect2: error: ld returned 1 exit status
* src/util/virnetdev.c: Provide virNetDevSysfsFile fallback.
Signed-off-by: Eric Blake <eblake@redhat.com>
Found by ./autobuild.sh during a mingw cross-compile:
Commit 8a96e87 was not innocuous - glibc happens to leak the
definition of time() through other headers, so that even without
<sys/select.h>, virrandom.c compiled just fine. But on mingw,
we were not so lucky; <sys/select.h> was important for its side
effect of dragging in <time.h>, and we now have nothing providing
the declaration of time():
../../src/util/virrandom.c: In function 'virRandomOnceInit':
../../src/util/virrandom.c:65:5: error: implicit declaration of function 'time' [-Werror=implicit-function-declaration]
unsigned int seed = time(NULL) ^ getpid();
^
../../src/util/virrandom.c:65:5: error: nested extern declaration of 'time' [-Werror=nested-externs]
Signed-off-by: Eric Blake <eblake@redhat.com>
Commit 2f36e6944 (re-)introduced a use of an identifier 'interface',
which causes this build failure on mingw:
../../tools/virsh-domain-monitor.c: In function 'cmdDomIfAddr':
../../tools/virsh-domain-monitor.c:2233:17: error: expected identifier or '(' before 'struct'
const char *interface = NULL;
^
See also commit 6512c8b. Sadly, I'm not quite sure how to write a
syntax check that can poison the use of this identifier.
* tools/virsh-domain-monitor.c (cmdDomIfAddr): Use ifacestr instead.
Signed-off-by: Eric Blake <eblake@redhat.com>
On IRC, Hydrar pointed a problem where 'virsh edit' failed on
his domain created through an ISCSI pool managed by virt-manager,
all because the XML included a block device with colons in the
name.
* docs/schemas/basictypes.rng (absFilePath): Add colon as safe.
* tests/qemuxml2argvdata/qemuxml2argv-disk-iscsi.xml: New file.
* tests/qemuxml2argvdata/qemuxml2argv-disk-iscsi.args: Likewise.
* tests/qemuxml2argvtest.c (mymain): Test it.
Signed-off-by: Eric Blake <eblake@redhat.com>
Usual update to latest gnulib status. In particular this update
fixes at least one issue that can be seen in libvirt, by silencing
a valgrind detection of uninitialized memory:
https://bugzilla.redhat.com/show_bug.cgi?id=1174147
* .gnulib: Update to latest.
* bootstrap: Resync to gnulib.
Signed-off-by: Eric Blake <eblake@redhat.com>
In qemu 2.3, the migration status will include 'cancelling' in the
window between when an asynchronous cancel has been requested and
when the migration is actually halted. Previously, qemu hid this
state and reported 'active'. Libvirt manages the sequence okay
even when the string is unrecognized (that is, it will report an
unknown state:
Migration: [ 69 %]^Cerror: internal error: unexpected migration status in cancelling.
but the migration is still cancelled), but recognizing the string
makes for a smoother user experience.
* src/qemu/qemu_monitor.h
(QEMU_MONITOR_MIGRATION_STATUS_CANCELLING): Add enum.
* src/qemu/qemu_monitor.c (qemuMonitorMigrationStatus): Map it.
* src/qemu/qemu_migration.c (qemuMigrationUpdateJobStatus): Adjust
clients.
* src/qemu/qemu_monitor_json.c
(qemuMonitorJSONGetMigrationStatusReply): Likewise.
Signed-off-by: Eric Blake <eblake@redhat.com>
https://bugzilla.redhat.com/show_bug.cgi?id=1199182 documents that
after a series of disk snapshots into existing destination images,
followed by active commits of the top image, it is possible for
qemu 2.2 and earlier to end up tracking a different name for the
image than what it would have had when opening the chain afresh.
That is, when starting with the chain 'a <- b <- c', the name
associated with 'b' is how it was spelled in the metadata of 'c',
but when starting with 'a', taking two snapshots into 'a <- b <- c',
then committing 'c' back into 'b', the name associated with 'b' is
now the name used when taking the first snapshot.
Sadly, older qemu doesn't know how to treat different spellings of
the same filename as identical files (it uses strcmp() instead of
checking for the same inode), which means libvirt's attempt to
commit an image using solely the names learned from qcow2 metadata
fails with a cryptic:
error: internal error: unable to execute QEMU command 'block-commit': Top image file /tmp/images/c/../b/b not found
even though the file exists. Trying to teach libvirt the rules on
which name qemu will expect is not worth the effort (besides, we'd
have to remember it across libvirtd restarts, and track whether a
file was opened via metadata or via snapshot creation for a given
qemu process); it is easier to just always directly ask qemu what
string it expects to see in the first place.
As a safety valve, we validate that any name returned by qemu
still maps to the same local file as we have tracked it, so that
a compromised qemu cannot accidentally cause us to act on an
incorrect file.
* src/qemu/qemu_monitor.h (qemuMonitorDiskNameLookup): New
prototype.
* src/qemu/qemu_monitor_json.h (qemuMonitorJSONDiskNameLookup):
Likewise.
* src/qemu/qemu_monitor.c (qemuMonitorDiskNameLookup): New function.
* src/qemu/qemu_monitor_json.c (qemuMonitorJSONDiskNameLookup)
(qemuMonitorJSONDiskNameLookupOne): Likewise.
* src/qemu/qemu_driver.c (qemuDomainBlockCommit)
(qemuDomainBlockJobImpl): Use it.
Signed-off-by: Eric Blake <eblake@redhat.com>
Commit 4f25146 (v1.2.8) managed to silence Coverity, but at the
cost of a memory leak detected by valgrind:
==24129== 40 bytes in 5 blocks are definitely lost in loss record 355 of 637
==24129== at 0x4A08B1C: realloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==24129== by 0x5084B8E: virReallocN (viralloc.c:245)
==24129== by 0x514D5AA: virDomainObjListExport (domain_conf.c:22200)
==24129== by 0x201227DB: qemuConnectListAllDomains (qemu_driver.c:18042)
==24129== by 0x51CC1B6: virConnectListAllDomains (libvirt-domain.c:6797)
==24129== by 0x14173D: remoteDispatchConnectListAllDomains (remote.c:1580)
==24129== by 0x121BE1: remoteDispatchConnectListAllDomainsHelper (remote_dispatch.h:1072)
In short, every time a client calls a ListAll variant and asks
for the resulting list, but there are 0 elements to return, we
end up leaking the 1-entry array that holds the NULL terminator.
What's worse, a read-only client can access these functions in a
tight loop to cause libvirtd to eventually run out of memory; and
this can be considered a denial of service attack against more
privileged clients. Thankfully, the leak is so small (8 bytes per
call) that you would already have some other denial of service with
any guest calling the API that frequently, so an out-of-memory
crash is unlikely enough that this did not warrant a CVE.
* daemon/remote.c (remoteDispatchConnectListAllDomains)
(remoteDispatchDomainListAllSnapshots)
(remoteDispatchDomainSnapshotListAllChildren)
(remoteDispatchConnectListAllStoragePools)
(remoteDispatchStoragePoolListAllVolumes)
(remoteDispatchConnectListAllNetworks)
(remoteDispatchConnectListAllInterfaces)
(remoteDispatchConnectListAllNodeDevices)
(remoteDispatchConnectListAllNWFilters)
(remoteDispatchConnectListAllSecrets)
(remoteDispatchNetworkGetDHCPLeases): Plug leak.
Signed-off-by: Eric Blake <eblake@redhat.com>
Valgrind detected a leak:
==17820== 102 (56 direct, 46 indirect) bytes in 1 blocks are definitely lost in loss record 479 of 646
==17820== at 0x4A08946: calloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==17820== by 0x508521A: virAllocVar (viralloc.c:560)
==17820== by 0x50D9FCA: virObjectNew (virobject.c:193)
==17820== by 0x50A4FD9: dnsmasqCapsNewEmpty (virdnsmasq.c:784)
==17820== by 0x50A514E: dnsmasqCapsNewFromBinary (virdnsmasq.c:830)
==17820== by 0x1B508287: networkStateInitialize (bridge_driver.c:666)
It looks like commit 172acef introduced the problem, because
networkGetDnsmasqCaps() increments the reference count but an
early exit never does a matching decrement.
* src/network/bridge_driver.c (networkStateCleanup): Plug leak.
Signed-off-by: Eric Blake <eblake@redhat.com>
Valgrind complained:
==3770== Syscall param ioctl(SIOCETHTOOL) points to uninitialised byte(s)
==3770== at 0x919D407: ioctl (syscall-template.S:81)
==3770== by 0x530FE7E: rpl_ioctl (ioctl.c:42)
==3770== by 0x50CB433: virNetDevFeatureAvailable (virnetdev.c:2764)
==3770== by 0x50CB6A7: virNetDevGetFeatures (virnetdev.c:2830)
==3770== by 0x1F0E5347: udevProcessNetworkInterface (node_device_udev.c:722)
==3770== by 0x1F0E689F: udevGetDeviceDetails (node_device_udev.c:1300)
==3770== by 0x1F0E6E06: udevAddOneDevice (node_device_udev.c:1422)
==3770== by 0x1F0E6FB8: udevProcessDeviceListEntry (node_device_udev.c:1464)
==3770== by 0x1F0E70CF: udevEnumerateDevices (node_device_udev.c:1494)
==3770== by 0x1F0E7BB4: nodeStateInitialize (node_device_udev.c:1806)
==3770== by 0x51B4303: virStateInitialize (libvirt.c:777)
==3770== by 0x11DEE7: daemonRunStateInit (libvirtd.c:906)
==3770== Address 0x228e38d4 is on thread 12's stack
==3770== in frame #2, created by virNetDevFeatureAvailable (virnetdev.c:2750)
* src/util/virnetdev.c (virNetDevFeatureAvailable): Initialize all
bytes of ifr.
Signed-off-by: Eric Blake <eblake@redhat.com>
Commit f182da20 (v1.2.6) caused a slight regression in virsh
reporting of a non-active block job; where it used to state
"Commit complete", it now states "Now in synchronized phase".
But the synchronized phase is only possible for an active commit.
For a reproducer, I created a chain 'a <- b <- c <- d <- e' and
ran virsh blockcommit $dom vda --top c --base a --verbose --wait
* tools/virsh-domain.c (cmdBlockCommit): Synchronized phase is
only possible on active commits.
Signed-off-by: Eric Blake <eblake@redhat.com>
https://bugzilla.redhat.com/show_bug.cgi?id=1135339 documents some
confusing behavior when a user tries to start an inactive block
commit in a second connection while there is already an on-going
active commit from a first connection. Eventually, qemu will
support multiple simultaneous block jobs, but as of now, it does
not; furthermore, libvirt also needs an overhaul before we can
support simultaneous jobs. So, the best way to avoid confusing
ourselves is to quit relying on qemu to tell us about the situation
(where we risk getting in weird states) and instead forbid a
duplicate block commit ourselves.
Note that we are still relying on qemu to diagnose attempts to
interrupt an inactive commit (since we only track XML of an active
commit), but as inactive commit is less confusing for libvirt to
manage, there is less that can go wrong by leaving that detection
up to qemu.
* src/qemu/qemu_driver.c (qemuDomainBlockCommit): Hoist check for
active commit to occur earlier outside of conditions.
Signed-off-by: Eric Blake <eblake@redhat.com>
I noticed this while working on a previous commit. Why should
we be calling out '../src/' when it is sufficient to refer to just
'./'? Blind copy-and-paste runs rampant in this file :)
* src/Makefile.am (INCLUDES, *_CFLAGS): Shorten to $(srcdir).
Signed-off-by: Eric Blake <eblake@redhat.com>
Ever since commit 2c78051 split out a helper library for the sake of
changing CFLAGS, a VPATH build with xenconfig enabled has failed:
CC xenconfig/libvirt_xenxldiskparser_la-xen_xl_disk.lo
../../src/xenconfig/xen_xl_disk.l:37:21: fatal error: xen_xl.h: No such file or directory
# include "xen_xl.h"
^
compilation terminated.
Makefile:9462: recipe for target 'xenconfig/libvirt_xenxldiskparser_la-xen_xl_disk.lo' failed
The solution is to tell the build to look for xen_xl.h relative
to $(srcdir), since we keep that file under version control.
[Not fixed here - the raw use of -Wno-unused-parameter in CFLAGS
is NOT portable; ideally, we should be doing a configure test
and only supplying that argument when we know the compiler supports
-Wunused-parameter; but that's a patch for another day]
[Not fixed here - there are still issues with parallel builds hitting
a race between generating the files and trying to compile/distribute
them]
* src/Makefile.am (libvirt_xenxldiskparser_la_CFLAGS): Add another
include directory.
Signed-off-by: Eric Blake <eblake@redhat.com>
Another update is required to pick up today's gnulib fix for mingw
builds (now that gnulib turns on mingw's replacement printf that
understands %lld, it must also tell the compiler to respect the
improved definition of PRIdMAX and friends).
* .gnulib: Update to latest.
Signed-off-by: Eric Blake <eblake@redhat.com>
Commit 4dc04d3a added virNetlinkGetErrorCode, but forgot to provide
a fallback, which kills the build on mingw (among others):
CCLD libvirt.la
Cannot export virNetlinkGetErrorCode: symbol not defined
collect2: error: ld returned 1 exit status
* src/util/virnetlink.c (virNetlinkGetErrorCode): Provide fallback.
Signed-off-by: Eric Blake <eblake@redhat.com>
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: Eric Blake <eblake@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: Eric 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: Eric Blake <eblake@redhat.com>
This patch introduces access to allocation information about
a backing chain of a live domain. While querying storage
volumes for read-only disks could provide some of the details,
we do NOT want to read() a file while qemu is writing it.
Also, there is one case where we have to rely on qemu: when
doing a block commit into a backing file, where that file is
stored in qcow2 format on a host block device, we want to know
the current highest write offset into that image, in order to
know if the disk must be resized larger. qemu-img does not
(currently) show this information, and none of the earlier
block APIs were extensible enough to expose it. But
virDomainListGetStats is perfect for the job!
We don't need a new group of statistics, as the existing block
group is sufficient. On the other hand, as existing libvirt
releases already report 1:1 mapping of block.count to <disk>
devices, changing the array size could confuse older clients;
and even with newer clients, the time and memory taken to
report additional statistics is not always necessary (backing
files are generally read-only except for block-commit, so while
read statistics may change, sizing statistics will not). So
the choice here is to add a new flag that only newer callers
will pass, when they are prepared for the additional information.
This patch introduces the new API, but it will take more
patches to get it implemented for qemu.
* include/libvirt/libvirt-domain.h
(VIR_CONNECT_GET_ALL_DOMAINS_STATS_BACKING): New flag.
* src/libvirt-domain.c (virConnectGetAllDomainStats): Document it,
and add a new field when it is in use.
* tools/virsh-domain-monitor.c (cmdDomstats): Use new flag.
* tools/virsh.pod (domstats): Document it.
Signed-off-by: Eric 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: Eric Blake <eblake@redhat.com>
The prior refactoring can now be put to use. With the same domain
as the earlier commit 7b49926 (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: Eric 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: Eric 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: Eric 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: Eric 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: Eric 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: Eric 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: Eric 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 b799259); 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: Eric Blake <eblake@redhat.com>
Several portability changes, but the one we are most interested in
is the improvement to bootstrap to detect perl modules.
This patch doesn't actually change our bootstrap requirements
(that will be a separate patch), but sets the stage for it.
* .gnulib: Update to latest.
* bootstrap: Regenerate from upstream.
Signed-off-by: Eric Blake <eblake@redhat.com>
Commit c75425734 introduced a compilation failure:
../../src/access/viraccessdriverpolkit.c: In function 'virAccessDriverPolkitCheck':
../../src/access/viraccessdriverpolkit.c:137:5: error: format '%d' expects argument of type 'int', but argument 9 has type 'pid_t' [-Werror=format=]
VIR_DEBUG("Check action '%s' for process '%d' time %lld uid %d",
^
Since mingw pid_t is 64 bits, it's easier to just follow what we've
done elsewhere and cast to a large enough type when printing pids.
* src/access/viraccessdriverpolkit.c (virAccessDriverPolkitCheck):
Add cast.
Signed-off-by: Eric Blake <eblake@redhat.com>
I'm about to make block stats optionally more complex to cover
backing chains, where block.count will no longer equal the number
of <disks> for a domain. For these reasons, it is nicer if the
statistics output includes the source path (for local files).
This patch doesn't add anything for network disks, although we
may decide to add that later.
With this patch, I now see the following for the same domain as
in the previous patch (one qcow2 file, 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.1.name=hdc
* src/libvirt-domain.c (virConnectGetAllDomainStats): Document
new field.
* tools/virsh.pod (domstats): Document new field.
* src/qemu/qemu_driver.c (qemuDomainGetStatsBlock): Return the new
stat for local files/block devices.
(QEMU_ADD_NAME_PARAM): Add parameter.
(qemuDomainGetStatsInterface): Update caller.
Signed-off-by: Eric Blake <eblake@redhat.com>
I noticed that for an offline domain, 'virsh domstats --block $dom'
was producing just the domain name, with no stats. But the older
'virsh domblkinfo' works just fine on offline domains. This patch
starts to get us closer, by at least reporting the disk names for
an offline domain.
With this patch, I now see the following for an offline domain
with one qcow2 disk and an empty cdrom drive:
$ virsh domstats --block foo
Domain: 'foo'
block.count=2
block.0.name=hda
block.1.name=hdc
* src/qemu/qemu_driver.c (qemuDomainGetStatsBlock): Don't short-circuit
output of block name.
Signed-off-by: Eric Blake <eblake@redhat.com>
At least with 'virsh domstats --block' on an offline domain, we
currently output no stats even though we recognize the stat
category. Although a later patch will improve this situation,
it is better to document that this is expected behavior.
Also, while the current implementation rejects filtering flags
for virDomainListGetStats, this limitation may be lifted in the
future and we do not enforce it at the API level.
* src/libvirt-domain.c (virConnectGetAllDomainStats): Document
that recognized stats might not be reported.
(virDomainListGetStats): Likewise, and tweak filtering documentation.
Signed-off-by: Eric Blake <eblake@redhat.com>
qemuDomainGetStatsBlock() could leak a stats hash table if it
encountered OOM while populating the virTypedParameters.
Oddly, the fix doesn't even touch qemuDomainGetStatsBlock :)
* src/qemu/qemu_driver.c (QEMU_ADD_COUNT_PARAM)
(QEMU_ADD_NAME_PARAM): Don't return early.
(qemuDomainGetStatsInterface): Adjust caller.
Signed-off-by: Eric Blake <eblake@redhat.com>
Commit 7557ddf added some additional block.* stats to
virDomainListGetStats, but failed to document them in 'man
virsh'. Also, I noticed some inconsistent use of commas.
* tools/virsh.pod (domstats): Tweak commas, add missing stats.
Signed-off-by: Eric Blake <eblake@redhat.com>
Commit 2aa167ca tried to fix the DBus interaction code to allow
callers to use native types instead of 4-byte bools. But in
fixing the issue, I missed the case of an arrayref; Conrad Meyer
shows the following valid complaint issued by clang:
CC util/libvirt_util_la-virdbus.lo
util/virdbus.c:956:13: error: cast from 'bool *' to 'dbus_bool_t *' (aka 'unsigned int *') increases required alignment from 1 to 4 [-Werror,-Wcast-align]
GET_NEXT_VAL(dbus_bool_t, bool_val, bool, "%d");
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
util/virdbus.c:858:17: note: expanded from macro 'GET_NEXT_VAL'
x = (dbustype *)(*xptrptr + (*narrayptr - 1)); \
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 1 error generated.
But fixing that points out that we have NEVER supported arrayrefs
of sub-int types (byte, i16, u16, and now bool). Again, while raw
types promote, arrays do not; so the macros HAVE to deal with both
size possibilities rather than assuming that an arrayref uses the
same sizing as the promoted raw type.
Obviously, our testsuite wasn't covering as much as it should have.
* src/util/virdbus.c (GET_NEXT_VAL): Also fix array cases.
(SET_NEXT_VAL): Fix uses of sub-int arrays.
* tests/virdbustest.c (testMessageArray, testMessageArrayRef):
Test it.
Signed-off-by: Eric Blake <eblake@redhat.com>
Commit c0e7022 breaks on a machine that lacks dbus headers:
In file included from util/virdbus.c:24:0:
util/virdbuspriv.h:31:3: error: unknown type name 'dbus_int16_t'
* src/util/virdbuspriv.h (DBusBasicValue): Only provide fallback
when dbus is compiled.
Signed-off-by: Eric Blake <eblake@redhat.com>
On 32-bit platforms with old gcc (hello RHEL 5 gcc 4.1.2), the
build fails with:
virsh-domain.c: In function 'cmdBlockCopy':
virsh-domain.c:2172: warning: comparison is always false due to limited range of data type
Adjust the code to silence the warning.
* tools/virsh-domain.c (cmdBlockCopy): Pacify RHEL 5 gcc.
Signed-off-by: Eric Blake <eblake@redhat.com>
Compilation on a RHEL 5 host failed, due to the older dbus headers
present on that machine, and triggered by commit 2aa167ca:
util/virdbus.c: In function 'virDBusMessageIterDecode':
util/virdbus.c:952: error: 'DBusBasicValue' undeclared (first use in this function)
* m4/virt-dbus.m4 (LIBVIRT_CHECK_DBUS): Check for DBusBasicValue.
* src/util/virdbuspriv.h (DBusBasicValue): Provide fallback.
Signed-off-by: Eric Blake <eblake@redhat.com>
Commit dc33e6e4 caused older platforms like Fedora 20 to emit
scary log messages at startup:
2014-11-19 23:12:58.800+0000: 28906: error : virCommandWait:2532 : internal error: Child process (/usr/sbin/iptables -w -L -n) unexpected exit status 2: iptables v1.4.19.1: unknown option "-w"
Try `iptables -h' or 'iptables --help' for more information.
Since we are probing and expect to handle the case where -w is not
supported, we should not let virCommand log it as an error.
* src/util/virfirewall.c (virFirewallCheckUpdateLock): Handle
non-zero status ourselves.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reported in https://bugzilla.redhat.com/show_bug.cgi?id=1165827;
dash complains:
checking for pkcheck... /usr/bin/pkcheck
checking whether pkcheck supports uid value... yes
./configure: 63906: test: xno: unexpected operator
checking for dtrace... no
* configure.ac: Use '=' not '==' in test.
Signed-off-by: Eric Blake <eblake@redhat.com>
I noticed this while working on qemuDomainGetBlockInfo. Assigning
a bool value to an int variable compiles fine, but raises red flags
on the maintenance front as it becomes too easy to assign -1 or 2
or any other non-bool value to the same variable.
* cfg.mk (sc_prohibit_int_assign_bool): New rule.
* src/conf/snapshot_conf.c (virDomainSnapshotRedefinePrep): Fix
offenders.
* src/qemu/qemu_driver.c (qemuDomainGetBlockInfo)
(qemuDomainSnapshotCreateXML): Likewise.
* src/test/test_driver.c (testDomainSnapshotAlignDisks):
Likewise.
* src/util/vircgroup.c (virCgroupSupportsCpuBW): Likewise.
* src/util/virpci.c (virPCIDeviceBindToStub): Likewise.
* src/util/virutil.c (virIsCapableVport): Likewise.
* tools/virsh-domain-monitor.c (cmdDomMemStat): Likewise.
* tools/virsh-domain.c (cmdBlockResize, cmdScreenshot)
(cmdInjectNMI, cmdSendKey, cmdSendProcessSignal)
(cmdDetachInterface): Likewise.
Signed-off-by: Eric Blake <eblake@redhat.com>
Use of an 'int' to represent a 'bool' value is confusing. Just
because dbus made the mistake of cementing their 4-byte wire
format of dbus_bool_t into their API doesn't mean we have to
repeat the mistake. With a little bit of finesse, we can
guarantee that we provide a large-enough value to the DBus
code, while still copying only the relevant one-byte bool
to the client code, and isolate the rest of our code base from
the DBus stupidity.
* src/util/virdbus.c (GET_NEXT_VAL): Add parameter.
(virDBusMessageIterDecode): Adjust all clients.
* src/util/virpolkit.c (virPolkitCheckAuth): Use nicer type.
* tests/virdbustest.c (testMessageSimple, testMessageStruct):
Test new behavior.
Signed-off-by: Eric Blake <eblake@redhat.com>
Commit 28f8dfd (v1.0.0) introduced a security hole: in at least
the qemu implementation of virDomainGetXMLDesc, the use of the
flag VIR_DOMAIN_XML_MIGRATABLE (which is usable from a read-only
connection) triggers the implicit use of VIR_DOMAIN_XML_SECURE
prior to calling qemuDomainFormatXML. However, the use of
VIR_DOMAIN_XML_SECURE is supposed to be restricted to read-write
clients only. This patch treats the migratable flag as requiring
the same permissions, rather than analyzing what might break if
migratable xml no longer includes secret information.
Fortunately, the information leak is low-risk: all that is gated
by the VIR_DOMAIN_XML_SECURE flag is the VNC connection password;
but VNC passwords are already weak (FIPS forbids their use, and
on a non-FIPS machine, anyone stupid enough to trust a max-8-byte
password sent in plaintext over the network deserves what they
get). SPICE offers better security than VNC, and all other
secrets are properly protected by use of virSecret associations
rather than direct output in domain XML.
* src/remote/remote_protocol.x (REMOTE_PROC_DOMAIN_GET_XML_DESC):
Tighten rules on use of migratable flag.
* src/libvirt-domain.c (virDomainGetXMLDesc): Likewise.
Signed-off-by: Eric Blake <eblake@redhat.com>
Commit 6c9a8a4 (Oct 2014) exposed a long-standing issue on 32-bit
machines: code related to virDomainSetMemoryParameters has always
been documented as using a 64-bit limit, but it was implemented by
calling virDomainParseMemory which enforced an 'unsigned long'
limit. Since VIR_DOMAIN_MEMORY_PARAM_UNLIMITED capped to a
long is -1, but virDomainParseScaledValue no longer accepts
negative values, an attempt to use 2^53-1 as a hard memory limit
started failing the testsuite. However, the problem with capping
things artificially low has existed for much longer - ever since
commits 4888f0fb and 2e22f23 (Mar 2012) switched internal tracking
from 'unsigned long' to 'unsigned long long' (prior to that time,
the cap was a side-effect of the choice of types). We _have_ to
cap the balloon memory values, (no thanks to baked in 'unsigned long'
of API such as virDomainSetMaxMemory or virDomainGetInfo with no
counterpart API that guarantees 64-bit access to those numbers)
but memory parameters have never needed the artificial limit.
At any rate, the solution is to make the parser function gain a
parameter, and only do the reduced 32-bit cap for the values that
are constrained due to API.
* src/conf/domain_conf.h (_virDomainMemtune): Add comments.
* src/conf/domain_conf.c (virDomainParseMemory): Add parameter.
(virDomainDefParseXML): Adjust callers.
Signed-off-by: Eric Blake <eblake@redhat.com>
https://bugzilla.redhat.com/show_bug.cgi?id=1140981 reports that
the qemu-kvm shipped as part of RHEL 7.0 intentionally[1] cripples
block jobs by removing the 'block-stream' QMP command, while still
leaving 'block-job-cancel' as an unusable no-op. Meanwhile, we
already had existing code that checked whether block jobs were
completely missing (such as qemu 0.15), old style (cancel is
synchronous, and all commands spelled with '_'), or new style
(cancel is asynchronous, and all commands spelled with '-'), and
used that three-way probe to give decent error messages. At the
time that code was added, all existing qemu versions fell in one
of three buckets, and the code was using the presence of
'block-job-cancel' as the witness of which of the three buckets.
But now that RHEL qemu has shipped with intentionally crippled
'block-stream', we have a fourth bucket, which results in ugly
error messages when trying 'virsh blockpull':
error: Requested operation is not valid: Command 'block-stream' is not found
In reality, the fourth bucket should be treated the same as the
first bucket (no block job support); we can do that by realizing
that no existing build of qemu has working block-stream while
lacking block-job-cancel, so it is easiest to change our witness
to the command that starts a job rather than ends one. We still
act correctly regarding command spelling and whether cancel is
asynchronous. And on crippled RHEL builds, we now get the desired:
error: unsupported configuration: block jobs not supported with this qemu binary
[1] The intentional cripple is limited to qemu-kvm of RHEL; when using
qemu-kvm-rhev of RHEV, block job functionality is supported. Don't ask
me to explain the "why" behind it all - I'm just dealing with fallout
from someone else's decision.
* src/qemu/qemu_capabilities.h (QEMU_CAPS_BLOCKJOB_SYNC): Tweak comment.
* src/qemu/qemu_capabilities.c (virQEMUCapsCommands): Look for stream
rather than cancel when determining the flavor of block jobs supported.
Signed-off-by: Eric Blake <eblake@redhat.com>
Now that all offenders have been cleaned, turn on a syntax-check
rule to prevent future offenders.
* cfg.mk (sc_prohibit_static_zero_init): New rule.
* src/qemu/qemu_driver.c (qemuDomainBlockJobImpl): Avoid false
positive.
Signed-off-by: Eric Blake <eblake@redhat.com>
C guarantees that static variables are zero-initialized. Some older
compilers (and also gcc -fno-zero-initialized-in-bss) create larger
binaries if you explicitly zero-initialize a static variable.
* tests/eventtest.c: Fix initialization.
* tests/testutils.c: Likewise.
* tests/virhostdevtest.c: Likewise.
* tests/virportallocatortest.c: Likewise.
* tests/virscsitest.c: Likewise.
Signed-off-by: Eric Blake <eblake@redhat.com>
C guarantees that static variables are zero-initialized. Some older
compilers (and also gcc -fno-zero-initialized-in-bss) create larger
binaries if you explicitly zero-initialize a static variable.
* tools/virsh-console.c (got_signal): Drop unused variable.
* tools/virsh-domain.c: Fix initialization.
* tools/virsh.c: Likewise.
* tools/virt-host-validate-common.c (virHostMsgWantEscape):
Likewise.
Signed-off-by: Eric Blake <eblake@redhat.com>
We weren't ever using the value for anything other than being non-zero.
* src/util/viraudit.h (virAuditLog): Change signature.
* src/util/viraudit.c (virAuditLog): Update user.
* daemon/libvirtd.c (main): Likewise.
Signed-off-by: Eric Blake <eblake@redhat.com>
https://bugzilla.redhat.com/show_bug.cgi?id=956506 documents that
given a domain where an internal snapshot parent has an external
snapshot child, we lacked a safety check when trying to use the
--children-only option to snapshot-delete:
$ virsh start dom
$ virsh snapshot-create-as dom internal
$ virsh snapshot-create-as dom external --disk-only
$ virsh snapshot-delete dom external
error: Failed to delete snapshot external
error: unsupported configuration: deletion of 1 external disk snapshots not supported yet
$ virsh snapshot-delete dom internal --children
error: Failed to delete snapshot internal
error: unsupported configuration: deletion of 1 external disk snapshots not supported yet
$ virsh snapshot-delete dom internal --children-only
Domain snapshot internal children deleted
While I'd still like to see patches that actually do proper external
snapshot deletion, we should at least fix the inconsistency in the
meantime. With this patch:
$ virsh snapshot-delete dom internal --children-only
error: Failed to delete snapshot internal
error: unsupported configuration: deletion of 1 external disk snapshots not supported yet
* src/qemu/qemu_driver.c (qemuDomainSnapshotDelete): Fix condition.
Signed-off-by: Eric Blake <eblake@redhat.com>
The function hypervEnumAndPull consumes query on success, but leaked
it on failure. Rather than having to change all callers (many of
them indirect callers through the generated
hypervGetMsvmComputerSystemList), it was easier to just guarantee
that the buffer is cleaned on return from the function.
* src/hyperv/hyperv_wmi.c (hypervEnumAndPull): Don't leak query on
failure.
Signed-off-by: Eric Blake <eblake@redhat.com>
Aeons ago (commit 34dcbbb4, v0.8.2), we added a new libvirt event
(VIR_DOMAIN_EVENT_ID_IO_ERROR_REASON) in order to tell the user WHY
the guest halted. This is because at least VDSM wants to react
differently to ENOSPC events (resize the lvm partition to be larger,
and resume the guest as if nothing had happened) from all other events
(I/O is hosed, throw up our hands and flag things as broken). At the
time this was done, downstream RHEL qemu added a vendor extension
'__com.redhat_reason', which would be exactly one of these strings:
"enospc", "eperm", "eio", and "eother". In our stupidity, we exposed
those exact strings to clients, rather than an enum, and we also
return "" if we did not have access to a reason (which was the case
for upstream qemu).
Fast forward to now: upstream qemu commit c7c2ff0c (will be qemu 2.2)
FINALLY adds a 'nospace' boolean, after discussion with multiple
projects determined that VDSM really doesn't care about distinction
between any other error types. So this patch converts 'nospace' into
the string "enospc" for compatibility with RHEL clients that were
already used to the downstream extension, while leaving the reason
blank for all other cases (no change from the status quo).
See also https://bugzilla.redhat.com/show_bug.cgi?id=1119784
* src/qemu/qemu_monitor_json.c (qewmuMonitorJSONHandleIOError):
Parse reason field from modern qemu.
* include/libvirt/libvirt.h.in
(virConnectDomainEventIOErrorReasonCallback): Document it.
Signed-off-by: Eric Blake <eblake@redhat.com>
A cygwin build of 1.2.9 fails with:
util/virprocess.c:87:27: fatal error: sys/syscall.h: No such file or directory
# include <sys/syscall.h>
But in reality, the ONLY user of setns() is lxc, which is Linux-only.
It's easiest to just limit the setns workarounds to Linux.
* src/util/virprocess.c (setns): Limit definition to Linux.
Signed-off-by: Eric Blake <eblake@redhat.com>
Eric Blake