[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [Libvir] PATCH: 4/4: Configuration & whitelisting users



"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.


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]