Re: [Libvir] PATCH: 4/4: Configuration & whitelisting users
"Daniel P. Berrange" <berrange(a)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.