[libvirt] [PATCH] daemon: Drop dependency on libvirt-admin.so
Currently, the daemon requires libvirt-admin.so because the functions encoding/decoding RPC messages for admin APIs live there. But this makes it very hard to split admin API into its own separate package: if libvirt-admin.so is going to live in a separate package than the daemon, either both packages must be installed or none. Solve this by statically linking the RPC message handling functions with the daemon. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- I'm a bit torn. I'm tempted to rewrite some parts of admin API so that it follows the driver architecture we have for other areas, e.g. domain drivers. Until then, this patch is needed. daemon/Makefile.am | 2 +- src/Makefile.am | 15 +++++++++++++-- 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/daemon/Makefile.am b/daemon/Makefile.am index 927d16f..3b6aafe 100644 --- a/daemon/Makefile.am +++ b/daemon/Makefile.am @@ -152,7 +152,7 @@ libvirtd_admin_la_LDFLAGS = \ $(NO_INDIRECT_LDFLAGS) \ $(NULL) libvirtd_admin_la_LIBADD = \ - ../src/libvirt-admin.la + ../src/libvirt-admin-rpc.la man8_MANS = libvirtd.8 diff --git a/src/Makefile.am b/src/Makefile.am index 9f8b638..7c3cef6 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -592,7 +592,7 @@ $(srcdir)/lock_protocol-struct: \ $(srcdir)/%-struct: locking/lockd_la-%.lo $(PDWTAGS) $(srcdir)/admin_protocol-struct: \ - $(srcdir)/%-struct: admin/libvirt_admin_la-%.lo + $(srcdir)/%-struct: admin/%.lo $(PDWTAGS) else !WITH_REMOTE @@ -2165,10 +2165,20 @@ libvirt_admin.syms: libvirt_admin_public.syms $(ADMIN_SYM_FILES) \ # need to include it in the dist EXTRA_DIST += admin/admin_remote.c +noinst_LTLIBRARIES += \ + libvirt-admin-rpc.la + +libvirt_admin_rpc_la_SOURCES = \ + $(ADMIN_PROTOCOL_GENERATED) + +libvirt_admin_rpc_la_LDFLAGS = \ + $(AM_LDFLAGS) \ + $(CYGWIN_EXTRA_LDFLAGS) \ + $(MINGW_EXTRA_LDFLAGS) + lib_LTLIBRARIES += libvirt-admin.la libvirt_admin_la_SOURCES = \ libvirt-admin.c \ - $(ADMIN_PROTOCOL_GENERATED) \ $(DATATYPES_SOURCES) libvirt_admin_la_LDFLAGS = \ @@ -2180,6 +2190,7 @@ libvirt_admin_la_LDFLAGS = \ libvirt_admin_la_LIBADD = \ libvirt.la \ + libvirt-admin-rpc.la \ $(CYGWIN_EXTRA_LIBADD) libvirt_admin_la_CFLAGS = \ -- 2.8.4
On Fri, Jun 24, 2016 at 03:12:23PM +0200, Michal Privoznik wrote:
Currently, the daemon requires libvirt-admin.so because the functions encoding/decoding RPC messages for admin APIs live there. But this makes it very hard to split admin API into its own separate package: if libvirt-admin.so is going to live in a separate package than the daemon, either both packages must be installed or none. Solve this by statically linking the RPC message handling functions with the daemon.
I'm not sure I see any need for a separate package for libvirt-admin.so For libvirt-qemu.so and libvirt-lxc.so we keep them in libvirt-client RPM, and I'd expect libvirt-admin.so to be there too really. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 24.06.2016 15:33, Daniel P. Berrange wrote:
On Fri, Jun 24, 2016 at 03:12:23PM +0200, Michal Privoznik wrote:
Currently, the daemon requires libvirt-admin.so because the functions encoding/decoding RPC messages for admin APIs live there. But this makes it very hard to split admin API into its own separate package: if libvirt-admin.so is going to live in a separate package than the daemon, either both packages must be installed or none. Solve this by statically linking the RPC message handling functions with the daemon.
I'm not sure I see any need for a separate package for libvirt-admin.so For libvirt-qemu.so and libvirt-lxc.so we keep them in libvirt-client RPM, and I'd expect libvirt-admin.so to be there too really.
So libvirt-client would contain not only virsh (and other .so files) but virt-admin binary too? Okay, if that's what we want my patch is useless. If we, however, want a separate package for libvirt-admin (which is kind of special compared to libvirt-qemu.so and libvirt-lxc.so), then I guess we need this patch. Michal
On Fri, Jun 24, 2016 at 03:59:38PM +0200, Michal Privoznik wrote:
On 24.06.2016 15:33, Daniel P. Berrange wrote:
On Fri, Jun 24, 2016 at 03:12:23PM +0200, Michal Privoznik wrote:
Currently, the daemon requires libvirt-admin.so because the functions encoding/decoding RPC messages for admin APIs live there. But this makes it very hard to split admin API into its own separate package: if libvirt-admin.so is going to live in a separate package than the daemon, either both packages must be installed or none. Solve this by statically linking the RPC message handling functions with the daemon.
I'm not sure I see any need for a separate package for libvirt-admin.so For libvirt-qemu.so and libvirt-lxc.so we keep them in libvirt-client RPM, and I'd expect libvirt-admin.so to be there too really.
So libvirt-client would contain not only virsh (and other .so files) but virt-admin binary too? Okay, if that's what we want my patch is useless. If we, however, want a separate package for libvirt-admin (which is kind of special compared to libvirt-qemu.so and libvirt-lxc.so), then I guess we need this patch.
Hmm, i guess libvirt-admin is only needed if libvirtd is actually present on the host. So I guess we could argue that virt-admin and libvirt-admin.so should just be a part of libvirt-daemon RPM. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 24/06/16 16:06, Daniel P. Berrange wrote:
On Fri, Jun 24, 2016 at 03:59:38PM +0200, Michal Privoznik wrote:
On 24.06.2016 15:33, Daniel P. Berrange wrote:
On Fri, Jun 24, 2016 at 03:12:23PM +0200, Michal Privoznik wrote:
Currently, the daemon requires libvirt-admin.so because the functions encoding/decoding RPC messages for admin APIs live there. But this makes it very hard to split admin API into its own separate package: if libvirt-admin.so is going to live in a separate package than the daemon, either both packages must be installed or none. Solve this by statically linking the RPC message handling functions with the daemon.
I'm not sure I see any need for a separate package for libvirt-admin.so For libvirt-qemu.so and libvirt-lxc.so we keep them in libvirt-client RPM, and I'd expect libvirt-admin.so to be there too really.
So libvirt-client would contain not only virsh (and other .so files) but virt-admin binary too? Okay, if that's what we want my patch is useless. If we, however, want a separate package for libvirt-admin (which is kind of special compared to libvirt-qemu.so and libvirt-lxc.so), then I guess we need this patch.
Hmm, i guess libvirt-admin is only needed if libvirtd is actually present on the host. So I guess we could argue that virt-admin and libvirt-admin.so should just be a part of libvirt-daemon RPM.
Regards, Daniel
Well, yes, but although we currently only support local connections, it might not stay that way forever. I thought about supporting remote tunneled connection, in which case I think it would be better to place in the client package. Erik
On 24.06.2016 16:06, Daniel P. Berrange wrote:
On Fri, Jun 24, 2016 at 03:59:38PM +0200, Michal Privoznik wrote:
On 24.06.2016 15:33, Daniel P. Berrange wrote:
On Fri, Jun 24, 2016 at 03:12:23PM +0200, Michal Privoznik wrote:
Currently, the daemon requires libvirt-admin.so because the functions encoding/decoding RPC messages for admin APIs live there. But this makes it very hard to split admin API into its own separate package: if libvirt-admin.so is going to live in a separate package than the daemon, either both packages must be installed or none. Solve this by statically linking the RPC message handling functions with the daemon.
I'm not sure I see any need for a separate package for libvirt-admin.so For libvirt-qemu.so and libvirt-lxc.so we keep them in libvirt-client RPM, and I'd expect libvirt-admin.so to be there too really.
So libvirt-client would contain not only virsh (and other .so files) but virt-admin binary too? Okay, if that's what we want my patch is useless. If we, however, want a separate package for libvirt-admin (which is kind of special compared to libvirt-qemu.so and libvirt-lxc.so), then I guess we need this patch.
Hmm, i guess libvirt-admin is only needed if libvirtd is actually present on the host. So I guess we could argue that virt-admin and libvirt-admin.so should just be a part of libvirt-daemon RPM.
The more I think about it the more I think that our current split into RPM packages has some minor flaws. For instance, libvirt-daemon requires libvirt-client; just because libvirt-client has some libraries that are required by the daemon too. So what if we: a) introduce libvirt-libs.rpm where all the libraries would go) b) have libvirt-daemon depend on -libs instead of -client, c) have libvirt-client install just virsh. This way we can enable users who really want to have just the daemon installed on their system (e.g. because there's one centralized mgmt point having the client libs/binaries). Or am I just too spoilt by gentoo? :-) Michal
On Fri, Jun 24, 2016 at 05:04:30PM +0200, Michal Privoznik wrote:
On 24.06.2016 16:06, Daniel P. Berrange wrote:
On Fri, Jun 24, 2016 at 03:59:38PM +0200, Michal Privoznik wrote:
On 24.06.2016 15:33, Daniel P. Berrange wrote:
On Fri, Jun 24, 2016 at 03:12:23PM +0200, Michal Privoznik wrote:
Currently, the daemon requires libvirt-admin.so because the functions encoding/decoding RPC messages for admin APIs live there. But this makes it very hard to split admin API into its own separate package: if libvirt-admin.so is going to live in a separate package than the daemon, either both packages must be installed or none. Solve this by statically linking the RPC message handling functions with the daemon.
I'm not sure I see any need for a separate package for libvirt-admin.so For libvirt-qemu.so and libvirt-lxc.so we keep them in libvirt-client RPM, and I'd expect libvirt-admin.so to be there too really.
So libvirt-client would contain not only virsh (and other .so files) but virt-admin binary too? Okay, if that's what we want my patch is useless. If we, however, want a separate package for libvirt-admin (which is kind of special compared to libvirt-qemu.so and libvirt-lxc.so), then I guess we need this patch.
Hmm, i guess libvirt-admin is only needed if libvirtd is actually present on the host. So I guess we could argue that virt-admin and libvirt-admin.so should just be a part of libvirt-daemon RPM.
The more I think about it the more I think that our current split into RPM packages has some minor flaws. For instance, libvirt-daemon requires libvirt-client; just because libvirt-client has some libraries that are required by the daemon too.
So what if we:
a) introduce libvirt-libs.rpm where all the libraries would go) b) have libvirt-daemon depend on -libs instead of -client, c) have libvirt-client install just virsh.
This way we can enable users who really want to have just the daemon installed on their system (e.g. because there's one centralized mgmt point having the client libs/binaries).
Actually I did want to have a libvirt-libs RPM way back when we first added libvirt-client, but was out-voted. I'd be fine with seeing us introduce a libvirt-libs minimal package. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Fri, Jun 24, 2016 at 04:11:52PM +0100, Daniel P. Berrange wrote:
On Fri, Jun 24, 2016 at 05:04:30PM +0200, Michal Privoznik wrote:
On 24.06.2016 16:06, Daniel P. Berrange wrote:
On Fri, Jun 24, 2016 at 03:59:38PM +0200, Michal Privoznik wrote:
On 24.06.2016 15:33, Daniel P. Berrange wrote:
On Fri, Jun 24, 2016 at 03:12:23PM +0200, Michal Privoznik wrote:
Currently, the daemon requires libvirt-admin.so because the functions encoding/decoding RPC messages for admin APIs live there. But this makes it very hard to split admin API into its own separate package: if libvirt-admin.so is going to live in a separate package than the daemon, either both packages must be installed or none. Solve this by statically linking the RPC message handling functions with the daemon.
I'm not sure I see any need for a separate package for libvirt-admin.so For libvirt-qemu.so and libvirt-lxc.so we keep them in libvirt-client RPM, and I'd expect libvirt-admin.so to be there too really.
So libvirt-client would contain not only virsh (and other .so files) but virt-admin binary too? Okay, if that's what we want my patch is useless. If we, however, want a separate package for libvirt-admin (which is kind of special compared to libvirt-qemu.so and libvirt-lxc.so), then I guess we need this patch.
Hmm, i guess libvirt-admin is only needed if libvirtd is actually present on the host. So I guess we could argue that virt-admin and libvirt-admin.so should just be a part of libvirt-daemon RPM.
The more I think about it the more I think that our current split into RPM packages has some minor flaws. For instance, libvirt-daemon requires libvirt-client; just because libvirt-client has some libraries that are required by the daemon too.
So what if we:
a) introduce libvirt-libs.rpm where all the libraries would go) b) have libvirt-daemon depend on -libs instead of -client, c) have libvirt-client install just virsh.
This way we can enable users who really want to have just the daemon installed on their system (e.g. because there's one centralized mgmt point having the client libs/binaries).
Actually I did want to have a libvirt-libs RPM way back when we first added libvirt-client, but was out-voted. I'd be fine with seeing us introduce a libvirt-libs minimal package.
I'm for introducing libvirt-libs too. I'm surprised that you've been out-voted since it's a good practice to place shared libraries or other files into separate package. Pavel
On Fri, 2016-06-24 at 17:04 +0200, Michal Privoznik wrote:
On 24.06.2016 16:06, Daniel P. Berrange wrote:
On Fri, Jun 24, 2016 at 03:59:38PM +0200, Michal Privoznik wrote:
On 24.06.2016 15:33, Daniel P. Berrange wrote:
On Fri, Jun 24, 2016 at 03:12:23PM +0200, Michal Privoznik wrote:
Currently, the daemon requires libvirt-admin.so because the functions encoding/decoding RPC messages for admin APIs live there. But this makes it very hard to split admin API into its own separate package: if libvirt-admin.so is going to live in a separate package than the daemon, either both packages must be installed or none. Solve this by statically linking the RPC message handling functions with the daemon.
I'm not sure I see any need for a separate package for libvirt-admin.so For libvirt-qemu.so and libvirt-lxc.so we keep them in libvirt-client RPM, and I'd expect libvirt-admin.so to be there too really.
So libvirt-client would contain not only virsh (and other .so files) but virt-admin binary too? Okay, if that's what we want my patch is useless. If we, however, want a separate package for libvirt-admin (which is kind of special compared to libvirt-qemu.so and libvirt-lxc.so), then I guess we need this patch.
Hmm, i guess libvirt-admin is only needed if libvirtd is actually present on the host. So I guess we could argue that virt-admin and libvirt-admin.so should just be a part of libvirt-daemon RPM.
The more I think about it the more I think that our current split into RPM packages has some minor flaws. For instance, libvirt-daemon requires libvirt-client; just because libvirt-client has some libraries that are required by the daemon too.
So what if we:
a) introduce libvirt-libs.rpm where all the libraries would go) b) have libvirt-daemon depend on -libs instead of -client, c) have libvirt-client install just virsh.
This way we can enable users who really want to have just the daemon installed on their system (e.g. because there's one centralized mgmt point having the client libs/binaries).
Or am I just too spoilt by gentoo? :-)
That's pretty much how it's handled in Debian as well: libvirt0 contains the shared libraries[1], libvirt-clients contains the clients[2], and libvirt-daemon contains the daemons[3]. Of course both libvirt-clients and libvirt-daemon depend on libvirt0. I'd say go for it! [1] https://packages.debian.org/sid/amd64/libvirt0/filelist [2] https://packages.debian.org/sid/amd64/libvirt-clients/filelist [3] https://packages.debian.org/sid/amd64/libvirt-daemon/filelist -- Andrea Bolognani / Red Hat / Virtualization
On Fri, 2016-06-24 at 15:12 +0200, Michal Privoznik wrote:
Currently, the daemon requires libvirt-admin.so because the functions encoding/decoding RPC messages for admin APIs live there. But this makes it very hard to split admin API into its own separate package: if libvirt-admin.so is going to live in a separate package than the daemon, either both packages must be installed or none. Solve this by statically linking the RPC message handling functions with the daemon.
I don't see the problem with having the daemon depend on libvirt-admin.so - it already depends on libvirt.so after all, and for example in Debian that is reflected by having the libvirt-daemon package (which contains libvirtd) depend on libvirt0 (which contains libvirt.so). It would just be a matter of adding a dependency on libvirt-admin0, or whatever the package would end up being called. -- Andrea Bolognani / Red Hat / Virtualization
On 24/06/16 15:12, Michal Privoznik wrote:
Currently, the daemon requires libvirt-admin.so because the functions encoding/decoding RPC messages for admin APIs live there. But this makes it very hard to split admin API into its own separate package: if libvirt-admin.so is going to live in a separate package than the daemon, either both packages must be installed or none. Solve this by statically linking the RPC message handling functions with the daemon.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> ---
I'm a bit torn. I'm tempted to rewrite some parts of admin API so that it follows the driver architecture we have for other areas, e.g. domain drivers. Until then, this patch is needed.
I agree that sooner or later we'll have to tweak the architecture to get around this issue properly.
daemon/Makefile.am | 2 +- src/Makefile.am | 15 +++++++++++++-- 2 files changed, 14 insertions(+), 3 deletions(-)
diff --git a/daemon/Makefile.am b/daemon/Makefile.am index 927d16f..3b6aafe 100644 --- a/daemon/Makefile.am +++ b/daemon/Makefile.am @@ -152,7 +152,7 @@ libvirtd_admin_la_LDFLAGS = \ $(NO_INDIRECT_LDFLAGS) \ $(NULL) libvirtd_admin_la_LIBADD = \ - ../src/libvirt-admin.la + ../src/libvirt-admin-rpc.la
man8_MANS = libvirtd.8
diff --git a/src/Makefile.am b/src/Makefile.am index 9f8b638..7c3cef6 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -592,7 +592,7 @@ $(srcdir)/lock_protocol-struct: \ $(srcdir)/%-struct: locking/lockd_la-%.lo $(PDWTAGS) $(srcdir)/admin_protocol-struct: \ - $(srcdir)/%-struct: admin/libvirt_admin_la-%.lo + $(srcdir)/%-struct: admin/%.lo $(PDWTAGS)
else !WITH_REMOTE @@ -2165,10 +2165,20 @@ libvirt_admin.syms: libvirt_admin_public.syms $(ADMIN_SYM_FILES) \ # need to include it in the dist EXTRA_DIST += admin/admin_remote.c
+noinst_LTLIBRARIES += \ + libvirt-admin-rpc.la + +libvirt_admin_rpc_la_SOURCES = \ + $(ADMIN_PROTOCOL_GENERATED) + +libvirt_admin_rpc_la_LDFLAGS = \ + $(AM_LDFLAGS) \ + $(CYGWIN_EXTRA_LDFLAGS) \ + $(MINGW_EXTRA_LDFLAGS) + lib_LTLIBRARIES += libvirt-admin.la libvirt_admin_la_SOURCES = \ libvirt-admin.c \ - $(ADMIN_PROTOCOL_GENERATED) \ $(DATATYPES_SOURCES)
libvirt_admin_la_LDFLAGS = \ @@ -2180,6 +2190,7 @@ libvirt_admin_la_LDFLAGS = \
libvirt_admin_la_LIBADD = \ libvirt.la \ + libvirt-admin-rpc.la \ $(CYGWIN_EXTRA_LIBADD)
libvirt_admin_la_CFLAGS = \
I rebased my patches onto yours, built the rpms, made a clean install, tested, and it worked. Then I removed the admin package to test for the dependency issues you described, and the daemon seems to run unchanged (compared to the failure caused by a missing library prior to applying your patch). ACK from me. Thanks, Erik
On 24/06/16 16:00, Erik Skultety wrote:
On 24/06/16 15:12, Michal Privoznik wrote:
Currently, the daemon requires libvirt-admin.so because the functions encoding/decoding RPC messages for admin APIs live there. But this makes it very hard to split admin API into its own separate package: if libvirt-admin.so is going to live in a separate package than the daemon, either both packages must be installed or none. Solve this by statically linking the RPC message handling functions with the daemon.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> ---
I'm a bit torn. I'm tempted to rewrite some parts of admin API so that it follows the driver architecture we have for other areas, e.g. domain drivers. Until then, this patch is needed.
I agree that sooner or later we'll have to tweak the architecture to get around this issue properly.
daemon/Makefile.am | 2 +- src/Makefile.am | 15 +++++++++++++-- 2 files changed, 14 insertions(+), 3 deletions(-)
diff --git a/daemon/Makefile.am b/daemon/Makefile.am index 927d16f..3b6aafe 100644 --- a/daemon/Makefile.am +++ b/daemon/Makefile.am @@ -152,7 +152,7 @@ libvirtd_admin_la_LDFLAGS = \ $(NO_INDIRECT_LDFLAGS) \ $(NULL) libvirtd_admin_la_LIBADD = \ - ../src/libvirt-admin.la + ../src/libvirt-admin-rpc.la
man8_MANS = libvirtd.8
diff --git a/src/Makefile.am b/src/Makefile.am index 9f8b638..7c3cef6 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -592,7 +592,7 @@ $(srcdir)/lock_protocol-struct: \ $(srcdir)/%-struct: locking/lockd_la-%.lo $(PDWTAGS) $(srcdir)/admin_protocol-struct: \ - $(srcdir)/%-struct: admin/libvirt_admin_la-%.lo + $(srcdir)/%-struct: admin/%.lo $(PDWTAGS)
else !WITH_REMOTE @@ -2165,10 +2165,20 @@ libvirt_admin.syms: libvirt_admin_public.syms $(ADMIN_SYM_FILES) \ # need to include it in the dist EXTRA_DIST += admin/admin_remote.c
+noinst_LTLIBRARIES += \ + libvirt-admin-rpc.la + +libvirt_admin_rpc_la_SOURCES = \ + $(ADMIN_PROTOCOL_GENERATED) + +libvirt_admin_rpc_la_LDFLAGS = \ + $(AM_LDFLAGS) \ + $(CYGWIN_EXTRA_LDFLAGS) \ + $(MINGW_EXTRA_LDFLAGS) + lib_LTLIBRARIES += libvirt-admin.la libvirt_admin_la_SOURCES = \ libvirt-admin.c \ - $(ADMIN_PROTOCOL_GENERATED) \ $(DATATYPES_SOURCES)
libvirt_admin_la_LDFLAGS = \ @@ -2180,6 +2190,7 @@ libvirt_admin_la_LDFLAGS = \
libvirt_admin_la_LIBADD = \ libvirt.la \ + libvirt-admin-rpc.la \ $(CYGWIN_EXTRA_LIBADD)
libvirt_admin_la_CFLAGS = \
I rebased my patches onto yours, built the rpms, made a clean install, tested, and it worked. Then I removed the admin package to test for the dependency issues you described, and the daemon seems to run unchanged (compared to the failure caused by a missing library prior to applying your patch). ACK from me.
Thanks, Erik
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Disregard my previous reply for now, at least until we agree on the most viable way to approach this. Erik
participants (5)
-
Andrea Bolognani -
Daniel P. Berrange -
Erik Skultety -
Michal Privoznik -
Pavel Hrdina