So far clients were closed when disposing the daemon, after the state
driver cleanup. This was leading to libvirtd crashing at shutdown due
to missing driver.

Moving the client close in virNetServerClose() fixes the problem.
Reviewed-by: NErik Skultety <eskultet@redhat.com>

Prior to this change, we relied solely on the inherited readonly
attribute of a service's socket. This only worked for our UNIX sockets
(and only to some degree), but doesn't work for TCP sockets which are RW
by default, but such connections support RO as well. This patch forces
an update on the client object once we have established a connection to
reflect the nature of the connection itself rather than relying on the
underlying socket's attributes.
Clients connected to the admin server have always been connected as RW
only.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1524399Signed-off-by: NErik Skultety <eskultet@redhat.com>

After the virNetDaemonAddServerPostExec call in virtlogd we should have
netserver refcount set to 2. One goes to netdaemon servers hashtable
and one goes to virt{logd,lock} own reference to netserver. Let's add
the missing increment in virNetDaemonAddServerPostExec itself while
holding the daemon lock.

Since lockd defers management of the @srv object by the presence
in the hash table, virLockDaemonNewPostExecRestart must Unref the
alloc'd Ref on the @srv object done as part of virNetDaemonAddServerPostExec
and virNetServerNewPostExecRestart processing. The virNetDaemonGetServer
in lock_daemon main will also take a reference which is Unref'd during
main cleanup.

Right-aligning backslashes when defining macros or using complex
commands in Makefiles looks cute, but as soon as any changes is
required to the code you end up with either distractingly broken
alignment or unnecessarily big diffs where most of the changes
are just pushing all backslashes a few characters to one side.

Generated using

  $ git grep -El '[[:blank:]][[:blank:]]\\$' | \
    grep -E '*\.([chx]|am|mk)$$' | \
    while read f; do \
      sed -Ei 's/[[:blank:]]*[[:blank:]]\\$/ \\/g' "$f"; \
    done
Signed-off-by: NAndrea Bolognani <abologna@redhat.com>

The packet with passed FD has the following format:

    --------------------------
    | len | header | payload |
    --------------------------

where "payload" has an additional count of FDs before the actual data:

    ------------------
    | nfds | payload |
    ------------------

When the packet is received we parse the "header", which as a side
effect updates msg->bufferOffset to point to the beginning of "payload".
If the message call contains FDs, we need to also parse the count of
FDs, which also updates the msg->bufferOffset.

The issue here is that when we attempt to read the FDs data from the
socket and we receive EAGAIN we finish the reading and call poll()
to wait for the data the we need.  When the data arrives we already have
the packet in our buffer so we read the "header" again but this time
we don't read the count of FDs because we already have it stored.

That means that the msg->bufferOffset is not updated to point to the
actual beginning of the payload data, but it points to the count of
FDs.  After all FDs are processed we dispatch the message to process
it and decode the payload.  Since the msg->bufferOffset points to wrong
data, we decode the wrong payload and the API call fails with
error messages:

    Domain not found: no domain with matching uuid '67656e65-7269-6300-0c87-5003ca6941f2' ()

Broken by commit 133c511b which fixed a FD and memory leak.
Signed-off-by: NPavel Hrdina <phrdina@redhat.com>

Seeing a log message saying 'flags=93' is ambiguous & confusing unless
you happen to know that libvirt always prints flags as hex.  Change our
debug messages so that they always add a '0x' prefix when printing flags,
and '0' prefix when printing mode. A few other misc places gain a '0x'
prefix in error messages too.
Signed-off-by: NDaniel P. Berrange <berrange@redhat.com>

This is particularly useful on operating systems that don't ship
Perl as part of the base system (eg. FreeBSD) while still working
just as well as it did before on Linux.

In one case (src/rpc/genprotocol.pl) the interpreter path was
missing altogether.
Signed-off-by: NAndrea Bolognani <abologna@redhat.com>

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

In ace45e67 I tried to fix a problem that we get the reply to
a D-Bus call while we were sleeping. In that case the callback
was never set. So I changed the code that the callback is called
directly in this case. However, I hadn't realized that since the
callback is called out of order it locks the virNetDaemon.
Exactly the very same virNetDaemon object that we are dealing
with right now and that we have locked already (in
virNetDaemonAddShutdownInhibition())
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>

Inspired by the recent GIT / Mercurial security flaws
(http://blog.recurity-labs.com/2017-08-10/scm-vulns),
consider someone/something manages to feed libvirt a bogus
URI such as:

  virsh -c qemu+ssh://-oProxyCommand=gnome-calculator/system

In this case, the hosname "-oProxyCommand=gnome-calculator"
will get interpreted as an argument to ssh, not a hostname.
Fortunately, due to the set of args we have following the
hostname, SSH will then interpret our bit of shell script
that runs 'nc' on the remote host as a cipher name, which is
clearly invalid. This makes ssh exit during argv parsing and
so it never tries to run gnome-calculator.

We are lucky this time, but lets be more paranoid, by using
'--' to explicitly tell SSH when it has finished seeing
command line options. This forces it to interpret
"-oProxyCommand=gnome-calculator" as a hostname, and thus
see a fail from hostname lookup.
Signed-off-by: NDaniel P. Berrange <berrange@redhat.com>

So there are couple of issues here. Firstly, we never unref the
@pendingReply and thus it leaks.

==13279== 144 (72 direct, 72 indirect) bytes in 1 blocks are definitely lost in loss record 1,095 of 1,259
==13279==    at 0x4C2E080: calloc (vg_replace_malloc.c:711)
==13279==    by 0x781FA97: _dbus_pending_call_new_unlocked (in /usr/lib64/libdbus-1.so.3.14.11)
==13279==    by 0x7812A4C: dbus_connection_send_with_reply (in /usr/lib64/libdbus-1.so.3.14.11)
==13279==    by 0x56BEDF3: virNetDaemonCallInhibit (virnetdaemon.c:514)
==13279==    by 0x56BEF18: virNetDaemonAddShutdownInhibition (virnetdaemon.c:536)
==13279==    by 0x12473B: daemonInhibitCallback (libvirtd.c:742)
==13279==    by 0x1249BD: daemonRunStateInit (libvirtd.c:823)
==13279==    by 0x554FBCF: virThreadHelper (virthread.c:206)
==13279==    by 0x8F913D3: start_thread (in /lib64/libpthread-2.23.so)
==13279==    by 0x928DE3C: clone (in /lib64/libc-2.23.so)

Secondly, while we send the message, we are suspended ('cos we're
talking to a UNIX socket).  However, until we are resumed back
again the reply might have came therefore subsequent
dbus_pending_call_set_notify() has no effect and in fact the
virNetDaemonGotInhibitReply() callback is never called. Thirdly,
the dbus_connection_send_with_reply() has really stupid policy
for return values. To cite the man page:

  Returns
      FALSE if no memory, TRUE otherwise.

Yes, that's right. If anything goes wrong and it's not case of
OOM then TRUE is returned, i.e. you're trying to pass FDs and
it's not supported, or you're not connected, or anything else.
Therefore, checking for return value of
dbus_connection_send_with_reply() is not enoguh. We also have to
check if @pendingReply is not NULL before proceeding any further.
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>

This reverts commit e4b980c8.

When a binary links against a .a archive (as opposed to a shared library),
any symbols which are marked as 'weak' get silently dropped. As a result
when the binary later runs, those 'weak' functions have an address of
0x0 and thus crash when run.

This happened with virtlogd and virtlockd because they don't link to
libvirt.so, but instead just libvirt_util.a and libvirt_rpc.a. The
virRandomBits symbols was weak and so left out of the virtlogd &
virtlockd binaries, despite being required by virHashTable functions.

Various other binaries like libvirt_lxc, libvirt_iohelper, etc also
link directly to .a files instead of libvirt.so, so are potentially
at risk of dropping symbols leading to a later runtime crash.

This is normal linker behaviour because a weak symbol is not treated
as undefined, so nothing forces it to be pulled in from the .a You
have to force the linker to pull in weak symbols using -u$SYMNAME
which is not a practical approach.

This risk is silent bad linkage that affects runtime behaviour is
not acceptable for a fix that was merely trying to fix the test
suite. So stop using __weak__ again.
Signed-off-by: NDaniel P. Berrange <berrange@redhat.com>

If we exceed a fixed limit in RPC code we get a horrible message
like this, if the parameter type is a 'string', because we forgot
to initialize the error message type field:

  $ virsh snapshot-list ostack1
  error: too many remote undefineds: 1329 > 1024

It would also be useful to know which RPC call and field was
exceeded. So this patch makes us report:

  $ virsh snapshot-list ostack1
  error: too many remote undefineds: 1329 > 1024,
  in parameter 'names' for 'virDomainSnapshotListNames'
Signed-off-by: NDaniel P. Berrange <berrange@redhat.com>

Currently all mockable functions are annotated with the 'noinline'
attribute. This is insufficient to guarantee that a function can
be reliably mocked with an LD_PRELOAD. The C language spec allows
the compiler to assume there is only a single implementation of
each function. It can thus do things like propagating constant
return values into the caller at compile time, or creating
multiple specialized copies of the function body each optimized
for a different caller. To prevent these optimizations we must
also set the 'noclone' and 'weak' attributes.

This fixes the test suite when libvirt.so is built with CLang
with optimization enabled.
Signed-off-by: NDaniel P. Berrange <berrange@redhat.com>

The log category for virnetdaemon.c was mistakenly set
to rpc.netserver. Some useful info about the inhibitor
file descriptor was also never logged.
Signed-off-by: NDaniel P. Berrange <berrange@redhat.com>

The DBus conditional was renamed way back:

  commit da77f04e
  Author: Daniel P. Berrange <berrange@redhat.com>
  Date:   Thu Sep 20 15:05:39 2012 +0100

    Convert HAVE_DBUS to WITH_DBUS

but the shutdown inhibit code was not updated. Thus libvirt
was never inhibiting shutdown by a logged in user when VMs
are running.
Reviewed-by: NPavel Hrdina <phrdina@redhat.com>
Signed-off-by: NDaniel P. Berrange <berrange@redhat.com>

Use ATTRIBUTE_FALLTHROUGH, introduced by commit
5d84f596, instead of comments to
indicate that the fall through is an intentional behavior.
Signed-off-by: NMarc Hartmayer <mhartmay@linux.vnet.ibm.com>
Reviewed-by: NBoris Fiuczynski <fiuczy@linux.vnet.ibm.com>
Reviewed-by: NBjoern Walk <bwalk@linux.vnet.ibm.com>

Signed-off-by: NMarc Hartmayer <mhartmay@linux.vnet.ibm.com>
Reviewed-by: NBoris Fiuczynski <fiuczy@linux.vnet.ibm.com>
Reviewed-by: NBjoern Walk <bwalk@linux.vnet.ibm.com>

While reworking client side of streams, I had to postpone payload
decoding so that stream holes and stream data can be
distinguished in virNetClientStreamRecvPacket. That's merely what
18944b7a does. However, I accidentally removed one important
bit: when server sends us an empty STREAM packet (with no
payload) - meaning end of stream - st->incomingEOF flag needs to
be set. It used to be before I touched the code. After I removed
it, virNetClientStreamRecvPacket will try to fetch more data from
the stream, but it will never come.
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>
Reviewed-by: NJim Fehlig <jfehlig@suse.com>

When increasing the buffer size up to VIR_NET_MESSAGE_MAX, we
currently quadruple it each time.  This unfortunately means that we
cannot allow certain buffer sizes -- for example the current
VIR_NET_MESSAGE_MAX == 33554432 can never be "hit" since ‘newlen’
jumps from 16MB to 64MB.

Instead of quadrupling, double it each time.

Thanks: Daniel Berrange.
Signed-off-by: NRichard W.M. Jones <rjones@redhat.com>

While most of the APIs are okay with 16M messages, the bulk stats API
can run into the limit in big configurations. Before we devise a new
plan for this, bump this limit slightly to accomodate some more configs.

Currently, we don't assign any meaning to that. Our current view
on virStream is that it's merely a pipe. And pipes don't support
seeking.
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>

Now, not all APIs are going to support sparse streams. To some it
makes no sense at all, e.g. virDomainOpenConsole() or
virDomainOpenChannel(). To others, we will need a special flag to
indicate that client wants to enable sparse streams. Instead of
having to write RPC dispatchers by hand we can just annotate in
our .x files that a certain flag to certain RPC call enables this
feature. For instance:

     /**
      * @generate: both
      * @ReadStream: 1
      * @sparseflag: VIR_SPARSE_STREAM
      * @acl: storage_vol:data_read
      */
     REMOTE_PROC_DOMAIN_SOME_API = XXX,

Therefore, whenever client calls virDomainSomeAPI(..,
VIR_SPARSE_STREAM); daemon will mark that down and send stream
skips when possible.
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>

This is fairly trivial now that we have everything in place.
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>

Whenever server sends a client stream packet (either regular with
actual data or stream skip one) it is queued on @st->rx. So the
list is a mixture of both types of stream packets. So now that we
have all the helpers needed we can wire their processing up. But
since virNetClientStreamRecvPacket doesn't support
VIR_STREAM_RECV_STOP_AT_HOLE flag yet, let's turn all received
skips into zeroes repeating requested times.
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>

This function will fetch previously processed stream holes and
return their sum.
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>

Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>

This is a function that handles an incoming STREAM_HOLE packet.
Even though it is not wired up yet, it will be soon. At the
beginning do couple of checks whether server plays nicely and
sent us a STREAM_HOLE packed only after we've enabled sparse
streams. Then decodes the message payload to see how big the hole
is and stores it in passed @length argument.
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>

While the previous commit implemented a helper for sending a
STREAM_HOLE packet for daemon, this is a client's counterpart.
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>

This is just a helper function that takes in a length value,
encodes it into XDR and sends to client.
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>

This is a special type of stream packet, that is bidirectional
and contains information regarding how many bytes each side will
be skipping in the stream.
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>

This is going to be RPC representation for virStreamSendHole.
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>

Add a new argument to daemonCreateClientStream in order to allow for
future expansion to mark that a specific stream can be used to skip
data, such as the case with sparsely populated files. The new flag will
be the eventual decision point between client/server to decide whether
both ends can support and want to use sparse streams.

A new bool 'allowSkip' is added to both _virNetClientStream and
daemonClientStream in order to perform the tracking.
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>

Add a virStreamPtr pointer to the _virNetClientStream
in order to reverse track the parent stream.
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>

When we get a POLLHUP or VIR_EVENT_HANDLE_HANGUP event for a client, we
still want to read from the socket to process any accumulated data. But
doing so inevitably results in an error and a call to
virNetClientMarkClose before we get to processing the hangup event (and
another call to virNetClientMarkClose). However the close reason passed
to the second virNetClientMarkClose call is ignored because another one
was already set. We need to pass the correct close reason when marking
the socket to be closed for the first time.

https://bugzilla.redhat.com/show_bug.cgi?id=1373859Signed-off-by: NJiri Denemark <jdenemar@redhat.com>

We prefer c89 style of comments.
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>

ka maybe have been freeed in virObjectUnref, application using
virKeepAliveTimer will segfault when unlock ka. We should keep
ka's refs positive before using it.

#0  0x00007fd8f79970e8 in virClassIsDerivedFrom (klass=0xdeadbeef, parent=0x7fd8e8001b80) at util/virobject.c:169
#1  0x00007fd8f799742e in virObjectIsClass (anyobj=anyobj entry=0x7fd8e800b9c0, klass=＜optimized out＞) at util/virobject.c:365
#2  0x00007fd8f79974e4 in virObjectUnlock (anyobj=0x7fd8e800b9c0) at util/virobject.c:338
#3  0x00007fd8f7ac477e in virKeepAliveTimer (timer=＜optimized out＞, opaque=0x7fd8e800b9c0) at rpc/virkeepalive.c:177
#4  0x00007fd8f7e5c9cf in libvirt_virEventInvokeTimeoutCallback () from /usr/lib64/python2.7/site-packages/libvirtmod.so
#5  0x00007fd8ff64db94 in PyEval_EvalFrameEx () from /lib64/libpython2.7.so.1.0
#6  0x00007fd8ff64f1ad in PyEval_EvalCodeEx () from /lib64/libpython2.7.so.1.0
#7  0x00007fd8ff64d85f in PyEval_EvalFrameEx () from /lib64/libpython2.7.so.1.0
#8  0x00007fd8ff64d950 in PyEval_EvalFrameEx () from /lib64/libpython2.7.so.1.0
#9  0x00007fd8ff64d950 in PyEval_EvalFrameEx () from /lib64/libpython2.7.so.1.0
#10 0x00007fd8ff64f1ad in PyEval_EvalCodeEx () from /lib64/libpython2.7.so.1.0
#11 0x00007fd8ff5dc098 in function_call () from /lib64/libpython2.7.so.1.0
#12 0x00007fd8ff5b7073 in PyObject_Call () from /lib64/libpython2.7.so.1.0
#13 0x00007fd8ff5c6085 in instancemethod_call () from /lib64/libpython2.7.so.1.0
#14 0x00007fd8ff5b7073 in PyObject_Call () from /lib64/libpython2.7.so.1.0
#15 0x00007fd8ff648ff7 in PyEval_CallObjectWithKeywords () from /lib64/libpython2.7.so.1.0
#16 0x00007fd8ff67d7e2 in t_bootstrap () from /lib64/libpython2.7.so.1.0
#17 0x00007fd8ff358df3 in start_thread () from /lib64/libpthread.so.0
#18 0x00007fd8fe97d3ed in clone () from /lib64/libc.so.6
Signed-off-by: NYi Wang <wang.yi59@zte.com.cn>
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>

In the RPC client event loop code, if poll() returns only a POLLHUP
or POLLERR status, then we end up reporting a bogus error message:

  error: failed to connect to the hypervisor
  error: An error occurred, but the cause is unknown

We do actually report an error, but we virNetClientMarkClose method
has already captured the error status before we report it, so the
real error gets thrown away. The key fix is to report the error
before calling virNetClientMarkClose(). In changing this, we also
split out reporting of POLLHUP vs POLLERR to make any future bugs
easier to diagnose.
Signed-off-by: NDaniel P. Berrange <berrange@redhat.com>

Apple have annotated all SASL functions as deprecated for
unknown reasons. Since they still work, lets just ignore
the warnings. If Apple finally delete the SASL functions
our configure check should already catch that
Signed-off-by: NDaniel P. Berrange <berrange@redhat.com>

CLang's optimizer is more aggressive at inlining functions than
gcc and so will often inline functions that our tests want to
mock-override. This causes the test to fail in bizarre ways.

We don't want to disable inlining completely, but we must at
least prevent inlining of mocked functions. Fortunately there
is a 'noinline' attribute that lets us control this per function.

A syntax check rule is added that parses tests/*mock.c to extract
the list of functions that are mocked (restricted to names starting
with 'vir' prefix). It then checks that src/*.h header file to
ensure it has a 'ATTRIBUTE_NOINLINE' annotation. This should prevent
use from bit-rotting in future.
Signed-off-by: NDaniel P. Berrange <berrange@redhat.com>