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.