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

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



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,
       GNUTLS_CRT_OPENPGP,
       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.

Rich.


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