Hey, Btw, I'm really becoming quite convinced we'll evenutally regret using SunRPC if we stick with it. On Wed, 2007-02-28 at 21:03 +0000, Richard W.M. Jones wrote

+AC_PATH_PROG(LOGGER, logger)

+AC_DEFINE_UNQUOTED(LOGGER, "$LOGGER", + [Define the location of the external 'logger' program, or + undefine to disable use of external 'logger'. This is + used by libvirtd to write to syslog.])

You may have explained this before, but why not use syslog(3)?

+dnl Availability of various common functions. +AC_CHECK_FUNCS([lrand48_r])

I've little interest in random number generation, so it might just be me, but do we really need this?

dnl Make sure we have an ANSI compiler AM_C_PROTOTYPES @@ -73,6 +81,8 @@

dnl dnl make CFLAGS very pedantic at least during the devel phase for everybody +dnl (Overriding $CFLAGS here is very wrong. See discussion of AM_CFLAGS +dnl in the automake info. - RWMJ) dnl if test "${GCC}" = "yes" ; then CFLAGS="-g -O -W -Wformat -Wunused -Wimplicit -Wreturn-type -Wswitch -Wcomment -Wtrigraphs -Wformat -Wchar-subscripts -Wuninitialized -Wparentheses -Wshadow -Wpointer-arith -Wcast-align -Wwrite-strings -Waggregate-return -Wstrict-prototypes -Wmissing-prototypes -Wnested-externs -Winline -Wredundant-decls -Wall"

I'll fix this ... patch incoming soon.

@@ -233,6 +243,14 @@ AC_SUBST(LIBXML_CONFIG) AC_SUBST(LIBXML_MIN_VERSION)

+dnl GnuTLS library +AC_CHECK_LIB(gnutls, gnutls_handshake, + [], + [AC_MSG_ERROR([gnutls library not found])])

You should check for the headers too with AC_CHECK_HEADER() - I don't think AC_CHECK_LIB() will pass without the -devel package installed, but just in case ...

+dnl /dev/urandom +AC_CHECK_FILES([/dev/urandom])

You don't seem to use this?

diff -urN --exclude=CVS --exclude=.git --exclude='*.pem' --exclude=demoCA libvirt-cvs/.gitignore libvirt-remote/.gitignore --- libvirt-cvs/.gitignore 1970-01-01 01:00:00.000000000 +0100 +++ libvirt-remote/.gitignore 2007-02-26 14:43:51.000000000 +0000

Add this to your --exclude

-virConfPtr virConfNew(void) +virConfPtr +_virConfNew(void)

Is git not making it easy enough for you to manage these patches individually? Why can't you get the remote patch from git without the other patches included? (Honest question - I briefly tried to do this with git in the past and failed. That's why I use quilt.)

* * Returns NULL in case of error, a static zero terminated string otherwise. */ +/* See also: https://www.redhat.com/archives/libvir-list/2007-February/msg00096.html */ const char * virConnectGetType(virConnectPtr conn)

I think we should add virConnectGetName() and mark this one as deprecated.

+++ libvirt-remote/src/libvirtd.c 2007-02-28 14:21:48.000000000 +0000

There's so much nasty stuff needed to get a daemon right, I'd suggest putting this in qemud for the start - e.g. you don't want to sort out some logging infrastructure only to have to make it use qemudLog() if/when we merge the daemons.

+/* XXX Still to decide where these certificates should be located. */ +const char *key_file = "serverkey.pem"; +const char *cert_file = "servercert.pem"; +const char *ca_file = "demoCA/cacert.pem"; +const char *crl_file = ""; + +/* This is autogenerated, in remote_rpc_svc.c */ +extern void libvirtremote_1 (struct svc_req *rqstp, register SVCXPRT *transp); + +static void copy_error (remote_error *err, virErrorPtr verr); +static void out_of_memory_error (remote_error *err); +static void bad_connection_error (remote_error *err);

Much as I personally dislike it, studlyCaps (for both functions and variables) is libvirt's coding style.

+remote_open_ret * +remote_open_1_svc (char **name, struct svc_req *req)

Weird, why is this passed a char**? Can we be sure it's never NULL?

+{ + static struct remote_open_ret ret; + + if (lookup_connection (req) != 0) {

The rest of the code in libvirt doesn't have a space between the function name and parenthesis.

+ ret.status = -1; + bad_connection_error (&ret.remote_open_ret_u.err); + } else { + /* If the name is "", convert it to NULL. */ + char *real_name = *name; + if (strcmp (real_name, "") == 0) real_name = NULL; + + /* Log connection name. */ + fprintf (stderr, "libvirtd (%d): open \"%s\"\n", getpid (), real_name);

logging this stuff to stderr isn't much good for us. (Uggh, I see now ... this goes to the logger?)

+remote_close_ret * +remote_close_1_svc (void *vp __attribute__((unused)), + struct svc_req *req) +{ + static struct remote_close_ret ret;

static?

+ virConnectPtr conn = lookup_connection (req); + + if (!conn) {

I'd prefer if (!(conn = lookup_connection(req)) ... I try and avoid doing too much in amongst variable declarations.

+ ret.status = -1; + bad_connection_error (&ret.remote_close_ret_u.err); + } else { + int r = virConnectClose (conn); + + if (r == 0) {

Elsewhere you do it like: if (virConnectClose(conn) == 0) which I'd also prefer. Actually, I'd code these functions like this: { struct remote_type_ret ret; virConnectPtr conn; const char *type; ret.status = -1; if (!(conn = lookupConnection(req))) { badConnectionError(&ret.remote_type_ret_u.err); goto out; } if (!(type = virConnectGetType(conn)) { copyError(&ret.remote_type_ret_u.err, virConnGetLastError (conn)); goto out; } ret.remote_type_ret_u.type = (char *) type; // string is static ret.status = 0; out: return &ret; } You avoid the nesting and the non-error code path is much easier to follow, IMHO.

+remote_type_ret * +remote_type_1_svc (void *vp __attribute__((unused)),

Use ATTRIBUTE_UNUSED

+ +// Just a number large enough to use for validation of the +// externally produced maxids. +#define MAX_DOMAINS 10000

Uggh.

+remote_listDomains_ret * +remote_listdomains_1_svc (int *maxids, + struct svc_req *req) +{ + static struct remote_listDomains_ret ret; + virConnectPtr conn = lookup_connection (req); + + if (!conn) { + ret.status = -1; + bad_connection_error (&ret.remote_listDomains_ret_u.err); + // Sanity-check maxids before allocating the on-stack array. + } else if (*maxids < 0 || *maxids > MAX_DOMAINS) { + ret.status = -1; + out_of_memory_error (&ret.remote_listDomains_ret_u.err); + } else { + int ids[*maxids]; + int len = virConnectListDomains (conn, ids, *maxids); + + if (len >= 0) { + ret.status = 0; + ret.remote_listDomains_ret_u.ids.ids_len = len; + ret.remote_listDomains_ret_u.ids.ids_val = ids;

Um, how does that work? You've allocated an array on the stack, with a C99 idiom I thought we were avoiding, and you're going to continue using that array after returning from the function?

+ virDomainPtr dom = virDomainCreateLinux (conn, + args->xmlDesc, args->flags); + + if (dom) { + ret.status = 0; + // XXX No idea if this is the right thing to do. + ret.remote_domainCreateLinux_ret_u.domain = + malloc (sizeof *ret.remote_domainCreateLinux_ret_u.domain);

Well, you don't check the malloc() succeeded for a start. (I know, you don't agree it's needed ... but it's not something we should stop doing on an ad-hoc basis) But, I think you're wondering how to free() what you've just allocated. That's a very important point, we need to figure that out.

+ ret.remote_domainCreateLinux_ret_u.domain->name = + (char *) dom->name;

I'd use virDomainGetName()

+static void +copy_error (remote_error *err, virErrorPtr verr) +{ + err->code = verr->code; + err->domain = verr->domain; + err->message = verr->message ? : null_string; + err->level = verr->level; + if (verr->dom) { + // XXX I have no idea if this is the right way to do this. + err->dom = malloc (sizeof *err->dom); + err->dom->name = verr->dom->name;

Same issue as above? Where can we free it?

+ +/*----------------------------------------------------------------------*/ +/* Map SunRPC connections to virConnectPtr. */ + +/* SunRPC is "connectionless", but in fact when used over TCP it is + * really connection-oriented. If your TCP connection goes down, + * the client needs to manually reconnect, which the remote client + * never does. So we associate virConnectPtr with an actual TCP + * connection. + * + * The questions are: (1) How and where do we store this association? + * (2) How can we clean up when the connection goes away? + * + * So for (1) we note that each server-side callback gets a pointer + * to struct svc_req, which contains a pointer to the transport + * (SVCXPRT *). It turns out (you need to read the code closely) + * that each transport pointer is unique to the connection, so we + * use that.

Presumably this means that on the client side we don't try and share an RPC connection amongst multiple libvirt connections? req->rq_xprt->xp_sock might do the trick too, no difference I guess.

+ +static struct virconnmap { + struct virconnmap *next; + + /* Key. */ + SVCXPRT *xprt; + + /* conn may be NULL in the case where a mapping exists, but the + * virConnectPtr has either not been created yet, or has been + * properly closed using the remote_close call. + */ + virConnectPtr conn; +} *virconnmap = 0;

(Btw, please use NULL instead of 0) Hmm, I think I'd do this with a realloc()able array rather than a linked list ... let's see how that looks: typedef struct { SVCXPRT *xprt; virConnectPtr conn; } virConnMapping; static virConnMapping *virConnMap = NULL; static int virConnMapLen = 0; static int createMapping(SVCXPRT *xprt, virConnectPtr conn) { virConnMap *newMap; for (i = 0; i++; i < virConnMapLen) if (virConnMap[i].xprt = xprt) { logError("Mapping for xprt %p already exists", %p); return -1; } newMap = (virConnMapping *)realloc(virConnMap, (virConnMapLen + 1) * sizeof(virConnMapping)); if (!newMap) { logError("Out of memory allocating virConnMapping"); return -1; } virConnMap = newMap; virConnMap[virConnMapLen].xprt = xprt; virConnMap[virConnMapLen].conn = conn; virConnMapLen++; return 0; } static int destroyMapping(SVCXPRT *xprt) { virConnMap *newMap; int i; for (i = 0; i++; i < virConnMapLen) if (virConnMap[i].xprt = xprt) break; if (i == virConnMapLen) { logError("Failed to find mapping for xprt %p", xprt); return -1; } memove(virConnMap + i, virConnMap + i + 1, virConnMapLen - i - 1); virConnMapLen--; newMap = (virConnMapping *)realloc(virConnMap, (virConnMapLen) * sizeof(virConnMapping)); if (newMap) virConnMap = newMap; return 0; }

+ + /* Add the mapping. */ + p = malloc (sizeof *p); + if (p == 0) { + perror ("malloc");

Again, better error reporting needed throughout and use NULL.

+static void +destroy_mapping (SVCXPRT *xprt) +{ + /* Log connection closed. */ + fprintf (stderr, "libvirtd (%d): connection closed\n", getpid ()); + + struct virconnmap *p, *lastp; + for (p = virconnmap, lastp = 0; p; lastp = p, p = p->next) + if (p->xprt == xprt) + goto found_it; + + /* Mapping not found - that's an internal error. */ + fprintf (stderr, "libvirtd: internal error: destroy_mapping called but mapping not found\n"); + return; + + found_it:

Nasty way to use a goto IMHO, see what I did above.

+ /* Do we need to clean up the connection? If the client called + * remote_close then conn will be NULL. Otherwise the connection + * has been dropped without a clean close, so we close it here. + */ + if (p->conn) + (void) virConnectClose (p->conn);

Why the void cast, and why not log an error if it fails?

+static void +set_connection (struct svc_req *req, virConnectPtr conn) +{ + SVCXPRT *xprt = req->rq_xprt; + + struct virconnmap *p; + for (p = virconnmap; p; p = p->next) + if (p->xprt == xprt) { + p->conn = conn; + return; + } + + abort (); // Should never happen.

Ouch. I'd be happier if we were using assert() around libvirt for this kind of stuff. Other places we just log an error.

+static void +log_peer (int sock) +{ + /* Log the connection to stderr. It will end up in syslog if + * logger is running. + */ + int pid = getpid (), r; + fprintf (stderr, "libvirtd (%d): new connection\n", pid);

pid should be unsigned long or pid_t and format should be %lu, AFAIR.

+ struct sockaddr_storage addr; + socklen_t addrlen = sizeof addr;

Try to avoid this intermingling of code and variable declarations, also using parenthesis with sizeof is normal.

+ // Read the configuration file. If it's not there, proceed with defaults. + virConfPtr conf = virConfReadFile (conffile);

Split all this config file reading out into a separate function.

+ if (conf) { + virConfValuePtr p; + +#define CHECK_TYPE(name,typ) if (p && p->type != (typ)) { \ + fprintf (stderr, "libvirtd: %s: %s: expected type " #typ "\n", \ + conffile, (name)); \ + exit (1); \ + }

Why not just log and error and continue? Also, perhaps worth doing: static inline libvirtdConfGetLong(virConfPtr conf, const char *setting, long default) { virConfValuePtr p; if (!(p = virConfGetValue(conf, setting))) return default; if (p->type != VIR_CONF_LONG) { logError("Expected a long for libvirtd.conf setting '%s'", setting); return default; } return p->l; }

+ p = virConfGetValue (conf, "tls_port"); + CHECK_TYPE ("tls_port", VIR_CONF_STRING); + tls_port = p ? my_strdup (p->str) : tls_port;

You're presumably going to free these strings at some point, so you need to initialize them with strdup()ed strings before parsing so that you can safely free them always. Also, you should free the previous value before assigning.

+ p = virConfGetValue (conf, "tls_allowed_clients"); + if (p) + { + switch (p->type) + {

Sudden change of coding style.

+ case VIR_CONF_STRING: + tls_allowed_clients = my_malloc (2 * sizeof (char *)); + tls_allowed_clients[0] = my_strdup (p->str); + tls_allowed_clients[1] = 0; + break;

If this is specified twice in the config file, you leak. Free any previous value.

+ // This may not be an error. cf. /etc/exports + if (!listen_tls && !listen_tcp && !listen_unix) {

Hmm, what does this comment mean? (Also, please don't use c++ comments)

+ err = gnutls_certificate_allocate_credentials (&x509_cred); + if (err) { gnutls_perror (err); exit (1); }

Don't squash things together like that. And I really don't like this exit(1) business - it's just lazy. If we add a PID file, for example, all these will need to be fixed so that we exit gracefully and clean up everything.

+ generate_dh_params (); + gnutls_certificate_set_dh_params (x509_cred, dh_params);

See comment on genererate_dh_params() below ... you wonder where dh_params magically appears from here ...

+ + int fds[2]; + int nfds = 0; + if (make_sockets (fds, 2, &nfds, tls_port) == -1) + exit (1);

Wow, this function is getting long.

+ if (listen_unix) { + /* XXX Not sure if this is the right thing to do. */ + if (unlink (unix_socket) == -1 && errno != ENOENT) { + perror (unix_socket); + exit (1); + }

Are we actually creating this socket in the filesystem instead of the abstract namespace?

+#ifdef LOGGER + /* Send stderr to syslog using logger. It's a lot simpler + * to do this. Note that SunRPC in glibc prints lots of + * gumf to stderr and it'd be a load of work to change that. + */

Now I understand. How perfectly hideous. I'd prefer to use syslog(3) in our code, so that if we do fix the RPC stuff at some stage we can drop the logger process.

+ int fd[2]; + if (pipe (fd) == -1) { + perror ("pipe"); + exit (2); + } + int pid = fork (); + if (pid == -1) { + perror ("fork"); + exit (2); + } + if (pid == 0) { /* Child - logger. */ + const char *args[] = { + "logger [libvirtd]", + "-t", "libvirtd", + "-p", "daemon.notice", + NULL + }; + close (fd[1]); + dup2 (fd[0], 0); + close (fd[0]); + execv (LOGGER, (char *const *) args); + perror ("execv"); + _exit (1); + } + close (fd[0]); + dup2 (fd[1], 2); + close (fd[1]);

If the exec fails, won't we die with a SIGPIPE the first time we write to the pipe?

+static void +generate_dh_params (void) +{ + int err; + + /* Generate Diffie Hellman parameters - for use with DHE + * kx algorithms. These should be discarded and regenerated + * once a day, once a week or once a month. Depending on the + * security requirements. + */ + err = gnutls_dh_params_init (&dh_params); + if (err) { gnutls_perror (err); exit (1); } + err = gnutls_dh_params_generate2 (dh_params, DH_BITS); + if (err) { gnutls_perror (err); exit (1); } +}

Return the dh_params and let the caller assign it to the global variable. Not sure why you split this code out into a separate function and not the rest of it?

+/* Check the IP address matches one on the list of wildcards + * tls_allowed_clients. + */ +static int +check_allowed_client (void *vp __attribute__((unused)), const char *addr) +{ + const char **wildcards = tls_allowed_clients;

I'd pass the tls_allowed_clients pointer to svc_create() rather than using the global variable here.

+/* Private malloc and strdup functions which always succeed. For + * justification, see http://et.redhat.com/~rjones/ These are only + * for use while the daemon is starting up. Once started we don't + * want memory allocations to abort, since that's a DoS. + */ +static void * +my_malloc (size_t n) +{ + void *ptr = malloc (n); + if (ptr == 0) abort (); + return ptr; +} + +static char * +my_strdup (const char *str) +{ + char *str2 = strdup (str); + if (str2 == 0) abort (); + return str2; +}

I just wouldn't bother ... it's not hard to do this properly like the rest of libvirt. Okay, I've run out of steam ... hopefully I'll read over the rest another time. Cheers, Mark.