3:03 p.m.
http://www.annexia.org/tmp/libvirt-tls-20070228.patch
Not an enormous amount of difference between this and the patch from two
days ago. This patch correctly logs/audits new connections and fixes a
whole collection of different bugs which used to happen when the server
rejected a client connection.
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)
6:20 a.m.
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.
7:56 a.m.
Mark McLoughlin wrote:
Hey,
Btw, I'm really becoming quite convinced we'll evenutally regret using
SunRPC if we stick with it.
Morning Mark, thanks for taking the time to look at the patch in detail.
Do you have some actual concrete problems with SunRPC? For me it solves
the problem of marshalling complicated data structures, including all
the error infrastructure, over the wire (see src/remote_rpc.x). It is
very trim and efficient. It demonstrably allows us to run over TLS,
Unix domain sockets, and ssh. It handles versioning for us.
On the other hand, coding with it is grotty to say the least.
But we definitely shouldn't publish the SunRPC interface or allow others
to interoperate with it, so that we can keep our options open in future.
Some more comments and questions below.
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)?
Because SunRPC code has a pleasant tendancy to write messages to stderr.
> +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?
It's required because the SunRPC client needs semi-random IDs for
outgoing messages. See src/sunrpc/create_xid.c for the details.
> @@ -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 ...
Yes, you're right. I'll fix this in the next iteration.
> +dnl /dev/urandom
> +AC_CHECK_FILES([/dev/urandom])
You don't seem to use this?
Ah indeed. I _used_ to use it, but then removed that code. I'll kill
this in the next iteration, although IIRC you'll be wanting /dev/urandom
for something?
> 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.)
I'm quite sure that git could do this, but my level of git knowledge is
pretty small at the moment, so I'm just using it as a kind of "local
CVS". I did supply some patches separately yesterday, so if those go in
to the main libvirt tree then they will disappear from future releases
of the remote patch.
> +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?
That's the way SunRPC (in glibc at least) works. Yes, you can be sure
it's never NULL in this instance.
> + /* 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?)
Indeed. Unless the -d (debug) option is passed on the command line in
which case it goes to stderr.
> +remote_close_ret *
> +remote_close_1_svc (void *vp __attribute__((unused)),
> + struct svc_req *req)
> +{
> + static struct remote_close_ret ret;
static?
Again, that's the way SunRPC servers work. There is no concurrency
problem here, because SunRPC servers don't run concurrently (at least
not the ones you can create using glibc). If we want concurrency, then
we prefork.
> +remote_type_ret *
> +remote_type_1_svc (void *vp __attribute__((unused)),
Use ATTRIBUTE_UNUSED
This is a bug. ATTRIBUTE_UNUSED isn't in the collection of headers I'm
using, but needs to be added. I'll get this fixed in the next rev.
> +
> +// Just a number large enough to use for validation of the
> +// externally produced maxids.
> +#define MAX_DOMAINS 10000
Uggh.
Yes, not sure how to do this right. Since the maxids comes from an
external process, but leads to an array being allocated inside libvirtd,
there is a very definite route to a DoS attack if the maxids parameter
is allowed to be very large. But how large is large?
> +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?
Um .. yes, that'll be a bug then. Well spotted.
> + 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)
A check is needed here - this is a bug.
As an aside, I only ever claimed we shouldn't check for malloc when the
only good thing to do is to fail. In long running daemons or any other
security-related context that's not the case because it's a DoS attack.
In any case I really need to find out what the right way to allocate
these structures is (hence XXX comment, etc.) I'll try and get this
right in the next iter.
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.
Supposedly clnt_freeres (called by the stub code in SunRPC) should free
the allocation, but that's what I need to work out.
> + ret.remote_domainCreateLinux_ret_u.domain->name
=
> + (char *) dom->name;
I'd use virDomainGetName()
Can you explain more?
> +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?
Ditto.
> +
> +/*----------------------------------------------------------------------*/
> +/* 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?
There's now a 1-1 relationship between virConnectPtr and the underlying
TCP connection. Is that what you meant? This comes out of Dan's email
about cookies (cookies have been removed completely from the code in the
meantime).
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)
http://www.faqs.org/faqs/C-faq/faq/ (section 5.2)
'0' in a pointer context is converted to the representation of NULL
(even if the representation of NULL on some wierd architecture is not
zero bits). Or do you mean stylistically you prefer NULL?
Hmm, I think I'd do this with a realloc()able array rather than
a
linked list ... let's see how that looks:
[realloc]
I'll take a look at both and use the one which is easier to
understand/shorter. (God, I really hate reimplementing fundamentals :-)
See next iteration.
> + /* 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?
Yes, that's a bug. I think my thinking was that there was no place to
log the error, but in fact that was wrong.
> +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.
This really is a "should never happen" place. If it does happen, I want
to know about it. What's the right way to report that? I worry that
logging an ordinary error will mean it's just ignored.
> +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.
Yup, bug, will investigate and fix.
> + 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?
This is during start-up. One thing I _really_ dislike about bind9 is
that errors in the zone files don't cause the thing to fail to start up.
Instead you have to manually check /var/log/messages to see if it
found any problems. If you don't check then your website disappears off
the map 12 hours later. Thus, I like daemons that fail when the
configuration file is broken.
> + 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.
I wasn't planning on it. Note that svc_run never returns, so libvirtd
can only ever exit on a signal. If my OS is leaking memory allocations,
then I've got a really big problem that free isn't going to solve.
Also, you should free the previous value before assigning.
In this case, the previous value is always a static string.
> + // This may not be an error. cf. /etc/exports
> + if (!listen_tls && !listen_tcp && !listen_unix) {
Hmm, what does this comment mean?
In nfsd, if /etc/exports is empty, then nfsd doesn't start up. So the
parallel is that if /etc/libvirtd.conf contains directives disabling
every service, then it is not a bug to just exit.
> + 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.
I was imagining that the wrapper script would clean up pid files if the
process doesn't start up correctly. I wouldn't rely on the process to
do this - what happens if it segfaults for instance?
> + 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?
In the filesystem, yes.
If the exec fails, won't we die with a SIGPIPE the first time we
write
to the pipe?
I'll take a look at this. There are actually other places where the
code could die on SIGPIPE -- possibly on client going away unexpectedly.
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?
Cut'n'paste job from the gnutls example code.
There's a Steve McConnell study where he shows that function length
doesn't affect code comprehension. I'll dig it up.
> +/* 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.
As an opaque data pointer? Yes, that's much better abstracted. I'll
fix that in the next release.
Okay, I've run out of steam ... hopefully I'll read over the
rest
another time.
Thanks again for looking at this.
Rich.
3:49 a.m.
Hi Rich,
I guess I never gave a real answer to this ...
On Thu, 2007-03-01 at 13:56 +0000, Richard W.M. Jones wrote:
Mark McLoughlin wrote:
> Hey,
> Btw, I'm really becoming quite convinced we'll evenutally regret using
> SunRPC if we stick with it.
Morning Mark, thanks for taking the time to look at the patch in detail.
Do you have some actual concrete problems with SunRPC? For me it solves
the problem of marshalling complicated data structures, including all
the error infrastructure, over the wire (see src/remote_rpc.x). It is
very trim and efficient. It demonstrably allows us to run over TLS,
Unix domain sockets, and ssh. It handles versioning for us.
On the other hand, coding with it is grotty to say the least.
But we definitely shouldn't publish the SunRPC interface or allow others
to interoperate with it, so that we can keep our options open in future.
So, thoughts on the SunRPC stuff:
- IMHO, we're never going to encourage people to use the SunRPC
interface directly, but at some point we may really want to expose
the remote interface directly and so we'll move to another
transport.
- I'm not sure the libvirt API is really well designed for remote
use, so I'm not sure that mapping the API calls to RPC calls is the
best approach.
e.g. to iterate over the list of domain UUIDs, you need to
ListDomains(), and then for each of them LookupByID() and
GetUUID(). That API might be fine for apps, but libvirt doesn't
necessarily need to map the API calls exactly to RPC calls - e.g.
you could have a ListDomains() RPC call return id/name/uuid tuples
and make LookupByID() and GetUUID() not involve a network
roundtrip[1].
One problem with that is that in order for the remote driver to
know when to invalidate the ListDomains() cache, it needs
asynchronous notification of domain creation. Which I think we want
in the API long term, anyway.
Maybe you could argue that all this is orthogonal to the transport
question - "XDR is just a marshaling format" - but I'm not
convinced, especially wrt. the async notification aspect. Also,
AFAIR "we can just map the library calls to RPC calls" was one of
the motivations for using SunRPC, so ...
- Yes, it's ugly code and even though you say it's done, code is
never really done. Especially here where there are lots of stuff
we're not sure we've gotten right - encryption, authentication,
mapping the library calls to RPC calls, async notification etc.
I think people might avoid hacking on this code, and that won't
help it evolve.
- Similarly, some people would consider SunRPC an old, legacy, crufty
protocol. RPC systems is one of those high-fashion areas where
people hold opinions which aren't necessarily terribly logical, and
so I think SunRPC will turn off hackers who might otherwise be
interested.
At the same time, though, I can sympathise with "look, we've written
all this code and it works fine, so let's just go with it". Perhaps
that's the right approach, and I'm just being a party pooper.
Cheers,
Mark.
[1] - Again, that's the kind of optimisation I think is really useful
rather than "SunRPC is faster then XML-RPC"
4:59 a.m.
Mark McLoughlin wrote:
- I'm not sure the libvirt API is really well designed for
remote
use, so I'm not sure that mapping the API calls to RPC calls is the
best approach.
The level that is right for abstraction is definitely troubling to me.
For example at the moment it'll work provided that the calls in
libvirt.c map directly on to driver calls. In other words, with a
unified Xen driver, if the functions in libvirt.c turn into:
int
virConnectGetVersion(virConnectPtr conn, unsigned long *hvVer)
{
return conn->driver->version (conn, hvVer);
}
(I've removed some error checking, but that's not material). And that
is kind of what happens at the moment, for most of the functions in
libvirt.c
With this scenario, it seems reasonable to abstract at the driver level,
which is what the remote patch does right now. We can also optimise
calls like ListDomains so that they optimistically pull all the data
over the wire that is needed for subsequent LookupByID calls.
This becomes problematic if the functions in libvirt.c become more
complicated -- for example if a single function maps to several
underlying driver calls (and I haven't really started to look at the
networking code).
e.g. to iterate over the list of domain UUIDs, you need to
ListDomains(), and then for each of them LookupByID() and
GetUUID(). That API might be fine for apps, but libvirt doesn't
necessarily need to map the API calls exactly to RPC calls - e.g.
you could have a ListDomains() RPC call return id/name/uuid tuples
and make LookupByID() and GetUUID() not involve a network
roundtrip[1].
One problem with that is that in order for the remote driver to
know when to invalidate the ListDomains() cache, it needs
asynchronous notification of domain creation. Which I think we want
in the API long term, anyway.
Agreed.
Rich.
11:19 p.m.
On Mon, Mar 05, 2007 at 09:49:42AM +0000, Mark McLoughlin wrote:
On Thu, 2007-03-01 at 13:56 +0000, Richard W.M. Jones wrote:
> Do you have some actual concrete problems with SunRPC? For me it solves
> the problem of marshalling complicated data structures, including all
> the error infrastructure, over the wire (see src/remote_rpc.x). It is
> very trim and efficient. It demonstrably allows us to run over TLS,
> Unix domain sockets, and ssh. It handles versioning for us.
>
> On the other hand, coding with it is grotty to say the least.
>
> But we definitely shouldn't publish the SunRPC interface or allow others
> to interoperate with it, so that we can keep our options open in future.
So, thoughts on the SunRPC stuff:
- IMHO, we're never going to encourage people to use the SunRPC
interface directly, but at some point we may really want to expose
the remote interface directly and so we'll move to another
transport.
I'm not sure what you mean by 'expose the remote interface directly' ?
Do you mean allow arbitrary non-libvirt clients to speak to the server
daemon directly, or something else ?
- I'm not sure the libvirt API is really well designed for
remote
use, so I'm not sure that mapping the API calls to RPC calls is the
best approach.
I agree some of the current APIs are too granular to be optimal in
terms of network traffic. In fact they're already sub-optimal even
in the local only case...
e.g. to iterate over the list of domain UUIDs, you need to
ListDomains(), and then for each of them LookupByID() and
GetUUID(). That API might be fine for apps, but libvirt doesn't
necessarily need to map the API calls exactly to RPC calls - e.g.
you could have a ListDomains() RPC call return id/name/uuid tuples
and make LookupByID() and GetUUID() not involve a network
roundtrip[1].
Even speaking to local XenD this sequence of calls is a PITA. In
virt-manager all internal tracking is based off UUID, but the API
to list domains gives me ids (for active VMs) or names (for inactive
domains). Translating to UUIDs is not nearly as lightweight as I'd
like thanks to some horrible aspects of XenD, requiring many more
RPC requests to XenD than we technically need. If we had a ListenDomains
public API method returning a tuple of (name,id,uuid), even the local
only case would be much more efficient, requiring only a 1 single
HTTP call to XenD, instead of O(n)
One problem with that is that in order for the remote driver to
know when to invalidate the ListDomains() cache, it needs
asynchronous notification of domain creation. Which I think we want
in the API long term, anyway.
Maybe you could argue that all this is orthogonal to the transport
question - "XDR is just a marshaling format" - but I'm not
convinced, especially wrt. the async notification aspect. Also,
AFAIR "we can just map the library calls to RPC calls" was one of
the motivations for using SunRPC, so ...
We do this same mapping library calls to wire messages in the QEMU
daemon, and libvirt proxy too. There isn't anything in SunRPC that
says '1 public API == 1 RPC call'. Since this is all internal impl
details, we can easily have a M-N model for public APIs <=> RPC
calls, regardless of what wire protocol, or network API we choose.
That all said, I'm not sure we'd want to do an internal M-N model
because it is going to require the maintainance of alot of state
internal to libvirt. The cache invalidation issues that implies
are non-trivial, and are quite possibly better answered by the
end application. We've already hit issues with the existing libvirt
caching of mere virDomainPtr objects in certain cases, so I'm not
all that enthusiastic about adding more complex caching.
So I'm all for adding in ways to let us get info about all domains
in batch calls & think we should seriously consider exposing batch
calls to the end applications. It'll give them alot of flexibility
in how to interact with libvirt in the most efficient manner for
their application model, while keeping the internals of libvirt
free of too much caching/state
- Yes, it's ugly code and even though you say it's done,
code is
never really done. Especially here where there are lots of stuff
we're not sure we've gotten right - encryption, authentication,
mapping the library calls to RPC calls, async notification etc.
I think people might avoid hacking on this code, and that won't
help it evolve.
- Similarly, some people would consider SunRPC an old, legacy,
crufty
protocol. RPC systems is one of those high-fashion areas where
people hold opinions which aren't necessarily terribly logical, and
so I think SunRPC will turn off hackers who might otherwise be
interested.
I'm not inclined to pay attention to fashion, otherwise I'd be writing
webapps in Ruby using XML-RPC ;-) Seriously though, I think we do need
to consider this point as one aspect of 'long term ease of maintainence'
criteria. ie less hackers == harder to maintainer. We can't decide on
that criteria alone though
At the same time, though, I can sympathise with "look,
we've written
all this code and it works fine, so let's just go with it". Perhaps
that's the right approach, and I'm just being a party pooper.
I'm seeing at least 4 core issues wrt to the question of remote management:
- Wire format - single most important aspect wrt to compatability
because once a libvirt client is released to the wild
we need to keep compatability for a non-trivial amount
of time.
- Internal API - basically what we're using inside the driver/daemon.
Within the constraints of the wire format decision, we
can change this at will since the use of the RPC API
is internal to the libvirt codebase
- API efficiency - the question of how/whether we batch up common
operations to improve the efficiency of the network
implementation. Whether any batching is private to
the libvirt internals, or placed in the public API
So how should we move forward in this whole area ? We've had many mailing
list discussions about remote management & lots of code from the proxy
to QEMU daemon, to the generic remote daemon, but there still seem to be
very different views of how this works from even the most basic starting
points.
My overriding concern is that we don't release anything until we're
confident we can support it long term without breaking compatability
with future releases. ie at very least old clients should always be
able to talk to new servers. Arguably new clients ought to be able to
talk to old servers too - albeit with possibly reduced functionality.
That clearly means we need stability in the wire format from day-1.
That puts some constraints on our internal API - but it is still
internal so it could be re-written completely if desired, provided
we keep wire format. API efficiency feels like one of those issues
we can evolve over time - if we have a wire format that lets us do
versioning, we can add new RPC calls at will without breaking old
ones.
[1] - Again, that's the kind of optimisation I think is really
useful
rather than "SunRPC is faster then XML-RPC"
I wouldn't prioritise one particular optimization over the other. The
efficiency of the general on-the-wire transmission & data (de-)marshalling
routines, is just as important to consider as the design of the APIs being
run over the transport. We really need to consider both the wire protocol
and the possibilty of 'bulk operation' APIs - a decision on one of these
shouldn't significantly impact the decision wrt the other since they're
at different layers of the comms stack.
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 -=|
5:25 a.m.
Daniel P. Berrange wrote:
On Mon, Mar 05, 2007 at 09:49:42AM +0000, Mark McLoughlin wrote:
> On Thu, 2007-03-01 at 13:56 +0000, Richard W.M. Jones wrote:
>> Do you have some actual concrete problems with SunRPC? For me it solves
>> the problem of marshalling complicated data structures, including all
>> the error infrastructure, over the wire (see src/remote_rpc.x). It is
>> very trim and efficient. It demonstrably allows us to run over TLS,
>> Unix domain sockets, and ssh. It handles versioning for us.
>>
>> On the other hand, coding with it is grotty to say the least.
>>
>> But we definitely shouldn't publish the SunRPC interface or allow others
>> to interoperate with it, so that we can keep our options open in future.
> So, thoughts on the SunRPC stuff:
>
> - IMHO, we're never going to encourage people to use the SunRPC
> interface directly, but at some point we may really want to expose
> the remote interface directly and so we'll move to another
> transport.
I'm not sure what you mean by 'expose the remote interface directly' ?
Do you mean allow arbitrary non-libvirt clients to speak to the server
daemon directly, or something else ?
I've been wondering this morning what reasons clients would have for
wanting to reverse-engineer/reimplement the wire protocol. If they're
using an obscure language without libvirt support? (Answer: write some
libvirt bindings, stupid!) If they're using an obscure language which
lacks a C FFI? If they have license problems with libvirt?
My overriding concern is that we don't release anything until
we're
confident we can support it long term without breaking compatability
with future releases. ie at very least old clients should always be
able to talk to new servers. Arguably new clients ought to be able to
talk to old servers too - albeit with possibly reduced functionality.
That clearly means we need stability in the wire format from day-1.
That puts some constraints on our internal API - but it is still
internal so it could be re-written completely if desired, provided
we keep wire format. API efficiency feels like one of those issues
we can evolve over time - if we have a wire format that lets us do
versioning, we can add new RPC calls at will without breaking old
ones.
So far, the release model for libvirt (correct me if I'm wrong) has been
that you periodically bundle up the latest version of libvirt and put it
on the website. There's no experimental branch where we can place
things like a libvirt remote patch with the proviso that at some point
later we can say "sorry, that didn't work". Apart from CVS of course
but even there it seems that once a patch is committed the libvirt
maintainers aim to support it forever.
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)
5:24 a.m.
On Tue, 2007-03-06 at 11:25 +0000, Richard W.M. Jones wrote:
So far, the release model for libvirt (correct me if I'm wrong)
has been
that you periodically bundle up the latest version of libvirt and put it
on the website. There's no experimental branch where we can place
things like a libvirt remote patch with the proviso that at some point
later we can say "sorry, that didn't work".
I'd be happy to see this stuff in CVS on a "remote branch", but it
would be quite a pain for you to maintain a branch in CVS.
Cheers,
Mark.
5:36 a.m.
Mark McLoughlin wrote:
On Tue, 2007-03-06 at 11:25 +0000, Richard W.M. Jones wrote:
> So far, the release model for libvirt (correct me if I'm wrong) has been
> that you periodically bundle up the latest version of libvirt and put it
> on the website. There's no experimental branch where we can place
> things like a libvirt remote patch with the proviso that at some point
> later we can say "sorry, that didn't work".
I'd be happy to see this stuff in CVS on a "remote branch", but it
would be quite a pain for you to maintain a branch in CVS.
It's probably about the same work that I'm doing now - periodically
pulling in CVS commits into git and resolving conflicts (there usually
aren't many, and so far none have been complex).
But yes, really I was talking about what the libvirt policy is / should
be on experimental stuff.
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)
5:36 a.m.
On Tue, 2007-03-06 at 11:36 +0000, Richard W.M. Jones wrote:
Mark McLoughlin wrote:
> On Tue, 2007-03-06 at 11:25 +0000, Richard W.M. Jones wrote:
>
>> So far, the release model for libvirt (correct me if I'm wrong) has been
>> that you periodically bundle up the latest version of libvirt and put it
>> on the website. There's no experimental branch where we can place
>> things like a libvirt remote patch with the proviso that at some point
>> later we can say "sorry, that didn't work".
>
> I'd be happy to see this stuff in CVS on a "remote branch", but it
> would be quite a pain for you to maintain a branch in CVS.
It's probably about the same work that I'm doing now - periodically
pulling in CVS commits into git and resolving conflicts (there usually
aren't many, and so far none have been complex).
It's especially a pain with CVS, though.
But yes, really I was talking about what the libvirt policy is /
should
be on experimental stuff.
Okay, in general terms, I'd be happy to see experimental stuff on
experimental branches, but it'd be quite a pain to maintain these
experimental branches.
So, experimental branches should only be used where we really can't get
the stuff on HEAD in the short term because e.g. the API won't have
settled by the time the next release is made.
You could argue that we should have a branch from which stable releases
are made, allowing unstable releases to be made, but it looks like this
"every release is stable, CVS is less stable" model works well for
smaller self-contained projects like this.
Cheers,
Mark.
3:33 a.m.
On Tue, Mar 06, 2007 at 11:25:32AM +0000, Richard W.M. Jones wrote:
Daniel P. Berrange wrote:
>I'm not sure what you mean by 'expose the remote interface directly' ?
>Do you mean allow arbitrary non-libvirt clients to speak to the server
>daemon directly, or something else ?
I've been wondering this morning what reasons clients would have for
wanting to reverse-engineer/reimplement the wire protocol. If they're
using an obscure language without libvirt support? (Answer: write some
libvirt bindings, stupid!) If they're using an obscure language which
lacks a C FFI? If they have license problems with libvirt?
Keeping C library based binding for a Java application is really
annoying, and JNI is like designed to make this hard. I would expect
large clusters monitoring solutions to be often Java based and we
need to have a network API for those use case. Whether the Sun-RPC
based one is the answer I don't think so, I guess they would be far
better off with XML-RPC, in term of existing libraries, tools, and
knowledge of programming the beast.
Daniel
--
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard | virtualization library http://libvirt.org/
veillard(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
3:56 a.m.
Daniel Veillard wrote:
On Tue, Mar 06, 2007 at 11:25:32AM +0000, Richard W.M. Jones wrote:
> Daniel P. Berrange wrote:
>> I'm not sure what you mean by 'expose the remote interface directly'
?
>> Do you mean allow arbitrary non-libvirt clients to speak to the server
>> daemon directly, or something else ?
> I've been wondering this morning what reasons clients would have for
> wanting to reverse-engineer/reimplement the wire protocol. If they're
> using an obscure language without libvirt support? (Answer: write some
> libvirt bindings, stupid!) If they're using an obscure language which
> lacks a C FFI? If they have license problems with libvirt?
Keeping C library based binding for a Java application is really
annoying, and JNI is like designed to make this hard.
Yes JNI sucks. Does anyone know what SWIG support for Java is like?
(It's pretty terrible for OCaml, the only language where I've used it,
but it is supposed to be better for more mainstream langs).
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)
4:01 a.m.
Java has SunRPC libs, for instance:
http://remotetea.sourceforge.net/
(and there are a bunch of apparently commercial ones - try searching for
'jrpcgen'). I don't have the stomach to install all the Java crap
needed to find out whether these interoperate ...
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)
5:06 p.m.
On Wed, 2007-03-07 at 04:33 -0500, Daniel Veillard wrote:
Keeping C library based binding for a Java application is really
annoying, and JNI is like designed to make this hard. I would expect
large clusters monitoring solutions to be often Java based and we
need to have a network API for those use case.
I'd be quite surprised though if those monitoring systems would talk
directly to the libvirt daemon across the wire - they are more likely to
install their own custom agent on the system (or do gross things with
ssh)
Having libvirtd running next to the monitoring daemon just to avoid JNI
pain seems like overkill.
David
10:06 a.m.
New subject: Add lots of warning flags to CFLAGS [was Re: [Libvir] Remote patch, 2007-02-28]
On Thu, 2007-03-01 at 12:20 +0000, Mark McLoughlin wrote:
> 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.
Okay, here's a patch which adds a LIBVIRT_COMPILE_WARNINGS autoconf
macro from GNOME which I've always found very, very useful.
I've changed it to include all the flags we were using.
The brilliant thing about this is that you can choose, while hacking,
to build with --enable-compile-warnings=error if you want to use -Werror
to catch any warnings you've introduced.
Cheers,
Mark.
10:09 a.m.
New subject: Add lots of warning flags to CFLAGS [was Re: [Libvir] Remote patch, 2007-02-28]
On Thu, 2007-03-01 at 16:06 +0000, Mark McLoughlin wrote:
@@ -75,7 +77,7 @@ dnl
dnl make CFLAGS very pedantic at least during the devel phase for everybody
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"
+ CFLAGS="-Wshadow -Wcast-align -Wwrite-strings -Waggregate-return
-Wstrict-prototypes -Winline -Wredundant-decls"
fi
Uh, never mind this ... fixed patch attached.
Cheers,
Mark.
10:13 a.m.
New subject: Add lots of warning flags to CFLAGS [was Re: [Libvir] Remote patch, 2007-02-28]
Mark McLoughlin wrote:
On Thu, 2007-03-01 at 16:06 +0000, Mark McLoughlin wrote:
> @@ -75,7 +77,7 @@ dnl
> dnl make CFLAGS very pedantic at least during the devel phase for everybody
> 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"
> + CFLAGS="-Wshadow -Wcast-align -Wwrite-strings -Waggregate-return
-Wstrict-prototypes -Winline -Wredundant-decls"
> fi
>
Uh, never mind this ... fixed patch attached.
Looks great.
Rich.
10:16 a.m.
New subject: Add lots of warning flags to CFLAGS [was Re: [Libvir] Remote patch, 2007-02-28]
On Thu, Mar 01, 2007 at 04:09:26PM +0000, Mark McLoughlin wrote:
On Thu, 2007-03-01 at 16:06 +0000, Mark McLoughlin wrote:
> @@ -75,7 +77,7 @@ dnl
> dnl make CFLAGS very pedantic at least during the devel phase for everybody
> 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"
> + CFLAGS="-Wshadow -Wcast-align -Wwrite-strings -Waggregate-return
-Wstrict-prototypes -Winline -Wredundant-decls"
> fi
>
Uh, never mind this ... fixed patch attached.
Looks good to me, lets commit it.
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 -=|
10:21 a.m.
New subject: Add lots of warning flags to CFLAGS [was Re: [Libvir] Remote patch, 2007-02-28]
On Thu, 2007-03-01 at 16:16 +0000, Daniel P. Berrange wrote:
On Thu, Mar 01, 2007 at 04:09:26PM +0000, Mark McLoughlin wrote:
>
> Uh, never mind this ... fixed patch attached.
Looks good to me, lets commit it.
Done. Now, who wants to make it build with
--enable-compile-warnings=error ? :-)
Cheers,
Mark.
10:23 a.m.
New subject: Add lots of warning flags to CFLAGS [was Re: [Libvir] Remote patch, 2007-02-28]
Mark McLoughlin wrote:
On Thu, 2007-03-01 at 16:16 +0000, Daniel P. Berrange wrote:
> On Thu, Mar 01, 2007 at 04:09:26PM +0000, Mark McLoughlin wrote:
>> Uh, never mind this ... fixed patch attached.
> Looks good to me, lets commit it.
Done. Now, who wants to make it build with
--enable-compile-warnings=error ? :-)
Actually that's a feature I _do_ want to use because the warnings always
fly past too quickly for me to read, so I will be testing it out.
Rich.
10:25 a.m.
New subject: Add lots of warning flags to CFLAGS [was Re: [Libvir] Remote patch, 2007-02-28]
Richard W.M. Jones wrote:
Mark McLoughlin wrote:
> On Thu, 2007-03-01 at 16:16 +0000, Daniel P. Berrange wrote:
>> On Thu, Mar 01, 2007 at 04:09:26PM +0000, Mark McLoughlin wrote:
>>> Uh, never mind this ... fixed patch attached.
>> Looks good to me, lets commit it.
>
> Done. Now, who wants to make it build with
> --enable-compile-warnings=error ? :-)
Actually that's a feature I _do_ want to use because the warnings always
fly past too quickly for me to read, so I will be testing it out.
Hmmm .... speaking too soon. rpcgen generates code stubs which do not
pass -Wunused. I may look at whether we can do per-file flags, I think
automake supports this.
Rich.
10:48 a.m.
New subject: [Libvir] Re: Add lots of warning flags to CFLAGS
"Richard W.M. Jones" <rjones(a)redhat.com> wrote:
Richard W.M. Jones wrote:
> Mark McLoughlin wrote:
>> On Thu, 2007-03-01 at 16:16 +0000, Daniel P. Berrange wrote:
>>> On Thu, Mar 01, 2007 at 04:09:26PM +0000, Mark McLoughlin wrote:
>>>> Uh, never mind this ... fixed patch attached.
>>> Looks good to me, lets commit it.
>>
>> Done. Now, who wants to make it build with
>> --enable-compile-warnings=error ? :-)
> Actually that's a feature I _do_ want to use because the warnings
> always fly past too quickly for me to read, so I will be testing it
> out.
Hmmm .... speaking too soon. rpcgen generates code stubs which do not
pass -Wunused. I may look at whether we can do per-file flags,
Hi Rich,
Or make rpcgen mark the offending bits with __attribute__ ((__unused__))?
Or, postprocess the generated code to add the same?
I think automake supports this.
You *can* do it in automake, but it's a kludge.
See the Per-Object Flags Emulation section of the manual.
"info automake per" should get it (hmm. but it doesn't work
on rawhide because the menu is missing).
http://sources.redhat.com/automake/automake.html#Per_002dObject-Flags
11:44 a.m.
New subject: [Libvir] Re: Add lots of warning flags to CFLAGS
Jim Meyering wrote:
"Richard W.M. Jones" <rjones(a)redhat.com> wrote:
> Richard W.M. Jones wrote:
>> Mark McLoughlin wrote:
>>> On Thu, 2007-03-01 at 16:16 +0000, Daniel P. Berrange wrote:
>>>> On Thu, Mar 01, 2007 at 04:09:26PM +0000, Mark McLoughlin wrote:
>>>>> Uh, never mind this ... fixed patch attached.
>>>> Looks good to me, lets commit it.
>>> Done. Now, who wants to make it build with
>>> --enable-compile-warnings=error ? :-)
>> Actually that's a feature I _do_ want to use because the warnings
>> always fly past too quickly for me to read, so I will be testing it
>> out.
> Hmmm .... speaking too soon. rpcgen generates code stubs which do not
> pass -Wunused. I may look at whether we can do per-file flags,
Hi Rich,
Or make rpcgen mark the offending bits with __attribute__ ((__unused__))?
Or, postprocess the generated code to add the same?
Postprocessing might be an idea too. For reasons I haven't quite
determined it also inserts statements with no effect ...
Rich.
6283
days inactive
6290
days old
22 comments
6 participants
participants (6)
-
Daniel P. Berrange -
Daniel Veillard -
David Lutterkort -
Jim Meyering -
Mark McLoughlin -
Richard W.M. Jones