"Daniel P. Berrange" <berrange@xxxxxxxxxx> 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.
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list