Re: [PATCH 07/13] Add libvirt-admin library

Martin Kletzander <mkletzan@xxxxxxxxxx> · Thu, 21 May 2015 22:22:26 -0700

On Thu, May 21, 2015 at 05:46:59PM +0200, Michal Privoznik wrote:
On 20.05.2015 07:19, Martin Kletzander wrote:
Initial scratch of the admin library.  It has its own virAdmConnectPtr
that inherits from virAbstractConnectPtr and thus trivially supports
error reporting.

There's pkg-config file added and spec-file adjusted as well.

Since the library should be "minimalistic" and not depend on any other
library, the list of files is especially crafted for it.  Most of them
could've been put to it's own sub-libraries that would be LIBADD'd to
libvirt_util, libvirt_net_rpc and libvirt_setuid_rpc_client to minimize
the number of object files being built, but that's a refactoring that
isn't the orginal aim of this commit.

Signed-off-by: Martin Kletzander <mkletzan@xxxxxxxxxx>
---
 cfg.mk                          |   1 +
 configure.ac                    |   4 +-
 include/libvirt/Makefile.am     |   4 +-
 include/libvirt/libvirt-admin.h |  62 +++++++
 libvirt-admin.pc.in             |  13 ++
 libvirt.spec.in                 |   7 +
 po/POTFILES.in                  |   1 +
 src/Makefile.am                 | 106 ++++++++++-
 src/datatypes.c                 |  30 ++++
 src/datatypes.h                 |  37 ++++
 src/internal.h                  |   1 +
 src/libvirt-admin.c             | 386 ++++++++++++++++++++++++++++++++++++++++
 src/libvirt_admin.syms          |  18 ++
 src/rpc/gendispatch.pl          |   4 +-
 14 files changed, 669 insertions(+), 5 deletions(-)
 create mode 100644 include/libvirt/libvirt-admin.h
 create mode 100644 libvirt-admin.pc.in
 create mode 100644 src/libvirt-admin.c
 create mode 100644 src/libvirt_admin.syms

diff --git a/libvirt-admin.pc.in b/libvirt-admin.pc.in
new file mode 100644
index 0000000..76126ae
--- /dev/null
+++ b/libvirt-admin.pc.in
@@ -0,0 +1,13 @@
+prefix=@prefix@
+exec_prefix=@exec_prefix@
+libdir=@libdir@
+includedir=@includedir@
+datarootdir=@datarootdir@
+
+libvirt_admin_api=@datadir@/libvirt/api/libvirt-admin-api.xml
+
+Name: libvirt-admin
+Version: @VERSION@
+Description: libvirt admin library
+Libs: -L${libdir} -lvirt-admin
+Cflags: -I${includedir}

This file should be put into AC_CONFIG_FILES() too. And into EXTRA_DIST
diff --git a/libvirt.spec.in b/libvirt.spec.in
index 4195518..afcfe31 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -2206,6 +2206,12 @@ exit 0
 %attr(0755, root, root) %{_libexecdir}/libvirt_sanlock_helper
 %endif

+%if %{with_admin}

This doesn't make much sense. libvirt-admin is build unconditionally.
Moreover, the macro {with_admin} is never defined.

+%files admin
+%defattr(-, root, root)
+%{_libdir}/%{name}/libvirt_admin.so

Nope. It's installed under different path.

+%endif

Unfortunately, you haven't defined admin package. So this won't fly.


Me and specfiles, we were never huge friends.

+
 %files client -f %{name}.lang
 %defattr(-, root, root)
 %doc COPYING COPYING.LESSER
@@ -2298,6 +2304,7 @@ exit 0
 %{_includedir}/libvirt/libvirt-stream.h
 %{_includedir}/libvirt/libvirt-qemu.h
 %{_includedir}/libvirt/libvirt-lxc.h
+%{_includedir}/libvirt/libvirt-admin.h
 %{_libdir}/pkgconfig/libvirt.pc
 %{_libdir}/pkgconfig/libvirt-qemu.pc
 %{_libdir}/pkgconfig/libvirt-lxc.pc
diff --git a/po/POTFILES.in b/po/POTFILES.in
index ebb0482..4afa2f9 100644
--- a/po/POTFILES.in
+++ b/po/POTFILES.in
@@ -59,6 +59,7 @@ src/interface/interface_backend_netcf.c
 src/interface/interface_backend_udev.c
 src/internal.h
 src/libvirt.c
+src/libvirt-admin.c
 src/libvirt-domain.c
 src/libvirt-domain-snapshot.c
 src/libvirt-host.c
diff --git a/src/Makefile.am b/src/Makefile.am
index e8dce78..1241d6d 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -501,6 +501,7 @@ PROTOCOL_STRUCTS = \
 	$(srcdir)/virkeepaliveprotocol-structs \
 	$(srcdir)/lxc_monitor_protocol-structs \
 	$(srcdir)/lock_protocol-structs \
+	$(srcdir)/admin_protocol-structs \
 	$(NULL)

 if WITH_REMOTE
@@ -522,6 +523,9 @@ $(srcdir)/lxc_monitor_protocol-struct: \
 $(srcdir)/lock_protocol-struct: \
 		$(srcdir)/%-struct: locking/lockd_la-%.lo
 	$(PDWTAGS)
+$(srcdir)/admin_protocol-struct: \
+		$(srcdir)/%-struct: admin/libvirt_admin_la-%.lo
+	$(PDWTAGS)

 else !WITH_REMOTE
 # The $(PROTOCOL_STRUCTS) files must live in git, because they cannot be
@@ -534,6 +538,7 @@ check-drivername:
 	$(AM_V_GEN)$(PERL) $(srcdir)/check-drivername.pl \
 		$(srcdir)/driver.h \
 		$(srcdir)/libvirt_public.syms \
+		$(srcdir)/libvirt_admin.syms \
 		$(srcdir)/libvirt_qemu.syms \
 		$(srcdir)/libvirt_lxc.syms

@@ -1092,6 +1097,7 @@ USED_SYM_FILES = $(srcdir)/libvirt_private.syms
 GENERATED_SYM_FILES = \
 	$(ACCESS_DRIVER_SYM_FILES) \
 	libvirt.syms libvirt.def libvirt_qemu.def libvirt_lxc.def \
+	libvirt_admin.def \
 	$(NULL)

 if WITH_TEST
@@ -1803,7 +1809,8 @@ EXTRA_DIST +=							\
 		$(VBOX_DRIVER_EXTRA_DIST)			\
 		$(VMWARE_DRIVER_SOURCES)			\
 		$(XENCONFIG_SOURCES)				\
-		$(ACCESS_DRIVER_POLKIT_POLICY)
+		$(ACCESS_DRIVER_POLKIT_POLICY)			\
+		$(libvirt_admin_la_SOURCES)

This is not necessary. libvirt-admin is build unconditionally.


 check-local: check-augeas

@@ -2000,6 +2007,7 @@ EXTRA_DIST += \
 	libvirt_public.syms		\
 	libvirt_lxc.syms		\
 	libvirt_qemu.syms		\
+	libvirt_admin.syms		\
 	$(SYM_FILES)			\
 	$(NULL)

@@ -2043,6 +2051,102 @@ libvirt_lxc.def: $(srcdir)/libvirt_lxc.syms
 	chmod a-w $@-tmp && \
 	mv $@-tmp libvirt_lxc.def

+libvirt_admin.def: $(srcdir)/libvirt_admin.syms
+	$(AM_V_GEN)rm -f -- $@-tmp $@ ; \
+	printf 'EXPORTS\n' > $@-tmp && \
+	sed -e '/^$$/d; /#/d; /:/d; /}/d; /\*/d; /LIBVIRT_/d'	\
+	    -e 's/[	 ]*\(.*\)\;/    \1/g' $^ >> $@-tmp && \
+	chmod a-w $@-tmp && \
+	mv $@-tmp libvirt_admin.def

This pattern repeats itself already. Maybe one day we can turn it into
a general rule how to make .def from .syms.


Done.

+
+lib_LTLIBRARIES += libvirt-admin.la
+libvirt_admin_la_SOURCES = \
+		libvirt-admin.c			\
+		$(ADMIN_PROTOCOL_GENERATED)
+
+libvirt_admin_la_SOURCES += 			\

Spurious space after =.

+		datatypes.c			\
+		util/viralloc.c			\
+		util/viratomic.c		\
+		util/virauth.c			\
+		util/virauthconfig.c		\
+		util/virbitmap.c		\
+		util/virbuffer.c		\
+		util/vircommand.c		\
+		util/virerror.c			\
+		util/virevent.c			\
+		util/vireventpoll.c		\
+		util/virfile.c			\
+		util/virhash.c			\
+		util/virhashcode.c		\
+		util/virjson.c			\
+		util/virkeyfile.c		\
+		util/virlog.c			\
+		util/virobject.c		\
+		util/virpidfile.c		\
+		util/virprocess.c		\
+		util/virrandom.c		\
+		util/virseclabel.c		\
+		util/virsocketaddr.c		\
+		util/virstorageencryption.c	\
+		util/virstoragefile.c		\
+		util/virstring.c		\
+		util/virthread.c		\
+		util/virtime.c			\
+		util/virtypedparam.c		\
+		util/viruri.c			\
+		util/virutil.c			\
+		util/viruuid.c			\
+		util/virxml.c			\
+		remote/remote_protocol.c	\

This drags in (de)serializers for all the public APIs we have. I guess
you have it here just becase of serializers for some basic types (e.g.
string). Well, if we introduce a separate libvirt_admin.x file, rpcgen
will generate the serializers again, and just for the types we need. So
I think it's safe to drop this line (and libvirt-admin.so will lose
some weight).


Yes, but I have to rename that to something else than remote_string
because that would collide in the libvirt daemon.  That would mean I
have to add each of the new names to gendispatch.pl.  And so on and so
on, just to get rid of some (de)serializers in the client library.

I'll do that for remote_string for now, but if there are more than
that, we should probably put those into another file.  Well, you'll
see how much bigger the diff will be even now.

Then, I wonder if we need to recompile nearly all utils/ again. Can't
we just link libvirt_utils.so in?


Well, I wanted to make it lightweight even when it increases
compilation time as it looks like we are constantly OK with that
(setuid_rpc_client, vbox libs, etc.), but as I said in the commit
message, I'd like to see a commit that minimizes the files being
compiled.

+		rpc/virnetmessage.h		\
+		rpc/virnetmessage.c		\
+		rpc/virnetsocket.c		\
+		rpc/virnetsshsession.c		\
+		rpc/virkeepalive.c		\
+		rpc/virnetclient.c		\
+		rpc/virnetclientprogram.c	\
+		rpc/virnetclientstream.c	\
+		rpc/virnetprotocol.c		\
+		rpc/virnettlscontext.c		\
+		rpc/virnetsaslcontext.c

SSH, TLS and SASL? It's going to be a local socket only. I guess we
don't need them. Or is it some kind of black magic of dependencies?


No magic, this is just our ugly way of handling WITH_GNUTLS nd
WITH_SASL that drags dependencies around.  I started the cleanup for
this, but since it's pretty deeply engrained in the code, I haven't
really found the time (and energy) to finish it.

+
+libvirt_admin_la_LDFLAGS = \
+		$(VERSION_SCRIPT_FLAGS)$(LIBVIRT_ADMIN_SYMBOL_FILE)	\
+		-version-info $(LIBVIRT_VERSION_INFO)			\
+		$(AM_LDFLAGS) 						\
+		$(CYGWIN_EXTRA_LDFLAGS) 				\
+		$(MINGW_EXTRA_LDFLAGS)
+
+libvirt_admin_la_LIBADD = \
+		$(CYGWIN_EXTRA_LIBADD)
+
+libvirt_admin_la_CFLAGS = \
+		$(AM_CFLAGS)		\
+		-I$(srcdir)/remote	\
+		-I$(srcdir)/rpc		\
+		-I$(srcdir)/admin
+
+libvirt_admin_la_CFLAGS += \
+		$(CAPNG_CFLAGS)			\
+		$(YAJL_CFLAGS)			\
+		$(SSH2_CFLAGS)			\
+		$(SASL_CFLAGS)			\
+		$(GNUTLS_CFLAGS)
+
+libvirt_admin_la_LIBADD += \
+		$(CAPNG_LIBS)			\
+		$(YAJL_LIBS)			\
+		$(DEVMAPPER_LIBS)		\
+		$(LIBXML_LIBS)			\
+		$(SSH2_LIBS)			\
+		$(SASL_LIBS)			\
+		$(GNUTLS_LIBS)
+

diff --git a/src/libvirt_admin.syms b/src/libvirt_admin.syms
new file mode 100644
index 0000000..ea6a8cc
--- /dev/null
+++ b/src/libvirt_admin.syms
@@ -0,0 +1,18 @@
+#
+# Officially exported symbols, for which header
+# file definitions are installed in /usr/include/libvirt
+# from libvirt-admin.h
+#
+# Versions here are *fixed* to match the libvirt version
+# at which the symbol was introduced. This ensures that
+# a new client app requiring symbol foo() can't accidentally
+# run with old libvirt-admin.so not providing foo() - the global
+# soname version info can't enforce this since we never
+# change the soname
+#
+LIBVIRT_ADMIN_1.3.0 {
+    global:
+        virAdmInitialize;
+        virAdmConnectOpen;
+        virAdmConnectClose;


I wonder if we should introduce (and implement) virAdmConnectRef. For
instance, if you have a multithreaded application, you open the
connection, and then spawn threads. But for some reason, you want to
have virAdmConnectClose in threads. Therefore each thread should
increase the reference counter on the connection objects, so the first
one to call the close() won't close the connection for the others.


It is implemented right in this series.  I just forgot to put it here ;)


+};
diff --git a/src/rpc/gendispatch.pl b/src/rpc/gendispatch.pl
index 5097e13..dda04a9 100755
--- a/src/rpc/gendispatch.pl
+++ b/src/rpc/gendispatch.pl
@@ -1255,8 +1255,8 @@ elsif ($mode eq "client") {
             }
         }

-        if (($structprefix ne "admin") && !@args_list) {
-            push(@args_list, "virConnectPtr conn");
+        if (!@args_list) {
+            push(@args_list, "$connect_ptr conn");
         }

         # handle return values of the function


This needs to be squashed in at least:


That itself won't work.  But I squashed way more.  And created another
commit or two.  Stay tuned!
Attachment:
signature.asc

Description: PGP signature
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



