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)