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

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



Mark McLoughlin wrote:
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.

I think there are a lot of reasons to not like SunRPC but the fact remains that it's a lot simpler to implement and has a much smaller overhead than XML-RPC and I definitely wouldn't want to implement a home-grown protocol. The reasons not to like SunRPC are that it's completely thread unsafe and that it is (or tries to be) stateless. It's very flexible however: see: http://et.redhat.com/~rjones/secure_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)

I think Dan addressed this. I don't even want to support TCP at all. It's only there for testing/debugging. The default will be only TLS for remote connections.

- There doesn't seem to be any authentication with TLS support. That's definitely necessary, even if it's just cert based.

It should be doing authentication. At the moment the code doesn't do it, but that's a bug (it prints out a big warning message instead). I'm not sure what you mean by "just cert based" though so maybe I didn't understand this point ...

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

I really think that ssh support is going to be important for casual sysadmins. ssh is much much more common than certificate infrastructures. Tunnels are possible, but not well understood by sysadmins either.

+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.

This function is essentially completely rewritten now, so ... And it uses the libxml URI parser (not quite working at the moment, but that's what I'm debugging ...)

Rich.

--
Emerging Technologies, Red Hat  http://et.redhat.com/~rjones/
64 Baker Street, London, W1U 7DF     Mobile: +44 7866 314 421
 "[Negative numbers] darken the very whole doctrines of the equations
 and make dark of the things which are in their nature excessively
 obvious and simple" (Francis Maseres FRS, mathematician, 1759)


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