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