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

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(a)redhat.com&gt; > --- > 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(a)/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.

> + > %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.

> + > +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).

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

> + 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?

> + > +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.

> +}; > 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: