Makefiles are another easy file to enforce line limits.
Mostly straightforward; interesting tricks worth noting:
src/Makefile.am: $(confdir) was already defined, use it in more places
tests/Makefile.am: path_add and VG required some interesting compression
* cfg.mk (sc_prohibit_long_lines): Add another test.
* Makefile.am: Fix offenders.
* daemon/Makefile.am: Likewise.
* docs/Makefile.am: Likewise.
* python/Makefile.am: Likewise.
* src/Makefile.am: Likewise.
* tests/Makefile.am: Likewise.
Signed-off-by: Eric Blake <eblake@redhat.com>
Long lines are harder to read and harder to diff; in fact, if lines get
too long (> 1000 bytes), it starts causing issues where git send-email
refuses to send patches for the file. I've cleaned up the tests
directory in the past (see commits bd6c46f, 3b750d1), but new long
lines have been introduced in the meantime.
Why 90 instead of 80? Because there were too many tests on the fringe
edge, and I didn't want to edit that many files.
Add a syntax check to prevent future long lines.
* cfg.mk (sc_prohibit_long_lines): New rule.
* tests/qemuxml2argvdata/qemuxml2argv-*.args: Split lines of any
file with content longer than 90 columns.
* tests/storagevolxml2argvdata/*.argv: Likewise.
Signed-off-by: Eric Blake <eblake@redhat.com>
Add two syntax-check rules
- sc_prohibit_int_ijk - block use of 'int' as a data type
for any variables named 'i', 'j', 'k'
- sc_prohibit_int_iijjkk - block use of 'ii', 'jj', 'kk'
for any variable names
Actually, I'm turning this function into a macro as filename,
function name and line number needs to be passed. The new
function virAsprintfInternal is introduced with the extended set
of arguments.
Based on a report by Chandrashekar Shastri, at
https://bugzilla.redhat.com/show_bug.cgi?id=979360
On systems where git cannot access the outside world, a developer
can instead arrange to get a copy of gnulib at the right commit
via side channels (such as NFS share drives), set GNULIB_SRCDIR,
then use ./autogen.sh --no-git. In this setup, we will now
avoid direct use of git. Of course, this means no automatic
gnulib updates when libvirt.git updates its submodule, but it
is expected that any developer in such a situation is already
prepared to deal with the fallout.
* .gnulib: Update to latest, for bootstrap.
* bootstrap: Synchronize from gnulib.
* autogen.sh (no_git): Avoid git when requested.
* cfg.mk (_update_required): Skip automatic rerun of bootstrap if
we can't use git.
* docs/compiling.html.in: Document this setup.
* docs/hacking.html.in: Mention this.
* HACKING: Regenerate.
Signed-off-by: Eric Blake <eblake@redhat.com>
Osier Yang pointed out that I introduced a syntax error in my
syntax check (I really shouldn't make last-minute changes without
testing them....).
/bin/sh: -c: line 2: syntax error near unexpected token `;'
/bin/sh: -c: line 2: ` { echo 'maint.mk: incorrect whitespace, see HACKING for rules' 2>&; \'
make: *** [bracket-spacing-check] Error 1
* cfg.mk (bracket-spacing-check): Fix copy-and-paste error.
Signed-off-by: Eric Blake <eblake@redhat.com>
To ensure we don't regress and cause the need for further
cleanups, add a 'make syntax-check' rule that ensures new
files have proper copyright contents.
* cfg.mk (sc_copyright_address): Rename...
(sc_copyright_usage): ...and enhance.
Signed-off-by: Eric Blake <eblake@redhat.com>
https://www.gnu.org/licenses/gpl-howto.html states:
You should also include a copy of the license itself somewhere in the
distribution of your program. All programs, whether they are released
under the GPL or LGPL, should include the text version of the GPL. In
GNU programs the license is usually in a file called COPYING.
If you are releasing your program under the LGPL, you should also
include the text version of the LGPL, usually in a file called
COPYING.LESSER. Please note that, since the LGPL is a set of
additional permissions on top of the GPL, it's important to include
both licenses so users have all the materials they need to understand
their rights.
* configure.ac (COPYING): No more games with non-git file.
* COPYING: New file, copied from gnulib.
* COPYING.LIB: Rename...
* COPYING.LESSER: ...to this.
* .gitignore: Track licenses in git.
* cfg.mk (exclude_file_name_regexp--sc_copyright_address): Tweak
rule.
* libvirt.spec.in (daemon, client, python): Reflect rename.
Signed-off-by: Eric Blake <eblake@redhat.com>
Use of the select() system call is inherantly dangerous since
applications will hit a buffer overrun if any FD number exceeds
the size of the select set size (typically 1024). Replace the
two uses of select() with poll() and use cfg.mk to ban any
future use of select().
NB: This changes the phyp driver so that it uses an infinite
timeout, instead of busy-waiting for 1ms at a time.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
These all existed before virfile.c was created, and for some reason
weren't moved.
This is mostly straightfoward, although the syntax rule prohibiting
write() had to be changed to have an exception for virfile.c instead
of virutil.c.
This movement pointed out that there is a function called
virBuildPath(), and another almost identical function called
virFileBuildPath(). They really should be a single function, which
I'll take care of as soon as I figure out what the arglist should look
like.
The source code base needs to be adapted as well. Some files
include virutil.h just for the string related functions (here,
the include is substituted to match the new file), some include
virutil.h without any need (here, the include is removed), and
some require both.
POSIX says that both basename() and dirname() may return static
storage (aka they need not be thread-safe); and that they may but
not must modify their input argument. Furthermore, <libgen.h>
is not available on all platforms. For these reasons, you should
never use these functions in a multi-threaded library.
Gnulib instead recommends a way to avoid the portability nightmare:
gnulib's "dirname.h" provides useful thread-safe counterparts. The
obvious dir_name() and base_name() are GPL (because they malloc(),
but call exit() on failure) so we can't use them; but the LGPL
variants mdir_name() (malloc's or returns NULL) and last_component
(always points into the incoming string without modifying it,
differing from basename semantics only on corner cases like the
empty string that we shouldn't be hitting in the first place) are
already in use in libvirt. This finishes the swap over to the safe
functions.
* cfg.mk (sc_prohibit_libgen): New rule.
* src/util/vircgroup.c: Fix offenders.
* src/parallels/parallels_storage.c (parallelsPoolAddByDomain):
Likewise.
* src/parallels/parallels_network.c (parallelsGetBridgedNetInfo):
Likewise.
* src/node_device/node_device_udev.c (udevProcessSCSIHost)
(udevProcessSCSIDevice): Likewise.
* src/storage/storage_backend_disk.c
(virStorageBackendDiskDeleteVol): Likewise.
* src/util/virpci.c (virPCIGetDeviceAddressFromSysfsLink):
Likewise.
* src/util/virstoragefile.h (_virStorageFileMetadata): Avoid false
positive.
Signed-off-by: Eric Blake <eblake@redhat.com>
With this patch, include public headers in "" form is only allowed
for "internal.h". And only the external tools (examples|tools|python
|include/libvirt) can include the public headers in <> form.
Directories python/tools/examples should include them in <> form,
though this patch allows "" form in these directories by excluding
them, a later patch will do the cleanup.
Some aspects of the cgroups setup / detection code are quite subtle
and easy to break. It would greatly benefit from unit testing, but
this is difficult because the test suite won't have privileges to
play around with cgroups. The solution is to use monkey patching
via LD_PRELOAD to override the fopen, open, mkdir, access functions
to redirect access of cgroups files to some magic stubs in the
test suite.
Using this we provide custom content for the /proc/cgroup and
/proc/self/mounts files which report a fixed cgroup setup. We
then override open/mkdir/access so that access to the cgroups
filesystem gets redirected into files in a temporary directory
tree in the test suite build dir.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
We've already scrubbed for comparisons of 'uid_t == -1' (which fail
on platforms where uid_t is a u16), but another one snuck in.
* src/util/virutil.c (virSetUIDGIDWithCaps): Correct uid comparison.
* cfg.mk (sc_prohibit_risky_id_promotion): New rule.
testutils.c likes to print summaries after a test completes,
including if it failed. But if the test outright exit()s,
this summary is skipped. Enforce that we return instead of exit.
* cfg.mk (sc_prohibit_exit_in_tests): New syntax check.
* tests/commandhelper.c (main): Fix offenders.
* tests/qemumonitorjsontest.c (mymain): Likewise.
* tests/seclabeltest.c (main): Likewise.
* tests/securityselinuxlabeltest.c (mymain): Likewise.
* tests/securityselinuxtest.c (mymain): Likewise.
* tests/testutils.h (VIRT_TEST_MAIN_PRELOAD): Likewise.
* tests/testutils.c (virtTestMain): Likewise.
(virtTestCaptureProgramOutput): Use symbolic name.
hacking: Add some text around the running of Valgrind along with example
output for "real" vs. "false positives".
cfg.mk: Add hacking.in.html to sc_prohibit_raw_allocation
Adjust the macros to free memory allocated during various calls to
perform the check if parameter is NULL prior to really freeing and to
set the pointer to NULL after done freeing.
Nested conditionals are hard to read if they are not indented.
We can't add arbitrary whitespace to everything in spec files,
but we CAN add spaces before %if and %define. Use this trick,
plus a fancy sed script that rewrites a spec file into a C
file, so we can use cppi to keep our spec file nice.
For reference, the sed script converts code like:
|# RHEL-5 builds are client-only for s390, ppc
|%if 0%{?rhel} == 5
| %ifnarch %{ix86} x86_64 ia64
| %define client_only 1
| %endif
|%endif
into the following for cppi:
|// # RHEL-5 builds are client-only for s390, ppc
|#if a // 0%{?rhel} == 5
|# if a // %{ix86} x86_64 ia64
|# define client_only 1
|# endif
|#endif
and errors from 'make syntax-check' look like:
spec_indentation
cppi: mingw-libvirt.spec.in: line 130: not properly indented
maint.mk: incorrect preprocessor indentation
* libvirt.spec.in: Add some indentation to make it easier to follow
various conditionals.
* mingw-libvirt-spec.in: Likewise.
* cfg.mk (sc_spec_indentation): New syntax check to enforce it.
This patch introduces support for LXC specific public APIs. In
common with what was done for QEMU, this creates a libvirt_lxc.so
library and libvirt/libvirt-lxc.h header file.
The actual APIs are
int virDomainLxcOpenNamespace(virDomainPtr domain,
int **fdlist,
unsigned int flags);
int virDomainLxcEnterNamespace(virDomainPtr domain,
unsigned int nfdlist,
int *fdlist,
unsigned int *noldfdlist,
int **oldfdlist,
unsigned int flags);
which provide a way to use the setns() system call to move the
calling process into the container's namespace. It is not
practical to write in a generically applicable manner. The
nearest that we could get to such an API would be an API which
allows to pass a command + argv to be executed inside a
container. Even if we had such a generic API, this LXC specific
API is still useful, because it allows the caller to maintain
the current process context, in particular any I/O streams they
have open.
NB the virDomainLxcEnterNamespace() API is special in that it
runs client side, so does not involve the internal driver API.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
There are many aspects of the guest XML which result in the
SELinux driver applying file labelling. With the increasing
configuration options it is desirable to test this behaviour.
It is not possible to assume that the test suite has the
ability to set SELinux labels. Most filesystems though will
support extended attributes. Thus for the purpose of testing,
it is possible to extend the existing LD_PRELOAD hack to
override setfilecon() and getfilecon() to simply use the
'user.libvirt.selinux' attribute for the sake of testing.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Most checks for libraries take the same format
* --with-libFOO=yes|no|check|/some/path argument
* check for a function NNN in libFOO.so
* check for a header file DDD/HHH.h
* Define a WITH_FOO config.h symbol
* Define a WITH_FOO make conditional
* Substitute FOO_CFLAGS and FOO_LIBS make variables
* Print CFLAGS & LIBS summary at the end
Doing all this correctly is rather difficult, typically
done by copy+paste of a previous usage. Further small
improvements people make are not applied to all previous
usages.
Improve this by creating some helper macros to apply
good practice. First, to perform the actual checks:
LIBVIRT_CHECK_LIB([SELINUX], [selinux],
[getfilecon], [selinux/selinux.h])
This checks for 'getfilecon' in -lselinux, and the
existence of 'selinux/selinux.h' header file. If successful
it sets SELINUX_CFLAGS and SELINUX_LIBS. The WITH_SELINUX
config.h macro and WITH_SELINUX make conditional are also
defined.
In some cases we need to check two variants of the same
library
LIBVIRT_CHECK_LIB_ALT([SASL], [sasl2],
[sasl_client_init], [sasl/sasl.h],
[SASL1], [sasl],
[sasl_client_init], [sasl/sasl.h])
This checks for sasl_client_init in libsasl2, and if that
is not found, checks sasl_client_init in libsasl. If the
first check succeeds WITH_SASL is set, while if the second
check succeeds *both* WITH_SASL and WITH_SASL1 are set.
If the library supports pkg-config, then another variant
is available
LIBVIRT_CHECK_PKG([AVAHI], [avahi-client], [0.6.0])
This checks for avahi-client >= 0.6.0 via pkg-config
and sets WITH_AVAHI if found.
Finally to print a summary of CFLAGS & LIBs found (if any):
LIBVIRT_RESULT_LIB([SELINUX])
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Test cases for virSysinfoRead. Initially, there are tests for
x86 (DMI based) and s390 (/proc/... based).
In lack of PPC data, I have stubbed out the test for it, but it
can be added with a minimal effort.
Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com>
The virtlockd daemon will be responsible for managing locks
on virtual machines. Communication will be via the standard
RPC infrastructure. This provides the XDR protocol definition
* src/locking/lock_protocol.x: Wire protocol for virtlockd
* src/Makefile.am: Include lock_protocol.[ch] in virtlockd
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
The virtlockd daemon will maintain locks on behalf of libvirtd.
There are two reasons for it to be separate
- Avoid risk of other libvirtd threads accidentally
releasing fcntl() locks by opening + closing a file
that is locked
- Ensure locks can be preserved across libvirtd restarts.
virtlockd will need to be able to re-exec itself while
maintaining locks. This is simpler to achieve if its
sole job is maintaining locks
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
I noticed when writing the backend functions for virNetworkUpdate that
I was repeating the same sequence of memmove, VIR_REALLOC, nXXX-- (and
messed up the args to memmove at least once), and had seen the same
sequence in a lot of other places, so I decided to write a few
utility functions/macros - see the .h file for full documentation.
The intent is to reduce the number of lines of code, but more
importantly to eliminate the need to check the element size and
element count arithmetic every time we need to do this (I *always*
make at least one mistake.)
VIR_INSERT_ELEMENT: insert one element at an arbitrary index within an
array of objects. The size of each object is determined
automatically by the macro using sizeof(*array). The new element's
contents are copied into the inserted space, then the original copy
of contents are 0'ed out (if everything else was
successful). Compile-time assignment and size compatibility between
the array and the new element is guaranteed (see explanation below
[*])
VIR_INSERT_ELEMENT_COPY: identical to VIR_INSERT_ELEMENT, except that
the original contents of newelem are not cleared to 0 (i.e. a copy
is made).
VIR_APPEND_ELEMENT: This is just a special case of VIR_INSERT_ELEMENT
that "inserts" one past the current last element.
VIR_APPEND_ELEMENT_COPY: identical to VIR_APPEND_ELEMENT, except that
the original contents of newelem are not cleared to 0 (i.e. a copy
is made).
VIR_DELETE_ELEMENT: delete one element at an arbitrary index within an
array of objects. It's assumed that the element being deleted is
already saved elsewhere (or cleared, if that's what is appropriate).
All five of these macros have an _INPLACE variant, which skips the
memory re-allocation of the array, assuming that the caller has
already done it (when inserting) or will do it later (when deleting).
Note that VIR_DELETE_ELEMENT* can return a failure, but only if an
invalid index is given (index + amount to delete is > current array
size), so in most cases you can safely ignore the return (that's why
the helper function virDeleteElementsN isn't declared with
ATTRIBUTE_RETURN_CHECK). A warning is logged if this ever happens,
since it is surely a coding error.
[*] One initial problem with the INSERT and APPEND macros was that,
due to both the array pointer and newelem pointer being cast to void*
when passing to virInsertElementsN(), any chance of type-checking was
lost. If we were going to move in newelem with a memmove anyway, we
would be no worse off for this. However, most current open-coded
insert/append operations use direct struct assignment to move the new
element into place (or just populate the new element directly) - thus
use of the new macros would open a possibility for new usage errors
that didn't exist before (e.g. accidentally sending &newelemptr rather
than newelemptr - I actually did this quite a lot in my test
conversions of existing code).
But thanks to Eric Blake's clever thinking, I was able to modify the
INSERT and APPEND macros so that they *do* check for both assignment
and size compatibility of *ptr (an element in the array) and newelem
(the element being copied into the new position of the array). This is
done via clever use of the C89-guaranteed fact that the sizeof()
operator must have *no* side effects (so an assignment inside sizeof()
is checked for validity, but not actually evaluated), and the fact
that virInsertElementsN has a "# of new elements" argument that we
want to always be 1.
Commit 71d1256 tried to fix a problem where rebasing an old
branch on top of newer libvirt.git resulted in automake failing
because of a missing AUTHORS file. However, while the fix
worked for an incremental 'make', it did not work for someone
that directly reran './autogen.sh'. Reported by Laine Stump.
* autogen.sh (autoreconf): Check for same conditions as cfg.mk.
* cfg.mk (_update_required): Add comments.
Ever since commit 7b21981c started generating AUTHORS, we now have
the situation that if you flip between two branches in the same
git repository that cross that commit boundary, then 'make' will
fail due to automake complaining about AUTHORS not existing. The
simplest solution is to realize that if AUTHORS does not exist,
then we flipped branches so we will need to rerun bootstrap
anyways; and rerunning bootstrap ensures AUTHORS will exist in time.
* cfg.mk (_update_required): Also depend on AUTHORS.
This documents the following whitespace rules
if(foo) // Bad
if (foo) // Good
int foo (int wizz) // Bad
int foo(int wizz) // Good
bar = foo (wizz); // Bad
bar = foo(wizz); // Good
typedef int (*foo) (int wizz); // Bad
typedef int (*foo)(int wizz); // Good
int foo( int wizz ); // Bad
int foo(int wizz); // Good
There is a syntax-check rule extension to validate all these rules.
Checking for 'function (...args...)' is quite difficult since it
needs to ignore valid usage with keywords like 'if (...test...)'
and while/for/switch. It must also ignore source comments and
quoted strings.
It is not possible todo this with a simple regex in the normal
syntax-check style. So a short Perl script is created instead
to analyse the source. In practice this works well enough. The
only thing it can't cope with is multi-line quoted strings of
the form
"start of string\
more lines\
more line\
the end"
but this can and should be written as
"start of string"
"more lines"
"more line"
"the end"
with this simple change, the bracket checking script does not
have any false positives across libvirt source, provided it
is only run against .c files. It is not practical to run it
against .h files, since those use whitespace extensively to
get alignment (though this is somewhat inconsistent and could
arguably be fixed).
The only limitation is that it cannot detect a violation where
the first arg starts with a '*', eg
foo(*wizz);
since this generates too many false positives on function
typedefs which can't be supressed efficiently.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
With our fix of mkostemp (pushed as 2b435c15) we define a macro
to compile with uclibc. However, this definition is conditional
and thus needs to be properly indented. Moreover, with this definition
sc_prohibit_mkstemp syntax-check rule keeps yelling:
src/util/logging.c:63:# define mkostemp(x,y) mkstemp(x)
maint.mk: use mkostemp with O_CLOEXEC instead of mkstemp
Therefore we should ignore this file for this rule.
https://bugzilla.redhat.com/show_bug.cgi?id=871756
Commit cd1e8d1 assumed that systems new enough to have journald
also have mkostemp; but this is not true for uclibc.
For that matter, use of mkstemp[s] is unsafe in a multi-threaded
program. We should prefer mkostemp[s] in the first place.
* bootstrap.conf (gnulib_modules): Add mkostemp, mkostemps; drop
mkstemp and mkstemps.
* cfg.mk (sc_prohibit_mkstemp): New syntax check.
* tools/virsh.c (vshEditWriteToTempFile): Adjust caller.
* src/qemu/qemu_driver.c (qemuDomainScreenshot)
(qemudDomainMemoryPeek): Likewise.
* src/secret/secret_driver.c (replaceFile): Likewise.
* src/vbox/vbox_tmpl.c (vboxDomainScreenshot): Likewise.
This patch resolves: https://bugzilla.redhat.com/show_bug.cgi?id=871201
If libvirt is restarted after updating the dnsmasq or radvd packages,
a subsequent "virsh net-destroy" will fail to kill the dnsmasq/radvd
process.
The problem is that when libvirtd restarts, it re-reads the dnsmasq
and radvd pidfiles, then does a sanity check on each pid it finds,
including checking that the symbolic link in /proc/$pid/exe actually
points to the same file as the path used by libvirt to execute the
binary in the first place. If this fails, libvirt assumes that the
process is no longer alive.
But if the original binary has been replaced, the link in /proc is set
to "$binarypath (deleted)" (it literally has the string " (deleted)"
appended to the link text stored in the filesystem), so even if a new
binary exists in the same location, attempts to resolve the link will
fail.
In the end, not only is the old dnsmasq/radvd not terminated when the
network is stopped, but a new dnsmasq can't be started when the
network is later restarted (because the original process is still
listening on the ports that the new process wants).
The solution is, when the initial "use stat to check for identical
inodes" check for identity between /proc/$pid/exe and $binpath fails,
to check /proc/$pid/exe for a link ending with " (deleted)" and if so,
truncate that part of the link and compare what's left with the
original binarypath.
A twist to this problem is that on systems with "merged" /sbin and
/usr/sbin (i.e. /sbin is really just a symlink to /usr/sbin; Fedora
17+ is an example of this), libvirt may have started the process using
one path, but /proc/$pid/exe lists a different path (indeed, on F17
this is the case - libvirtd uses /sbin/dnsmasq, but /proc/$pid/exe
shows "/usr/sbin/dnsmasq"). The further bit of code to resolve this is
to call virFileResolveAllLinks() on both the original binarypath and
on the truncated link we read from /proc/$pid/exe, and compare the
results.
The resulting code still succeeds in all the same cases it did before,
but also succeeds if the binary was deleted or replaced after it was
started.
