[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,
>> 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.
More information about the libvir-list
mailing list