11:14 a.m.
On Wed, Mar 21, 2018 at 02:21:05PM +0000, Daniel P. Berrangé wrote:
On Wed, Mar 21, 2018 at 11:02:43AM +0100, Pavel Hrdina wrote:
> We will switch to GDBus implementation of D-Bus protocol because
> sd-bus implementation is not thread safe.
>
> Processing messages in threads is essential since Libvirt API can
> take some significant amount of time to return and that would block
> the whole libvirt-dbus daemon.
>
> Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
> ---
>
> Changes in v3:
> - fixed a bug while loading XML interface files, now it fails
> gracefully with error message
>
> Changes in v2:
> - changed glib2 required version to 2.44.0, required for the
> auto-cleanup macros
> - changed libvirt-glib required version to 0.0.7
>
> README | 1 +
> configure.ac | 12 ++
> data/Makefile.am | 5 +
> libvirt-dbus.spec.in | 6 +
> src/Makefile.am | 17 ++-
> src/gdbus.c | 398 +++++++++++++++++++++++++++++++++++++++++++++++++++
> src/gdbus.h | 108 ++++++++++++++
> test/Makefile.am | 3 +-
> test/travis-run | 2 +-
> 9 files changed, 547 insertions(+), 5 deletions(-)
> create mode 100644 src/gdbus.c
> create mode 100644 src/gdbus.h
>
> diff --git a/README b/README
> index 754d957..a85114e 100644
> --- a/README
> +++ b/README
> @@ -58,6 +58,7 @@ The packages required to build libvirt-dbus are
>
> - systemd-211
> - libvirt
> + - glib2
>
> Patches submissions
> ===================
> diff --git a/configure.ac b/configure.ac
> index df1a375..0e3bc01 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -11,10 +11,14 @@ AC_USE_SYSTEM_EXTENSIONS
>
> AM_SILENT_RULES([yes])
>
> +GLIB2_REQUIRED=2.44.0
> LIBVIRT_REQUIRED=1.2.8
> SYSTEMD_REQUIRED=211
> +LIBVIRT_GLIB_REQUIRED=0.0.7
> +AC_SUBST([GLIB2_REQUIRED]) dnl used in the .spec file
> AC_SUBST([LIBVIRT_REQUIRED]) dnl used in the .spec file
> AC_SUBST([SYSTEMD_REQUIRED]) dnl used in the .spec file
> +AC_SUBST([LIBVIRT_GLIB_REQUIRED]) dnl used in the .spec file
>
> LIBVIRT_DBUS_MAJOR_VERSION=`echo $VERSION | awk -F. '{print $1}'`
> LIBVIRT_DBUS_MINOR_VERSION=`echo $VERSION | awk -F. '{print $2}'`
> @@ -34,8 +38,11 @@ AC_PROG_MKDIR_P
> AM_PROG_CC_C_O
> AC_PROG_CC_STDC
>
> +PKG_CHECK_MODULES(GIO2, gio-unix-2.0 >= GLIB2_REQUIRED)
> +PKG_CHECK_MODULES(GLIB2, glib-2.0 >= GLIB2_REQUIRED)
> PKG_CHECK_MODULES(LIBVIRT, libvirt >= $LIBVIRT_REQUIRED)
> PKG_CHECK_MODULES(SYSTEMD, libsystemd >= $SYSTEMD_REQUIRED)
> +PKG_CHECK_MODULES(LIBVIRT_GLIB, libvirt-glib-1.0 >= LIBVIRT_GLIB_REQUIRED)
>
> LIBVIRT_COMPILE_WARNINGS
> LIBVIRT_LINKER_RELRO
> @@ -56,6 +63,11 @@ LIBVIRT_ARG_WITH([DBUS_SYSTEM_POLICIES], [where D-Bus system
policies directory
> DBUS_SYSTEM_POLICIES_DIR="$with_dbus_system_policies"
> AC_SUBST(DBUS_SYSTEM_POLICIES_DIR)
>
> +LIBVIRT_ARG_WITH([DBUS_INTERFACES], [where D-Bus interfaces directory is],
> + ['$(datadir)/dbus-1/interfaces'])
> +DBUS_INTERFACES_DIR="$with_dbus_interfaces"
> +AC_SUBST([DBUS_INTERFACES_DIR])
FYI, you can actually get the default value for this from
pkg-config
$ pkgconf --variable interfaces_dir dbus-1
/usr/share/dbus-1/interfaces
And arguably don't need to make that configurable install dir
at all with args. Not a blocker for this though, so fine if
you patch that separately.
You would need a BuildRequires on dbus-devel to get the pkgconfig
file though.
Good point, I'll send a followup patch.
> diff --git a/data/Makefile.am b/data/Makefile.am
> index 9a53305..a886687 100644
> --- a/data/Makefile.am
> +++ b/data/Makefile.am
> @@ -18,11 +18,16 @@ polkit_files = \
> polkitdir = $(sysconfdir)/polkit-1/rules.d
> polkit_DATA = $(polkit_files:.rules.in=.rules)
>
> +interfaces_files =
> +interfacesdir = $(DBUS_INTERFACES_DIR)
> +interfaces_DATA = $(interfaces_files)
> +
> EXTRA_DIST = \
> $(service_in_files) \
> $(system_service_in_files) \
> $(system_policy_files) \
> $(polkit_files) \
> + $(interfaces_files) \
> $(NULL)
>
> CLEANFILES = \
> diff --git a/libvirt-dbus.spec.in b/libvirt-dbus.spec.in
> index ce5cecf..662ece1 100644
> --- a/libvirt-dbus.spec.in
> +++ b/libvirt-dbus.spec.in
> @@ -1,7 +1,9 @@
> # -*- rpm-spec -*-
>
> +%define glib2_version @GLIB2_REQUIRED@
> %define libvirt_version @LIBVIRT_REQUIRED@
> %define systemd_version @SYSTEMD_REQUIRED@
> +%define libvirt_glib_version @LIBVIRT_GLIB_REQUIRED@
> %define system_user @SYSTEM_USER@
>
> Name: @PACKAGE@
> @@ -15,11 +17,15 @@ Source0:
ftp://libvirt.org/libvirt/dbus/%{name}-%{version}.tar.gz
> BuildRoot: %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
>
> BuildRequires: gcc
> +BuildRequires: glib2-devel >= %{glib2_version}
> BuildRequires: libvirt-devel >= %{libvirt_version}
> BuildRequires: systemd-devel >= %{systemd_version}
> +BuildRequires: libvirt-glib-devel >= %{libvirt_glib_version}
>
> +Requires: glib2 >= %{glib2_version}
> Requires: libvirt-libs >= %{libvirt_version}
> Requires: systemd-libs >= %{systemd_version}
> +Requires: libvirt-glib >= %{libvirt_glib_version}
>
> Requires(pre): shadow-utils
>
> diff --git a/src/Makefile.am b/src/Makefile.am
> index 1a5b50b..e7bba9d 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -1,12 +1,14 @@
> AM_CPPFLAGS = \
> - -I$(top_srcdir)/src
> + -I$(top_srcdir)/src \
> + -DVIRT_DBUS_INTERFACES_DIR=\"$(DBUS_INTERFACES_DIR)\"
>
> DAEMON_SOURCES = \
> main.c \
> connect.c connect.h \
> util.c util.h \
> domain.c domain.h \
> - events.c events.h
> + events.c events.h \
> + gdbus.c gdbus.h
>
> EXTRA_DIST = \
> $(DAEMON_SOURCES)
> @@ -18,18 +20,27 @@ libvirt_dbus_SOURCES = $(DAEMON_SOURCES)
>
> libvirt_dbus_CFLAGS = \
> $(SYSTEMD_CFLAGS) \
> + $(GIO2_CFLAGS) \
> + $(GLIB2_CFLAGS) \
> $(LIBVIRT_CFLAGS) \
> + $(LIBVIRT_GLIB_CFLAGS) \
> $(WARN_CFLAGS) \
> $(PIE_CFLAGS) \
> $(NULL)
>
> libvirt_dbus_LDFLAGS = \
> $(SYSTEMD_LDFLAGS) \
> + $(GIO2_LDFLAGS) \
> + $(GLIB2_LDFLAGS) \
> $(LIBVIRT_LDFLAGS) \
> + $(LIBVIRT_GLIB_LDFLAGS) \
> $(RELRO_LDFLAGS) \
> $(PID_LDFLAGS) \
> $(NULL)
>
> libvirt_dbus_LDADD = \
> $(SYSTEMD_LIBS) \
> - $(LIBVIRT_LIBS)
> + $(GIO2_LIBS) \
> + $(GLIB2_LIBS) \
> + $(LIBVIRT_LIBS) \
> + $(LIBVIRT_GLIB_LIBS)
> +static void
> +virtDBusGDBusHandlePropertyGet(GVariant *parameters,
> + GDBusMethodInvocation *invocation,
> + const gchar *objectPath,
> + virtDBusGDBusMethodData *data)
> +{
> + virtDBusGDBusPropertyGetFunc getFunc = NULL;
> + const gchar *interface;
> + const gchar *name;
> + GVariant *value = NULL;
> + GError *error = NULL;
> +
> + g_variant_get(parameters, "(&s&s)", &interface,
&name);
> +
> + for (gint i = 0; data->properties[i].name; i++) {
> + if (g_strcmp0(name, data->properties[i].name) == 0) {
Nitpick g_str_equal() - no need to repost for that though. Sevaral
similar cases below that I won't repeat.
I've missed this one because it's for hash tables. The only difference
is that this one is not NULL-safe, but all the places should be already
safe not to pass NULL.
> + getFunc = data->properties[i].getFunc;
> + break;
> + }
> + }
> +void
> +virtDBusGDBusRegisterSubtree(GDBusConnection *bus,
> + gchar const *objectPath,
> + GDBusInterfaceInfo *interface,
> + virtDBusGDBusEnumerateFunc enumerate,
> + virtDBusGDBusMethodTable *methods,
> + virtDBusGDBusPropertyTable *properties,
> + gpointer userData);
Not sure how much you care, but if you want libvirt-dbus APIs to
match glib conventions, then method names would be all lowercsae
and underscore separated
If this would by library I would use the snake_case style to match
glib conventions, but since the origin code already uses CamelCase
I was not feeling like change it.
Regardless of comments,
Reviewed-by: Daniel P. Berrangé <berrange(a)redhat.com>
Thanks for review.
Pavel