Use virAppendElement instead of virInsertElementsN to implement
VIR_APPEND_ELEMENT which allows us to remove error handling as the
only relevant errors were removed when switching to aborting memory
allocation functions.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Historically, we declared pointer type to our types:
typedef struct _virXXX virXXX;
typedef virXXX *virXXXPtr;
But usefulness of such declaration is questionable, at best.
Unfortunately, we can't drop every such declaration - we have to
carry some over, because they are part of public API (e.g.
virDomainPtr). But for internal types - we can do drop them and
use what every other C project uses 'virXXX *'.
This change was generated by a very ugly shell script that
generated sed script which was then called over each file in the
repository. For the shell script refer to the cover letter:
https://listman.redhat.com/archives/libvir-list/2021-March/msg00537.html
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Recent changes which meant to fix daemon shutdown broke the exec-restart
capability of virtlogd and virtlockd, since the code actually closed all
the sockets and shut down all the internals.
Add virNetDaemonQuitExecRestart, which requests a shutdown of the
process, but keeps all the services open and registered since they are
preserved across the restart.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
The parent object takes ownership of the inserted value once all checks
pass. Don't make the callers second-guess when that happens and modify
the function to take a double pointer so that it can be cleared once the
ownership is taken.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
When the host is shutting down then we get PrepareForShutdown
signal on DBus to which we react by creating a thread which
runs virStateStop() and thus qemuStateStop(). But if scheduling
the thread is delayed just a but it may happen that we receive
SIGTERM (sent by systemd) to which we respond by quitting our
event loop and cleaning up everything (including drivers). And
only after that the thread gets to run only to find qemu_driver
being NULL.
What we can do is to delay exiting event loop and join the thread
that's executing virStateStop(). If the join doesn't happen in
given timeout (currently 30 seconds) then libvirtd shuts down
forcefully anyways (see virNetDaemonRun()).
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1895359
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1739564
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Don't hide our use of GHashTable behind our typedef. This will also
promote the use of glibs hash function directly.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Reviewed-by: Matt Coleman <matt@datto.com>
All but one of the callers either use the list in arbitrary order or
sorted by key. Rewrite the function so that it supports sorting by key
natively and make it return the element count. This in turn allows to
rewrite the only caller to sort by value internally.
This allows to remove multiple sorting functions which were sorting by
key and the function will be also later reused for some hash operations
internally.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Reviewed-by: Matt Coleman <matt@datto.com>
All users of virHashTable pass strings as the name/key of the entry.
Make this an official requirement by turning the variables to 'const
char *'.
For any other case it's better to use glib's GHashTable.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
It doesn't make much sense to configure the bucket count in the hash
table for each case specifically. Replace all calls of virHashCreate
with virHashNew which has a pre-set size and remove virHashCreate
completely.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
g_variant_new() returns a weak reference which can be consumed by passing
to other g_variant* functions or to g_dbus_connection_call* functions.
This make it possible to call g_variant_new() directly as argument to
the functions above. Because this might be confusing I explicitly call
g_variant_ref_sink() to make it normal reference in both
virGDBusCallMethod() and virGDBusCallMethodWithFD() so the caller is
always responsible for the data.
Reported-by: Peter Krempa <pkrempa@redhat.com>
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
We used to check the format of reply data with libdbus so we should do
the same with GLib DBus as well.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Currently we have issues like [1] on libvirtd shutdown as we cleanup while RPC
and other threads are still running. Let's finish all threads other then main
before cleanup.
The approach to finish threads is suggested in [2]. In order to finish RPC
threads serving API calls we let the event loop run but stop accepting new API
calls and block processing any pending API calls. We also inform all drivers of
shutdown so they can prepare for shutdown too. Then we wait for all RPC threads
and driver's background thread to finish. If finishing takes more then 15s we
just exit as we can't safely cleanup in time.
[1] https://bugzilla.redhat.com/show_bug.cgi?id=1828207
[2] https://www.redhat.com/archives/libvir-list/2020-April/msg01328.html
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
The function is used to set shutdown prepare and wait callbacks. Prepare
callback is used to inform other threads of the daemon that the daemon will be
closed soon so that they can start to shutdown. Wait callback is used to wait
for other threads to actually finish.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Use https: links for websites that support them.
The URIs which are used as namespace identifiers
are left alone.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Reviewed-by: Neal Gompa <ngompa13@gmail.com>
Make it obvious that the function always returns a valid pointer and fix
all callers.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
This addreses portability to Windows and standardizes
error reporting. This fixes a number of places which
failed to set O_CLOEXEC or failed to report errors.
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
The Windows platform does not have the signal handling
support we need, so it must be disabled in several parts
of the codebase.
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Use G_GNUC_UNUSED from GLib instead of ATTRIBUTE_UNUSED.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
As a side effect, this also silences the possible:
internal error: Unable to get DBus system bus connection:
Failed to connect to socket /run/dbus/system_bus_socket:
No such file or directory
error, since we check upfront whether dbus is available.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Libvirtd has long had integration with avahi for advertising libvirtd
using mDNS when TCP/TLS listening is enabled. For a long time the
virt-manager application had support for auto-detecting libvirtds
on the local network using mDNS, but this was removed last year
commit fc8f8d5d7e3ba80a0771df19cf20e84a05ed2422
Author: Cole Robinson <crobinso@redhat.com>
Date: Sat Oct 6 20:55:31 2018 -0400
connect: Drop avahi support
Libvirtd can advertise itself over avahi. The feature is disabled by
default though and in practice I hear of no one actually using it
and frankly I don't think it's all that useful
The 'Open Connection' wizard has a disproportionate amount of code
devoted to this feature, but I don't think it's useful or worth
maintaining, so let's drop it
I've never heard of any other applications having support for using
mDNS to detect libvirtd instances. Though it is theoretically possible
something exists out there, it is clearly going to be a niche use case
in the virt ecosystem as a whole.
By removing avahi integration we can cut down the dependency chain for
the basic libvirtd install and reduce our code maint burden.
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Missing semicolon at the end of macros can confuse some analyzers
(like cppcheck <filename>). VIR_ONCE_GLOBAL_INIT is almost
exclusively called without an ending semicolon, but let's
standardize on using one like the other macros.
Add a dummy struct definition at the end of the macro, so
the compiler will require callers to add a semicolon.
Reviewed-by: John Ferlan <jferlan@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
In many files there are header comments that contain an Author:
statement, supposedly reflecting who originally wrote the code.
In a large collaborative project like libvirt, any non-trivial
file will have been modified by a large number of different
contributors. IOW, the Author: comments are quickly out of date,
omitting people who have made significant contribitions.
In some places Author: lines have been added despite the person
merely being responsible for creating the file by moving existing
code out of another file. IOW, the Author: lines give an incorrect
record of authorship.
With this all in mind, the comments are useless as a means to identify
who to talk to about code in a particular file. Contributors will always
be better off using 'git log' and 'git blame' if they need to find the
author of a particular bit of code.
This commit thus deletes all Author: comments from the source and adds
a rule to prevent them reappearing.
The Copyright headers are similarly misleading and inaccurate, however,
we cannot delete these as they have legal meaning, despite being largely
inaccurate. In addition only the copyright holder is permitted to change
their respective copyright statement.
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
All of the ones being removed are pulled in by internal.h. The only
exception is sanlock which expects the application to include <stdint.h>
before sanlock's headers, because sanlock prototypes use fixed width
int, but they don't include stdint.h themselves, so we have to leave
that one in place.
Signed-off-by: Erik Skultety <eskultet@redhat.com>
Acked-by: Michal Privoznik <mprivozn@redhat.com>
Rather than have virJSONValueArraySize return a -1 when the input
is not an array and then splat an error message, let's check for
an array before calling and then change the return to be a size_t
instead of ssize_t.
That means using the helper virJSONValueIsArray as well as using a
more generic error message such as "Malformed <something> array".
In some cases we can remove stack variables and when we cannot,
those variables should be size_t not ssize_t. Alter a few references
of if (!value) to be if (value == 0) instead as well.
Some callers can already assume an array is being worked on based
on the previous call, so there's less to do.
Signed-off-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
So far we are repeating the following lines over and over:
if (!(virSomeObjectClass = virClassNew(virClassForObject(),
"virSomeObject",
sizeof(virSomeObject),
virSomeObjectDispose)))
return -1;
While this works, it is impossible to do some checking. Firstly,
the class name (the 2nd argument) doesn't match the name in the
code in all cases (the 3rd argument). Secondly, the current style
is needlessly verbose. This commit turns example into following:
if (!(VIR_CLASS_NEW(virSomeObject,
virClassForObject)))
return -1;
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
With the current code it is neccessary to call
virNetDaemonNewPostExecRestart()
and then for each server that needs restarting you are supposed
to call
virNetDaemonAddSeverPostExecRestart()
This is fine if there's only ever one server, but as soon as you
have two servers it is impossible to use this design. The code
has no idea which servers were recorded in the JSON state doc,
nor in which order the hash table serialized its keys.
So this patch changes things so that we only call
virNetDaemonNewPostExecRestart()
passing in a callback, which is invoked once for each server
found int he JSON state doc.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
It is not possible to blindly call virNetDaemonGetServer()
because in a post-exec restart scenario, some servers may
not exist and this method will pollute the error logs.
Signed-off-by: Daniel P. Berrange <berrange@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.
https://bugzilla.redhat.com/show_bug.cgi?id=1487322
In ace45e67ab 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: Michal Privoznik <mprivozn@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: Michal Privoznik <mprivozn@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: Daniel P. Berrange <berrange@redhat.com>
The DBus conditional was renamed way back:
commit da77f04ed5
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: Pavel Hrdina <phrdina@redhat.com>
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Commit 252610f7dd switched to use hash to store servers.
Function virHashGetItems returns allocated array which needs
to be freed also for successful path, not only if there is
an error.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Trying to reload/SIGUSR1 virtlogd or virtlockd fails with:
error : virNetDaemonRun:747 : internal error: Not all servers restored, cannot run server
Commit 252610f7 changed the daemon state json to allow tracking
multiple servers. However it missed clearing dmn->srvObject after
the json is empty, like the previous code paths handled. Later on in
virNewDaemonRun, dmn->srvObject is expected to be empty otherwise we
throw the above error.
https://bugzilla.redhat.com/show_bug.cgi?id=1311013
For now it does not matter which ones we return as the code is similarly
complex, however it will fit in with other constructs in the future,
mainly when we will be able to generate dispatch helpers.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
virHashForEach() returns 0 if everything went nice, so our session
daemon was timing out even when there was a client connected.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1315606
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
Since servers know their name, there is no need to supply such
information twice. Also defeats inconsistencies.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
At first I did not want to do this, but after trying to implement some
newer feaures in the admin API I realized we need that to make our lives
easier. On the other hand they are not saved redundantly and the
virNetServer objects are still kept in a hash table.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
This API is merely a convenience API, i.e. when managing clients connected to
daemon's servers, we should know (convenience) which server the specific client
is connected to. This implies a client-side representation of a server along
with a basic API to let the administrating client know what servers are actually
available on the daemon.
Signed-off-by: Erik Skultety <eskultet@redhat.com>
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
Since the daemon can manage and add (at fresh start) multiple servers,
we also should be able to add them from a JSON state file in case of a
daemon restart, so post exec restart support for multiple servers is also
provided. Patch also updates virnetdaemontest accordingly.
Signed-off-by: Erik Skultety <eskultet@redhat.com>
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
Apparently we are not the only ones with dumb free functions
because dbus_message_unref() does not accept NULL either. But if
I were to vote, this one is even more evil. Instead of returning
an error just like we do it immediately dereference any pointer
passed and thus crash you app. Well done DBus!
Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7f878ebda700 (LWP 31264)]
0x00007f87be4016e5 in ?? () from /usr/lib64/libdbus-1.so.3
(gdb) bt
#0 0x00007f87be4016e5 in ?? () from /usr/lib64/libdbus-1.so.3
#1 0x00007f87be3f004e in dbus_message_unref () from /usr/lib64/libdbus-1.so.3
#2 0x00007f87bf6ecf95 in virSystemdGetMachineNameByPID (pid=9849) at util/virsystemd.c:228
#3 0x00007f879761bd4d in qemuConnectCgroup (driver=0x7f87600a32a0, vm=0x7f87600c7550) at qemu/qemu_cgroup.c:909
#4 0x00007f87976386b7 in qemuProcessReconnect (opaque=0x7f87600db840) at qemu/qemu_process.c:3386
#5 0x00007f87bf6edfff in virThreadHelper (data=0x7f87600d5580) at util/virthread.c:206
#6 0x00007f87bb602334 in start_thread (arg=0x7f878ebda700) at pthread_create.c:333
#7 0x00007f87bb3481bd in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:109
(gdb) frame 2
#2 0x00007f87bf6ecf95 in virSystemdGetMachineNameByPID (pid=9849) at util/virsystemd.c:228
228 dbus_message_unref(reply);
(gdb) p reply
$1 = (DBusMessage *) 0x0
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Daemon used false logic for determining whether there were any clients.
When the timer was inactive, it was activated if at least one of the
servers did not have clients. So the bool was being flipped there and
back all the time in case there was one client, for example.
Initially introduced by fa14207368.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1240283
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
