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@xxxxxxxxxx>
> > ---
> >
> > 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@xxxxxxxxxx>
Thanks for review.
Pavel
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
