Re: [Libvir] Preliminary patch to support remote driver / libvirtd

Hey, On Tue, 2007-01-30 at 17:42 +0000, Richard W.M. Jones wrote: 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) 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) 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-sh... 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; } ---- 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. .... A goto in a switch() statement? ... that's a whole litter of cute little puppies you've killed, right there! :-) HTH, Mark.