Hey,
On Tue, 2007-01-30 at 17:42 +0000, Richard W.M. Jones wrote:
> This patch is just for discussion. It's not in a state to be applied,
> even if it were accepted (which is a long-shot at present anyway).
Cool stuff on getting this far.
Okay, some comments to start with:
- From past dealings with SunRPC and CORBA and the like, the choice
of SunRPC makes me pretty nervous. The amount of code we'd be
adding to libvirt to support SunRPC over the different transports
makes me even more nervous. I wouldn't fancy having to debug
problems there, or add another transport, for example.
I'm pretty jaded of RPC systems in general, though. So, I'd suggest
either a homegrown binary protocol like we're using now or, if it
turns out to be fairly simple, XML RPC.
- Wrt. the TLS support - as recommended by the RFC AFAIR, new
protocols are supposed to negotiate TLS at connection time rather
than having a separate port for TLS (as Dan does with qemud)
- There doesn't seem to be any authentication with TLS support.
That's definitely necessary, even if it's just cert based.
- If libvirtd is going to be a pure proxy, I don't think the UNIX
transport is going useful.
- Also, if it's just a proxy, couldn't this be launched by xinetd?
- I'd advocate merging qemud into libvirtd, but you could have a long
discussion either way on that one.
- The ssh and ext transports just seem like hacks, especially if the
ssh transport won't support authentication (both ways). If people
want to use SSH, they can just set up their own tunnel.
- Wrt. coding style - libvirt has a *fairly* consistent coding style
throughout and I think that helps with the maintainability of any
project. Deviations are already starting to creep in (e.g. not
using studlyCaps in places) ... but we should try and stem that
tide IMHO (even though the current coding style wouldn't be my
favourite)
> * SunRPC is stateless so we need to hand out a cookie to
> represent the virConnectPtr handle on the server side.
> However if the client dies without explicitly calling
> close, we have no way to know, and so the cookie/handle
> on the server side lives forever.
The server side context should be the file descriptor - i.e. there
should be only a single open() call per fd, and so only a single
virConnectPtr per fd ... so if socket is closed by the remote side, we
know we can call close() on the virConnectPtr. There shouldn't be a need
for cookies, IMHO.
(Perhaps this "single open() call per connection" implies that there
shouldn't be an open() RPC, but rather that happens as part of the
initial negotiation stage)
> + /* driver private data
> + * (Ought to replace the above ad-hoc Xen data, IMHO anyway.
> + * Currently only the 'remote' driver uses this.
> + * - RWMJ).
> + */
> + void *private;
> +
Yep, that'd be a nice cleanup - e.g. I have this patch to make qemud
and xen not share the "handle" member:
http://www.gnome.org/~markmc/code/libvirt-networking/libvirt-qemu-dont-share-cnx-handle-with-xen.patch
> +// See: http://people.redhat.com/drepper/userapi-ipv6.html
> +static int
> +make_sockets (int *fds, int max_fds, int *nfds_r, const char *service)
> +{
> + struct addrinfo *ai;
> + struct addrinfo hints;
> + memset (&hints, 0, sizeof hints);
> + hints.ai_flags = AI_PASSIVE | AI_ADDRCONFIG;
> + hints.ai_socktype = SOCK_STREAM;
> +
> + int e = getaddrinfo (NULL, service, &hints, &ai);
> + if (e != 0) {
> + fprintf (stderr, "getaddrinfo: %s\n", gai_strerror (e));
> + return -1;
> + }
> +
> + struct addrinfo *runp = ai;
> + while (runp && *nfds_r < max_fds) {
> + fds[*nfds_r] = socket (runp->ai_family, runp->ai_socktype,
> + runp->ai_protocol);
> + if (fds[*nfds_r] == -1) {
> + perror ("socket");
> + return -1;
> + }
> +
> + int opt = 1;
> + setsockopt (fds[*nfds_r], SOL_SOCKET, SO_REUSEADDR, &opt, sizeof opt);
> +
> + if (bind (fds[*nfds_r], runp->ai_addr, runp->ai_addrlen) == -1) {
> + if (errno != EADDRINUSE) {
> + perror ("bind");
> + return -1;
> + }
> + close (fds[*nfds_r]);
> + }
> + else {
> + if (listen (fds[*nfds_r], SOMAXCONN) == -1) {
> + perror ("listen");
> + return -1;
> + }
> + ++*nfds_r;
> + }
> + runp = runp->ai_next;
> + }
> +
> + freeaddrinfo (ai);
I'm really not a big fan of this way of doing IPv6 support because:
- getaddrinfo() is a pretty complicated function, so the code above
is pretty obtuse without a thorough read of the getaddrinfo() man
page
- you really want to end up with *either* a single IPv6 socket or and
IPv4 socket ... the above code can end up with multiple sockets.
Here's what I prefer to do:
---
struct sockaddr_in addr_in;
struct sockaddr *addr;
socklen_t addrlen;
#ifdef ENABLE_IPV6
struct sockaddr_in6 addr_in6;
memset(&addr_in6, 0, sizeof(addr_in6));
addr_in6.sin6_family = AF_INET6;
addr_in6.sin6_port = htons(port);
addr_in6.sin6_addr = localOnly ? in6addr_loopback : in6addr_any;
addr = (struct sockaddr *)&addr_in6;
addrlen = sizeof(addr_in6);
sock = socket(AF_INET6, SOCK_STREAM, 0);
#endif
if (sock < 0) {
if ((sock = socket(AF_INET, SOCK_STREAM, 0)) < 0)
return -1;
memset(&addr_in, 0, sizeof(addr_in));
addr_in.sin_family = AF_INET;
addr_in.sin_port = htons(port);
addr_in.sin_addr.s_addr = localOnly ? htonl(INADDR_LOOPBACK) : htonl(INADDR_ANY);
addr = (struct sockaddr *)&addr_in;
addrlen = sizeof(addr_in);
}
if (setsockopt(sock, SOL_SOCKET, SO_REUSEADDR,
(char *)&one, sizeof(one)) < 0) {
close(sock);
return -1;
}
if (bind(sock, addr, addrlen) < 0) {
close(sock);
return -1;
}
----
> +static int
> +remote_open (virConnectPtr conn, const char *name,
> + int flags __attribute__((unused)))
This function is huge, and extremely hard to verify. Recommend
splitting it up a lot. Perhaps when you use libxml's URI parsing etc. it
won't be so bad, though.
Also, libvirt has ATTRIBUTE_UNUSED.
> + switch (proto) {
> + case proto_unknown:
> + abort (); /* Internal error in this function. */
> +
> + case proto_tls:
> + case proto_tcp: {
....
> + proto == proto_tls
> + ? clntgnutls_create (r->ai_family, r->ai_addr, r->ai_addrlen, xcred,
> + LIBVIRTREMOTE, LIBVIRTREMOTE_VERS1,
> + &private.sock, 0, 0)
> + : clnttcp2_create (r->ai_family, r->ai_addr, r->ai_addrlen,
> + LIBVIRTREMOTE, LIBVIRTREMOTE_VERS1,
> + &private.sock, 0, 0);
> + if (private.cl)
> + goto tcp_connected;
> + }
> +
> + freeaddrinfo (res);
> + error (conn, VIR_ERR_RPC,
> + clnt_spcreateerror ("could not create SunRPC client"));
> + goto failed;
> +
> + tcp_connected:
> + freeaddrinfo (res);
A goto in a switch() statement? ... that's a whole litter of cute
little puppies you've killed, right there! :-)
HTH,
Mark.