[libvirt] [PATCH 07/13] Add libvirt-admin library
Martin Kletzander
mkletzan at redhat.com
Fri May 22 05:22:26 UTC 2015
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 at redhat.com>
>> ---
>> 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!
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150521/4d55730c/attachment-0001.sig>
More information about the libvir-list
mailing list