2:06 p.m.
On Wed, Jan 31, 2007 at 07:07:13PM +0000, 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).
- 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.
My home grown protocol was becoming decidely less simple as I made it
more portable. It had a huge flaw in that although it used fixed size
types, it still assumes that struct padding was the same across all
hosts - not the case on 64-bit uint64_t is aligned on 8-bytes boundaris
while 32-bit aligns it on 4-byte boundaries. The XDR encoding that is
part of SunRPC guarentees this kind of issue is dealt with reliably
- 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)
We could probably add a RPC call to the SunRPC protocol to enable
switching to the TLS mode on the fly. In many ways, however, I
regretted adapting my original code to do up-negotiation to TLS.
It made it seriously more complicated and I'm still not entirely
sure I got it correct. So if anything I'd be infavour of refusing
to support plain TCP at all - except that it could be valid to
use plain TCP if your tunnelling over SSH.
- There doesn't seem to be any authentication with TLS support.
That's definitely necessary, even if it's just cert based.
Yes, I say stick to just client cert auth to start with.
- If libvirtd is going to be a pure proxy, I don't think the
UNIX
transport is going useful.
It will be useful for the local Xen case - letting us have a full
read-write connection to Xen even when unprivileged - so we would no
longre have to run virt-manager as root.
- Also, if it's just a proxy, couldn't this be launched by
xinetd?
That sounds like a reasonable option to allow - at the same time its
nice to have a persistently running daemon to avoid having to re-init
all the one-time TLS stuff for every connection because that can take
a non-negligable time & deplete /dev/random unneccessarily
- I'd advocate merging qemud into libvirtd, but you could have
a long
discussion either way on that one.
Yes, I definitely want to do that - makes no sense to have 2 daemons if
one will do. I'm waiting till libvirtd is further developed before
seriously attacking it.
- 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)
For indentation we can append these following to source files to do the
right right thing automagically. I've been adding this to source files
as I do patches & fixing up indentation.
/*
* vim: set tabstop=4:
* vim: set shiftwidth=4:
* vim: set expandtab:
*/
/*
* Local variables:
* indent-tabs-mode: nil
* c-indent-level: 4
* c-basic-offset: 4
* tab-width: 4
* End:
*/
> +// 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
It may be complicated - but this is only because doing the correct thing
for IPv6 is complicated - the traditional getaddr/host related functions
all have a number of flaws which make them unsuitable for modern day
usage.
- 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.
It can end up with multiple sockets because if a machine has multiple NICs,
it is perfectly possible for a single address to be associated with multiple
NICs. Thus to bind to all interfaces associated with an address, it may be
neccessary to have multiple sockets.
Here's what I prefer to do:
[snip]
IMHO, this code isn't simpler /shorter by any significant amount, and it is
far less capable than the code already written above to deal with getaddrinfo.
The semantics of the getaddrinfo code are also well documented by Uli if any
reference is needed http://people.redhat.com/drepper/userapi-ipv6.html
Regards,
Dan.
--
|=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=|
|=- Perl modules: http://search.cpan.org/~danberr/ -=|
|=- Projects: http://freshmeat.net/~danielpb/ -=|
|=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|