[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



On Thu, May 21, 2015 at 10:22:26PM -0700, Martin Kletzander wrote:
> 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.

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

I don't think you are actually making this lightweight as you intend.

In reality anyone who is using libvirt-admin.so will likely already
have libvirt.so in memory. So by recompiling the files you are causing
two copies of the same code to be kept in RAM. So if libvirt-admin.so
just linked to libvirt.so, then we would in fact be more lightweight
than this, and avoid the recompilation.

The recompilation for the setuid binary is a special case because
we have security requirements that trump the extra overhead that
the duplication implies both at compile time and runtime.

So I don't think the recompilation is really justified in the case
of libvirt-admin.so

One day though, I do think we should perhaps turn things like
libvirt_util.la, libvirt_rpc.la, etc into a formally installed
.so's that we can dynamically link to from other libvirt parts.
We'd /not/ include any header files, as these .so's would be
solely for libvirt usage, not application usage. I think this
would ultimately better achieve what you're trying todo here.

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 :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]