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

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



On Fri, May 22, 2015 at 12:59:44PM +0100, Daniel P. Berrange wrote:
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.


OK, I though that shouldn't be the case, so it is installable
separably, but since we allow only local socket for now, you're right
that there's libvirt.so already loaded.

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

Attachment: signature.asc
Description: PGP signature


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