Re: [dbus PATCH v3 1/5] introduce support for GDBus implementation

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux