[Libvir] [PATCH] Remote 3/8: Client-side

Richard W.M. Jones rjones at redhat.com
Tue May 8 12:29:17 UTC 2007

Daniel P. Berrange wrote:
> On Sat, May 05, 2007 at 12:17:44PM +0100, Richard W.M. Jones wrote:
>> Richard W.M. Jones wrote:
>>> 3 Client-side
>>> -------------
>>> A src/remote_internal.c
>>> A src/remote_internal.h
>>> M src/driver.h
>>> M src/libvirt.c
> What sort of info is currently stored in the  $sysconfdir/libvirtd.conf
> file ? It seems to be referred to by both the client & server.

Hopefully only the server is reading this file.

It contains things like:
* What ports the server should listen on?
* Should the server verify TLS certs?
* ACL (list of IP addresses) of clients
* Location of various PKI files

All of these things have sensible defaults, so you can run the server 
without the file at all.

If you look at qemud/qemud.c:remoteReadConfigFile you'll find the code 
which reads this file.  Yes, this needs to be documented (see step 7 of 8).

> On the subject of 
>>  /* GnuTLS functions used by remoteOpen. */
>>  #define CAFILE "demoCA/cacert.pem" /* XXX */
>>  #define KEY_FILE "127001key.pem" /* XXX */
>>  #define CERT_FILE "127001cert.pem" /* XXX */
> We've got to think about best way to provide TLS parameters. Bearing in mind
> that people may want to port to Mozilla NSS in the future I think it is 
> probably best to not provide formal APIs for these options at this time, and
> instead pick them up from the client's config file. As previously discussed
> there's no clear standard for location of TLS certs, so I reckon we just 
> default to something like
>    tls_ca_cert = "/etc/pks/tls/ca-cert.pem"
>    tls_secret = "/etc/pks/tls/libvirt-key.pem"
>    tls_cert = "/etc/pks/tls/libvirt-cert.pem"

Yes, agreed.  That is a to-do action, as well as documenting how to 
create these files.  One thing I need to do is go back over my notes and 
work out how _I_ created the files ...

> Since we're using client certs for authentication to start with, we ought
> to actually make it possible to keep the certs in per-user home dirs rather
> than globally readable. eg $HOME/.pki/tls/libvirt-key.pem  and mode 0600

Yes, I'll make a note to talk to the Fedora people about this.

>> The driver itself (remote_internal.c) consists of the following parts, 
>> arranged here in the same order that they appear in the file:
>> (1) remoteOpen and associated, GnuTLS initialisation
>> This function and associated functions deals with parsing the remote 
>> URI, deciding which transport to use, deciding the URI to forward to the 
>> remote end, creating the transport (including initialising GnuTLS, 
>> forking ssh, ...), and verifying the server's certificate.
> Wrt to this snippet in negotiate_gnutls_on_connection()
>>    const int cert_type_priority[3] = {
>>        GNUTLS_CRT_X509,
>>        0
>>    };
> Do we have support in the client/server for doing whatever checks are needed
> for the OpenPGP style 'certs'. If not we should probably comment out the
> OpenGPG option for now.

Actually looking at the code it's worse than that - at the moment it 
accepts the certificate and then rejects it later on because it's not an 
X.509 cert.  --> To fix.

>> (2) The driver functions, remoteClose, remoteType, etc. etc.
>> (3) The network driver functions, remoteNetworkOpen, etc. etc.
>> For each function we serialise the parameters as an XDR remote_foo_args 
>> object, use the generic "call" function to dispatch the call, then 
>> deserialise the XDR remote_foo_ret return value.  Most of the heavy 
>> lifting for the RPC is done in the "call" function itself ...
> This all loooks good to me. Only thought I had was perhaps that
>       GET_PRIVATE (conn, -1);
> is slightly more readable as
>       struct private_data *priv = GET_PRIVATE (conn, -1);
> ie, to make it clear to the readable what local variable is being
> provided, while still hiding away all the tedious error checking /
> assertion logic.

The problem is that GET_PRIVATE has side effects, in particular it can 
return with an error if the connection object or private pointer is 
corrupt.  Would this require a gcc extension to implement?  I'll check.

BTW, although this business of checking if connections are NULL and 
checking magics and then returning is done a lot in libvirt, I don't 
agree with it _at all_.  I think that the library should abort / assert 
fail in such cases, because the earlier that memory corruption is 
detected the better.  (Better still would be to use a language where 
this can't happen).

>> (4) The RPC implementation (function "call"), error handling, 
>> serialisation of virDomainPtr and virNetworkPtr.
> In the 'call' method:
>>    /* Get a unique serial number for this message. */
>>    /* XXX This is supposed to be atomic over threads, but I don't know
>>     * if it is.
>>     */
>>    static sig_atomic_t counter = 1;
>>    sig_atomic_t serial = counter++;
> Serial numbers only need to be unique within the scope of a single
> client <-> server socket connection. So could we just store the
> counter in the virConectPtr private data struct to avoid the threading
> question here

Yes, good thinking.  I'll fix it.

>> ... which also deals with making error handling transparent.
>> static void
>> error (virConnectPtr conn, virErrorNumber code, const char *info)
>> {
>>     const char *errmsg;
>>     /* XXX I don't think this is correct use of virErrorMsg. */
>>    errmsg = __virErrorMsg (code, info);
> This looks consistent with the way other drivers are using __virErrorMsg
> at least. Anything in particular you thought was wrong ?

I looked at what precisely happened inside virterror on Friday and came 
to the conclusion that this was correct, so I'll remove the comment.


More information about the libvir-list mailing list