Re: [libvirt] [PATCH 09/15] Add libvirt-admin library
On Fri, Apr 17, 2015 at 11:28:11AM +0100, Daniel P. Berrange wrote:
On Thu, Apr 16, 2015 at 04:46:44PM +0200, Martin Kletzander wrote:
> Initial scratch of the admin library. It has its own virAdmConnectPtr
> that inherits from virAbstractConnectPtr and thus trivially supports
> error reporting.
See my note earlier about error reporting on the connection being
a bad idea due to lack of thread safety.
> Configuration option --with-admin is added to control whether the admin
> library should be built or not (set to 'yes' by default).
Is there a compelling reason why we'd need/want to be able to
disable building of the admin library ? As a comparison we
unconditionally build libvirt-qemu.so & libvirt-lxc.so
No reason, I just figured someone might want to disable that.
[...]
> +virAdmConnectPtr virAdmConnectOpen(unsigned int flags);
How does this figure out which libvirtd daemon to connect to ? Presumably
you've hardcoded it based on the UID you're running as ?
It doesn't. I don't think anyone is going to use this to connect to
the session daemon. I also don't see anyone having session daemon
running without the possibility of restarting it.
I think for future proofing we should probably define a URI syntax
for this.
eg
admin:///system
admin:///session
And allow an optional parameter for the socket path, for people who
have built their daemon with an unusual --prefix arg.
I think we can keep it without the possibility of connecting to the
session daemon and if that is going to change, we can add
virAdmConnectSession() function. In the meantime I'd rather focus on
enabling this functionality then dealing with session daemon, all the
unnecessary code it adds and the problems we already have with the
session daemon.
[...]
> diff --git a/src/libvirt_admin.syms b/src/libvirt_admin.syms
> new file mode 100644
> index 0000000..292433f
> --- /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.2.3 {
Either you've been working on this a really long time, or this
version was a typo.
That version is one year old and I've sent the original RFC 2 years
ago. The progress can be seen thanks to that. I started a long time
ago, then haven't had a time for that and every time I managed to get
rid of higher priority tasks I had to rebase it and start off again
and only for so much time until another higher priority tasks
appeared.
Either way, when we are ready to merge the admin library, that
might be a nice reason to bump our version to 1.3.x, since
we've been on 1.2.x a long time.
That'd be great! Or 2.0? Just kidding.
Attachments:
- signature.asc (application/pgp-signature — 819 bytes)