"Daniel P. Berrange" <berrange@redhat.com> wrote:
This patch provides the ability to configure what authentication mechanism is used on each socket - UNIX RW, UNIX RO, TCP, and TLS sockets - all can have independant settings. By default the UNIX & TLS sockets have no auth, ...
Hi Dan, I've gone through part 3/4 and had no further feedback beyond the two comments I already made there. As usual, it looks very good. I spotted a few minor problems below. ...
diff -r 54ffed012e46 qemud/qemud.c --- a/qemud/qemud.c Thu Nov 01 16:33:15 2007 -0400 +++ b/qemud/qemud.c Thu Nov 01 16:35:57 2007 -0400 ... +static int remoteConfigGetStringList(virConfPtr conf, const char *key, char ***list, const char *filename) { + virConfValuePtr p; + + p = virConfGetValue (conf, key); + if (p) { + switch (p->type) { + case VIR_CONF_STRING: + *list = malloc (2 * sizeof (char *)); + (*list)[0] = strdup (p->str);
check for malloc and strdup failure [I do see you're just moving this existing code.]
+ (*list)[1] = 0; + break; + + case VIR_CONF_LIST: { + int i, len = 0; + virConfValuePtr pp; + for (pp = p->list; pp; pp = p->next) + len++; + *list = + malloc ((1+len) * sizeof (char *));
here too
+ for (i = 0, pp = p->list; pp; ++i, pp = p->next) { + if (pp->type != VIR_CONF_STRING) { + qemudLog (QEMUD_ERR, "remoteReadConfigFile: %s: %s: should be a string or list of strings\n", filename, key); + return -1; + } + (*list)[i] = strdup (pp->str);
here too. While technically not necessary (there'd be no segfault), a failed strdup would mean ignoring part of the config file.
+ } + (*list)[i] = 0;
s/0/NULL/ makes it clearer that (*list)[i] is a pointer. ...
-remoteReadConfigFile (const char *filename) +remoteReadConfigFile (struct qemud_server *server, const char *filename) { virConfPtr conf;
@@ -1692,6 +1739,15 @@ remoteReadConfigFile (const char *filena p = virConfGetValue (conf, "tcp_port"); CHECK_TYPE ("tcp_port", VIR_CONF_STRING); tcp_port = p ? strdup (p->str) : tcp_port;
We need to check strdup here, too, since a couple levels down, tcp_port becomes "service" in a call to getaddrinfo(NULL, service, ... and in that case, service must not be NULL. There are a few more strdup calls in that area, but for all others, it looks like it's ok for the result to be NULL.