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

Re: [libvirt] [PATCH 1/3] Add dnsmasq module (v2.1)



Satoru SATOH wrote:
> Here is updated version of the patch adds the files implements dnsmasq
> (hostsfile) module [1] based on advices from Jim-san.
>
> [1] https://www.redhat.com/archives/libvir-list/2010-April/msg01003.html
> [2] https://www.redhat.com/archives/libvir-list/2010-April/msg01046.html

Thank you for the update.
Here are a few more suggestions:

> +static int
> +hostsfileWrite(const char *path,
> +               dnsmasqDhcpHost *hosts,
> +               unsigned int nhosts)
> +{
> +    char *tmp = NULL;
> +    FILE *f;
> +    bool istmp = true;
> +    unsigned int i;
> +    int rc;

Please initialize "rc" here,

       int rc = 0;

so that, below(at the end)...

> +    if (nhosts == 0)
> +        return 0;
> +
> +    if (virAsprintf(&tmp, "%s.new", path) < 0)
> +        return -1;

Please change that to "return ENOMEM;".
At first I thought it should be "return errno;",
but neither virAsprintf nor vasprintf/asprintf specify
if/how errno is set upon failure.

> +    if (!(f = fopen(tmp, "w"))) {
> +        istmp = false;
> +        if (!(f = fopen(path, "w"))) {
> +            rc = errno;
> +            goto cleanup;
> +        }
> +    }
> +
> +    for (i = 0; i < nhosts; i++) {
> +        if (fputs(hosts[i].host, f) == EOF || fputc('\n', f) == EOF) {
> +            rc = errno;
> +            fclose(f);
> +
> +            if (istmp)
> +                unlink(tmp);
> +
> +            goto cleanup;
> +        }
> +    }
> +
> +    if (fclose(f) == EOF) {
> +        rc = errno;
> +        goto cleanup;
> +    }
> +
> +    if (istmp) {
> +        if (rename(tmp, path) < 0) {
> +            rc = errno;
> +            unlink(tmp);
> +            goto cleanup;
> +        }
> +
> +        if (unlink(tmp) < 0) {
> +            rc = errno;
> +            goto cleanup;
> +        }
> +    }
> +
> +    VIR_FREE(tmp);
> +
> +    return 0;

... you can remove the "VIR_FREE..." and "return 0" above.

> + cleanup:
> +    VIR_FREE(tmp);
> +
> +    return rc;
> +}
...
> +int
> +dnsmasqReload(pid_t pid)
> +{
> +    if (kill(pid, SIGHUP) != 0) {
> +        virReportSystemError(errno,
> +            _("Failed to make dnsmasq (PID: %d) reloading config files.\n"),

s/reloading/reload/

> +            pid);
> +        return -1;
> +    }
> +
> +    return 0;
> +}


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