[libvirt] [PATCH 0/3] Add dnsmasq module (v2)
The following series of patches is a prototype implementation of dnsmasq module. It implements an idea to save dhcp hosts' macaddr vs. ipaddr mappings to static file and make dnsmasq loading it with "--dhcp-hostsfile" option, originally suggested by Dan-san, and can address the problem that too many "--dhcp-host" args hitting ARG_MAX limit I reported last year [1]. This is a updated version reflecting some advices from Jim-san [2] and Eric-san [3]. [1] https://www.redhat.com/archives/libvir-list/2009-October/msg00216.html [2] https://www.redhat.com/archives/libvir-list/2010-April/msg00849.html [3] https://www.redhat.com/archives/libvir-list/2010-April/msg00850.html - satoru
This patch adds the files implements dnsmasq (hostsfile) module. Signed-off-by: Satoru SATOH <satoru.satoh@gmail.com> --- src/util/dnsmasq.c | 330 ++++++++++++++++++++++++++++++++++++++++++++++++++++ src/util/dnsmasq.h | 59 +++++++++ 2 files changed, 389 insertions(+), 0 deletions(-) create mode 100644 src/util/dnsmasq.c create mode 100644 src/util/dnsmasq.h diff --git a/src/util/dnsmasq.c b/src/util/dnsmasq.c new file mode 100644 index 0000000..f67c73e --- /dev/null +++ b/src/util/dnsmasq.c @@ -0,0 +1,330 @@ +/* + * Copyright (C) 2007-2010 Red Hat, Inc. + * Copyright (C) 2010 Satoru SATOH <satoru.satoh@gmail.com> + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + * + * Based on iptables.c + */ + +#include <config.h> + +#include <stdio.h> +#include <stdlib.h> +#include <stdarg.h> +#include <string.h> +#include <errno.h> +#include <limits.h> +#include <unistd.h> +#include <fcntl.h> +#include <sys/types.h> +#include <sys/stat.h> +#include <signal.h> + +#ifdef HAVE_PATHS_H +# include <paths.h> +#endif + +#include "internal.h" +#include "datatypes.h" +#include "dnsmasq.h" +#include "util.h" +#include "memory.h" +#include "virterror_internal.h" +#include "logging.h" + +#define VIR_FROM_THIS VIR_FROM_NETWORK +#define DNSMASQ_HOSTSFILE_SUFFIX "hostsfile" + +static void +dhcphostFree(dnsmasqDhcpHost *host) +{ + VIR_FREE(host->host); +} + +static void +hostsfileFree(dnsmasqHostsfile *hostsfile) +{ + unsigned int i; + + if (hostsfile->hosts) { + for (i = 0; i < hostsfile->nhosts; i++) + dhcphostFree(&hostsfile->hosts[i]); + + VIR_FREE(hostsfile->hosts); + + hostsfile->nhosts = 0; + } + + VIR_FREE(hostsfile->path); + + VIR_FREE(hostsfile); +} + +static int +hostsfileAdd(dnsmasqHostsfile *hostsfile, + const char *mac, + const char *ip, + const char *name) +{ + if (VIR_REALLOC_N(hostsfile->hosts, hostsfile->nhosts + 1) < 0) + goto alloc_error; + + if (name) { + if (virAsprintf(&hostsfile->hosts[hostsfile->nhosts].host, "%s,%s,%s", + mac, ip, name) < 0) { + goto alloc_error; + } + } else { + if (virAsprintf(&hostsfile->hosts[hostsfile->nhosts].host, "%s,%s", + mac, ip) < 0) { + goto alloc_error; + } + } + + hostsfile->nhosts++; + + return 0; + + alloc_error: + virReportSystemError(ENOMEM, + _("Failed to add dhcp host entry: mac=%s, ip=%s, name=%s\n"), + mac, ip, (name ? name : "(null)")); + return -1; +} + +static dnsmasqHostsfile * +hostsfileNew(const char *name, + const char *config_dir) +{ + int err; + dnsmasqHostsfile *hostsfile; + + if (VIR_ALLOC(hostsfile) < 0) + return NULL; + + hostsfile->hosts = NULL; + hostsfile->nhosts = 0; + + if (virAsprintf(&hostsfile->path, "%s/%s.%s", config_dir, name, + DNSMASQ_HOSTSFILE_SUFFIX) < 0) { + goto error; + } + + if ((err = virFileMakePath(config_dir))) { + virReportSystemError(err, _("cannot create config directory '%s'"), + config_dir); + goto error; + } + + return hostsfile; + + error: + hostsfileFree(hostsfile); + return NULL; +} + +static int +hostsfileWrite(const char *path, + dnsmasqDhcpHost *hosts, + unsigned int nhosts) +{ + char *tmp; + FILE *f; + bool istmp = true; + unsigned int i; + int saved_errno; + + if (nhosts == 0 && unlink(path) == 0) + return 0; + + if (virAsprintf(&tmp, "%s.new", path) < 0) + return -1; + + if (!(f = fopen(tmp, "w"))) { + istmp = false; + if (!(f = fopen(path, "w"))) + return errno; + } + + for (i = 0; i < nhosts; i++) { + if (fputs(hosts[i].host, f) == EOF || fputc('\n', f) == EOF) { + saved_errno = errno; + fclose(f); + + if (istmp) + unlink(tmp); + + return saved_errno; + } + } + + fclose(f); + + if (istmp && rename(tmp, path) < 0) { + saved_errno = errno; + unlink(tmp); + + return saved_errno; + } + + if (istmp) + unlink(tmp); + + return 0; +} + +static int +hostsfileSave(dnsmasqHostsfile *hostsfile) +{ + int err = hostsfileWrite(hostsfile->path, hostsfile->hosts, + hostsfile->nhosts); + + if (err < 0) { + virReportSystemError(err, _("cannot write config file '%s'"), + hostsfile->path); + return -1; + } + + return 0; +} + +static int +hostsfileDelete(dnsmasqHostsfile *hostsfile) +{ + if (!virFileExists(hostsfile->path)) + return 0; + + if (unlink(hostsfile->path) < 0) { + virReportSystemError(errno, _("cannot remove config file '%s'"), + hostsfile->path); + return -1; + } + + return 0; +} + +/** + * dnsmasqContextNew: + * + * Create a new Dnsmasq context + * + * Returns a pointer to the new structure or NULL in case of error + */ +dnsmasqContext * +dnsmasqContextNew(const char *network_name, + const char *config_dir) +{ + dnsmasqContext *ctx; + + if (VIR_ALLOC(ctx) < 0) + return NULL; + + if (!(ctx->hostsfile = hostsfileNew(network_name, config_dir))) + goto error; + + return ctx; + + error: + dnsmasqContextFree(ctx); + return NULL; +} + +/** + * dnsmasqContextFree: + * @ctx: pointer to the dnsmasq context + * + * Free the resources associated with an dnsmasq context + */ +void +dnsmasqContextFree(dnsmasqContext *ctx) +{ + if (!ctx) + return; + + if (ctx->hostsfile) + hostsfileFree(ctx->hostsfile); + + VIR_FREE(ctx); +} + +/** + * dnsmasqAddDhcpHost: + * @ctx: pointer to the dnsmasq context for each network + * @mac: pointer to the string contains mac address of the host + * @ip: pointer to the string contains ip address of the host + * @name: pointer to the string contains hostname of the host or NULL + * + * Add dhcp-host entry. + */ +void +dnsmasqAddDhcpHost(dnsmasqContext *ctx, + const char *mac, + const char *ip, + const char *name) +{ + if (ctx->hostsfile) + hostsfileAdd(ctx->hostsfile, mac, ip, name); +} + +/** + * dnsmasqSave: + * @ctx: pointer to the dnsmasq context for each network + * + * Saves all the configurations associated with a context to disk. + */ +int +dnsmasqSave(const dnsmasqContext *ctx) +{ + if (ctx->hostsfile) + return hostsfileSave(ctx->hostsfile); + + return 0; +} + + +/** + * dnsmasqDelete: + * @ctx: pointer to the dnsmasq context for each network + * + * Delete all the configuration files associated with a context. + */ +int +dnsmasqDelete(const dnsmasqContext *ctx) +{ + if (ctx->hostsfile) + return hostsfileDelete(ctx->hostsfile); + + return 0; +} + +/** + * dnsmasqReload: + * @pid: the pid of the target dnsmasq process + * + * Reloads all the configurations associated to a context + */ +int +dnsmasqReload(pid_t pid) +{ + if (kill(pid, SIGHUP) != 0) { + virReportSystemError(errno, + _("Failed to make dnsmasq (PID: %d) reloading config files.\n"), + pid); + return -1; + } + + return 0; +} diff --git a/src/util/dnsmasq.h b/src/util/dnsmasq.h new file mode 100644 index 0000000..121ccad --- /dev/null +++ b/src/util/dnsmasq.h @@ -0,0 +1,59 @@ +/* + * Copyright (C) 2007-2010 Red Hat, Inc. + * Copyright (C) 2010 Satoru SATOH <satoru.satoh@gmail.com> + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + * + * based on iptables.h + */ + +#ifndef __DNSMASQ_H__ +# define __DNSMASQ_H__ + +typedef struct +{ + /* + * Each entry holds a string, "<mac_addr>,<hostname>,<ip_addr>" such as + * "01:23:45:67:89:0a,foo,10.0.0.3". + */ + char *host; + +} dnsmasqDhcpHost; + +typedef struct +{ + unsigned int nhosts; + dnsmasqDhcpHost *hosts; + + char *path; /* Absolute path of dnsmasq's hostsfile. */ +} dnsmasqHostsfile; + +typedef struct +{ + dnsmasqHostsfile *hostsfile; +} dnsmasqContext; + +dnsmasqContext * dnsmasqContextNew(const char *network_name, + const char *config_dir); +void dnsmasqContextFree(dnsmasqContext *ctx); +void dnsmasqAddDhcpHost(dnsmasqContext *ctx, + const char *mac, + const char *ip, + const char *name); +int dnsmasqSave(const dnsmasqContext *ctx); +int dnsmasqDelete(const dnsmasqContext *ctx); +int dnsmasqReload(pid_t pid); + +#endif /* __DNSMASQ_H__ */ -- 1.6.6.1
Satoru SATOH wrote: ...
+static int +hostsfileWrite(const char *path, + dnsmasqDhcpHost *hosts, + unsigned int nhosts) +{ + char *tmp; + FILE *f; + bool istmp = true; + unsigned int i; + int saved_errno; + + if (nhosts == 0 && unlink(path) == 0) + return 0; + + if (virAsprintf(&tmp, "%s.new", path) < 0) + return -1;
Any return after this point must be preceded by code that frees "tmp". Otherwise it is a leak.
+ if (!(f = fopen(tmp, "w"))) { + istmp = false; + if (!(f = fopen(path, "w"))) + return errno; + } + + for (i = 0; i < nhosts; i++) { + if (fputs(hosts[i].host, f) == EOF || fputc('\n', f) == EOF) { + saved_errno = errno; + fclose(f); + + if (istmp) + unlink(tmp); + + return saved_errno; + } + } + + fclose(f);
I mentioned this already. You must detect write failure here.
+ if (istmp && rename(tmp, path) < 0) { + saved_errno = errno; + unlink(tmp); + + return saved_errno; + } + + if (istmp) + unlink(tmp);
Failure to remove this temporary file might deserve a warning. No big deal either way.
+ + return 0; +} ...
+static int +hostsfileDelete(dnsmasqHostsfile *hostsfile) +{ + if (!virFileExists(hostsfile->path)) + return 0; + + if (unlink(hostsfile->path) < 0) { + virReportSystemError(errno, _("cannot remove config file '%s'"),
I think saying "failed to..." is slightly clearer than "cannot...".
+ hostsfile->path); + return -1; + } + + return 0; +} + +/** + * dnsmasqAddDhcpHost: + * @ctx: pointer to the dnsmasq context for each network + * @mac: pointer to the string contains mac address of the host + * @ip: pointer to the string contains ip address of the host + * @name: pointer to the string contains hostname of the host or NULL + * + * Add dhcp-host entry. + */ +void +dnsmasqAddDhcpHost(dnsmasqContext *ctx, + const char *mac, + const char *ip, + const char *name) +{ + if (ctx->hostsfile) + hostsfileAdd(ctx->hostsfile, mac, ip, name);
Would a caller ever want to know if this function fails? If so, don't ignore hostsfileAdd failure. That would be more consistent with how the following functions work, too. BTW, shouldn't hostsfileAdd be spelled hostsFileAdd (with a capital "F")?
+} + +/** + * dnsmasqSave: + * @ctx: pointer to the dnsmasq context for each network + * + * Saves all the configurations associated with a context to disk. + */ +int +dnsmasqSave(const dnsmasqContext *ctx) +{ + if (ctx->hostsfile) + return hostsfileSave(ctx->hostsfile); + return 0; +}
Thanks again for your advice! On Fri, Apr 23, 2010 at 10:42:14AM +0200, Jim Meyering wrote:
Satoru SATOH wrote: ...
+static int +hostsfileWrite(const char *path, + dnsmasqDhcpHost *hosts, + unsigned int nhosts) +{ + char *tmp; + FILE *f; + bool istmp = true; + unsigned int i; + int saved_errno; + + if (nhosts == 0 && unlink(path) == 0) + return 0; + + if (virAsprintf(&tmp, "%s.new", path) < 0) + return -1;
Any return after this point must be preceded by code that frees "tmp". Otherwise it is a leak.
oops, abosolutely right. fixed locally.
+ if (!(f = fopen(tmp, "w"))) { + istmp = false; + if (!(f = fopen(path, "w"))) + return errno; + } + + for (i = 0; i < nhosts; i++) { + if (fputs(hosts[i].host, f) == EOF || fputc('\n', f) == EOF) { + saved_errno = errno; + fclose(f); + + if (istmp) + unlink(tmp); + + return saved_errno; + } + } + + fclose(f);
I mentioned this already. You must detect write failure here.
sorry, missed.
+ if (istmp && rename(tmp, path) < 0) { + saved_errno = errno; + unlink(tmp); + + return saved_errno; + } + + if (istmp) + unlink(tmp);
Failure to remove this temporary file might deserve a warning. No big deal either way.
fixed.
+ + return 0; +} ...
+static int +hostsfileDelete(dnsmasqHostsfile *hostsfile) +{ + if (!virFileExists(hostsfile->path)) + return 0; + + if (unlink(hostsfile->path) < 0) { + virReportSystemError(errno, _("cannot remove config file '%s'"),
I think saying "failed to..." is slightly clearer than "cannot...".
+ hostsfile->path); + return -1; + } + + return 0; +} + +/** + * dnsmasqAddDhcpHost: + * @ctx: pointer to the dnsmasq context for each network + * @mac: pointer to the string contains mac address of the host + * @ip: pointer to the string contains ip address of the host + * @name: pointer to the string contains hostname of the host or NULL + * + * Add dhcp-host entry. + */ +void +dnsmasqAddDhcpHost(dnsmasqContext *ctx, + const char *mac, + const char *ip, + const char *name) +{ + if (ctx->hostsfile) + hostsfileAdd(ctx->hostsfile, mac, ip, name);
Would a caller ever want to know if this function fails? If so, don't ignore hostsfileAdd failure. That would be more consistent with how the following functions work, too.
right but I want to keep as it is for a while.
BTW, shouldn't hostsfileAdd be spelled hostsFileAdd (with a capital "F")?
+} + +/** + * dnsmasqSave: + * @ctx: pointer to the dnsmasq context for each network + * + * Saves all the configurations associated with a context to disk. + */ +int +dnsmasqSave(const dnsmasqContext *ctx) +{ + if (ctx->hostsfile) + return hostsfileSave(ctx->hostsfile); + return 0; +}
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 Signed-off-by: Satoru SATOH <satoru.satoh@gmail.com> --- src/util/dnsmasq.c | 345 ++++++++++++++++++++++++++++++++++++++++++++++++++++ src/util/dnsmasq.h | 59 +++++++++ 2 files changed, 404 insertions(+), 0 deletions(-) create mode 100644 src/util/dnsmasq.c create mode 100644 src/util/dnsmasq.h diff --git a/src/util/dnsmasq.c b/src/util/dnsmasq.c new file mode 100644 index 0000000..27050d9 --- /dev/null +++ b/src/util/dnsmasq.c @@ -0,0 +1,345 @@ +/* + * Copyright (C) 2007-2010 Red Hat, Inc. + * Copyright (C) 2010 Satoru SATOH <satoru.satoh@gmail.com> + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + * + * Based on iptables.c + */ + +#include <config.h> + +#include <stdio.h> +#include <stdlib.h> +#include <stdarg.h> +#include <string.h> +#include <errno.h> +#include <limits.h> +#include <unistd.h> +#include <fcntl.h> +#include <sys/types.h> +#include <sys/stat.h> +#include <signal.h> + +#ifdef HAVE_PATHS_H +# include <paths.h> +#endif + +#include "internal.h" +#include "datatypes.h" +#include "dnsmasq.h" +#include "util.h" +#include "memory.h" +#include "virterror_internal.h" +#include "logging.h" + +#define VIR_FROM_THIS VIR_FROM_NETWORK +#define DNSMASQ_HOSTSFILE_SUFFIX "hostsfile" + +static void +dhcphostFree(dnsmasqDhcpHost *host) +{ + VIR_FREE(host->host); +} + +static void +hostsfileFree(dnsmasqHostsfile *hostsfile) +{ + unsigned int i; + + if (hostsfile->hosts) { + for (i = 0; i < hostsfile->nhosts; i++) + dhcphostFree(&hostsfile->hosts[i]); + + VIR_FREE(hostsfile->hosts); + + hostsfile->nhosts = 0; + } + + VIR_FREE(hostsfile->path); + + VIR_FREE(hostsfile); +} + +static int +hostsfileAdd(dnsmasqHostsfile *hostsfile, + const char *mac, + const char *ip, + const char *name) +{ + if (VIR_REALLOC_N(hostsfile->hosts, hostsfile->nhosts + 1) < 0) + goto alloc_error; + + if (name) { + if (virAsprintf(&hostsfile->hosts[hostsfile->nhosts].host, "%s,%s,%s", + mac, ip, name) < 0) { + goto alloc_error; + } + } else { + if (virAsprintf(&hostsfile->hosts[hostsfile->nhosts].host, "%s,%s", + mac, ip) < 0) { + goto alloc_error; + } + } + + hostsfile->nhosts++; + + return 0; + + alloc_error: + virReportSystemError(ENOMEM, + _("Failed to add dhcp host entry: mac=%s, ip=%s, name=%s\n"), + mac, ip, (name ? name : "(null)")); + return -1; +} + +static dnsmasqHostsfile * +hostsfileNew(const char *name, + const char *config_dir) +{ + int err; + dnsmasqHostsfile *hostsfile; + + if (VIR_ALLOC(hostsfile) < 0) + return NULL; + + hostsfile->hosts = NULL; + hostsfile->nhosts = 0; + + if (virAsprintf(&hostsfile->path, "%s/%s.%s", config_dir, name, + DNSMASQ_HOSTSFILE_SUFFIX) < 0) { + goto error; + } + + if ((err = virFileMakePath(config_dir))) { + virReportSystemError(err, _("cannot create config directory '%s'"), + config_dir); + goto error; + } + + return hostsfile; + + error: + hostsfileFree(hostsfile); + return NULL; +} + +static int +hostsfileWrite(const char *path, + dnsmasqDhcpHost *hosts, + unsigned int nhosts) +{ + char *tmp = NULL; + FILE *f; + bool istmp = true; + unsigned int i; + int rc; + + if (nhosts == 0) + return 0; + + if (virAsprintf(&tmp, "%s.new", path) < 0) + return -1; + + 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; + + cleanup: + VIR_FREE(tmp); + + return rc; +} + +static int +hostsfileSave(dnsmasqHostsfile *hostsfile) +{ + int err = hostsfileWrite(hostsfile->path, hostsfile->hosts, + hostsfile->nhosts); + + if (err < 0) { + virReportSystemError(err, _("cannot write config file '%s'"), + hostsfile->path); + return -1; + } + + return 0; +} + +static int +hostsfileDelete(dnsmasqHostsfile *hostsfile) +{ + if (!virFileExists(hostsfile->path)) + return 0; + + if (unlink(hostsfile->path) < 0) { + virReportSystemError(errno, _("cannot remove config file '%s'"), + hostsfile->path); + return -1; + } + + return 0; +} + +/** + * dnsmasqContextNew: + * + * Create a new Dnsmasq context + * + * Returns a pointer to the new structure or NULL in case of error + */ +dnsmasqContext * +dnsmasqContextNew(const char *network_name, + const char *config_dir) +{ + dnsmasqContext *ctx; + + if (VIR_ALLOC(ctx) < 0) + return NULL; + + if (!(ctx->hostsfile = hostsfileNew(network_name, config_dir))) + goto error; + + return ctx; + + error: + dnsmasqContextFree(ctx); + return NULL; +} + +/** + * dnsmasqContextFree: + * @ctx: pointer to the dnsmasq context + * + * Free the resources associated with an dnsmasq context + */ +void +dnsmasqContextFree(dnsmasqContext *ctx) +{ + if (!ctx) + return; + + if (ctx->hostsfile) + hostsfileFree(ctx->hostsfile); + + VIR_FREE(ctx); +} + +/** + * dnsmasqAddDhcpHost: + * @ctx: pointer to the dnsmasq context for each network + * @mac: pointer to the string contains mac address of the host + * @ip: pointer to the string contains ip address of the host + * @name: pointer to the string contains hostname of the host or NULL + * + * Add dhcp-host entry. + */ +void +dnsmasqAddDhcpHost(dnsmasqContext *ctx, + const char *mac, + const char *ip, + const char *name) +{ + if (ctx->hostsfile) + hostsfileAdd(ctx->hostsfile, mac, ip, name); +} + +/** + * dnsmasqSave: + * @ctx: pointer to the dnsmasq context for each network + * + * Saves all the configurations associated with a context to disk. + */ +int +dnsmasqSave(const dnsmasqContext *ctx) +{ + if (ctx->hostsfile) + return hostsfileSave(ctx->hostsfile); + + return 0; +} + + +/** + * dnsmasqDelete: + * @ctx: pointer to the dnsmasq context for each network + * + * Delete all the configuration files associated with a context. + */ +int +dnsmasqDelete(const dnsmasqContext *ctx) +{ + if (ctx->hostsfile) + return hostsfileDelete(ctx->hostsfile); + + return 0; +} + +/** + * dnsmasqReload: + * @pid: the pid of the target dnsmasq process + * + * Reloads all the configurations associated to a context + */ +int +dnsmasqReload(pid_t pid) +{ + if (kill(pid, SIGHUP) != 0) { + virReportSystemError(errno, + _("Failed to make dnsmasq (PID: %d) reloading config files.\n"), + pid); + return -1; + } + + return 0; +} diff --git a/src/util/dnsmasq.h b/src/util/dnsmasq.h new file mode 100644 index 0000000..121ccad --- /dev/null +++ b/src/util/dnsmasq.h @@ -0,0 +1,59 @@ +/* + * Copyright (C) 2007-2010 Red Hat, Inc. + * Copyright (C) 2010 Satoru SATOH <satoru.satoh@gmail.com> + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + * + * based on iptables.h + */ + +#ifndef __DNSMASQ_H__ +# define __DNSMASQ_H__ + +typedef struct +{ + /* + * Each entry holds a string, "<mac_addr>,<hostname>,<ip_addr>" such as + * "01:23:45:67:89:0a,foo,10.0.0.3". + */ + char *host; + +} dnsmasqDhcpHost; + +typedef struct +{ + unsigned int nhosts; + dnsmasqDhcpHost *hosts; + + char *path; /* Absolute path of dnsmasq's hostsfile. */ +} dnsmasqHostsfile; + +typedef struct +{ + dnsmasqHostsfile *hostsfile; +} dnsmasqContext; + +dnsmasqContext * dnsmasqContextNew(const char *network_name, + const char *config_dir); +void dnsmasqContextFree(dnsmasqContext *ctx); +void dnsmasqAddDhcpHost(dnsmasqContext *ctx, + const char *mac, + const char *ip, + const char *name); +int dnsmasqSave(const dnsmasqContext *ctx); +int dnsmasqDelete(const dnsmasqContext *ctx); +int dnsmasqReload(pid_t pid); + +#endif /* __DNSMASQ_H__ */ -- 1.6.2.5
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; +}
Ok, here is updated one. Could you please take a look at this? This patch adds the files implements dnsmasq (hostsfile) module. Signed-off-by: Satoru SATOH <satoru.satoh@gmail.com> --- src/util/dnsmasq.c | 341 ++++++++++++++++++++++++++++++++++++++++++++++++++++ src/util/dnsmasq.h | 59 +++++++++ 2 files changed, 400 insertions(+), 0 deletions(-) create mode 100644 src/util/dnsmasq.c create mode 100644 src/util/dnsmasq.h diff --git a/src/util/dnsmasq.c b/src/util/dnsmasq.c new file mode 100644 index 0000000..a6197be --- /dev/null +++ b/src/util/dnsmasq.c @@ -0,0 +1,341 @@ +/* + * Copyright (C) 2007-2010 Red Hat, Inc. + * Copyright (C) 2010 Satoru SATOH <satoru.satoh@gmail.com> + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + * + * Based on iptables.c + */ + +#include <config.h> + +#include <stdio.h> +#include <stdlib.h> +#include <stdarg.h> +#include <string.h> +#include <errno.h> +#include <limits.h> +#include <unistd.h> +#include <fcntl.h> +#include <sys/types.h> +#include <sys/stat.h> +#include <signal.h> + +#ifdef HAVE_PATHS_H +# include <paths.h> +#endif + +#include "internal.h" +#include "datatypes.h" +#include "dnsmasq.h" +#include "util.h" +#include "memory.h" +#include "virterror_internal.h" +#include "logging.h" + +#define VIR_FROM_THIS VIR_FROM_NETWORK +#define DNSMASQ_HOSTSFILE_SUFFIX "hostsfile" + +static void +dhcphostFree(dnsmasqDhcpHost *host) +{ + VIR_FREE(host->host); +} + +static void +hostsfileFree(dnsmasqHostsfile *hostsfile) +{ + unsigned int i; + + if (hostsfile->hosts) { + for (i = 0; i < hostsfile->nhosts; i++) + dhcphostFree(&hostsfile->hosts[i]); + + VIR_FREE(hostsfile->hosts); + + hostsfile->nhosts = 0; + } + + VIR_FREE(hostsfile->path); + + VIR_FREE(hostsfile); +} + +static int +hostsfileAdd(dnsmasqHostsfile *hostsfile, + const char *mac, + const char *ip, + const char *name) +{ + if (VIR_REALLOC_N(hostsfile->hosts, hostsfile->nhosts + 1) < 0) + goto alloc_error; + + if (name) { + if (virAsprintf(&hostsfile->hosts[hostsfile->nhosts].host, "%s,%s,%s", + mac, ip, name) < 0) { + goto alloc_error; + } + } else { + if (virAsprintf(&hostsfile->hosts[hostsfile->nhosts].host, "%s,%s", + mac, ip) < 0) { + goto alloc_error; + } + } + + hostsfile->nhosts++; + + return 0; + + alloc_error: + virReportSystemError(ENOMEM, + _("Failed to add dhcp host entry: mac=%s, ip=%s, name=%s\n"), + mac, ip, (name ? name : "(null)")); + return -1; +} + +static dnsmasqHostsfile * +hostsfileNew(const char *name, + const char *config_dir) +{ + int err; + dnsmasqHostsfile *hostsfile; + + if (VIR_ALLOC(hostsfile) < 0) + return NULL; + + hostsfile->hosts = NULL; + hostsfile->nhosts = 0; + + if (virAsprintf(&hostsfile->path, "%s/%s.%s", config_dir, name, + DNSMASQ_HOSTSFILE_SUFFIX) < 0) { + goto error; + } + + if ((err = virFileMakePath(config_dir))) { + virReportSystemError(err, _("cannot create config directory '%s'"), + config_dir); + goto error; + } + + return hostsfile; + + error: + hostsfileFree(hostsfile); + return NULL; +} + +static int +hostsfileWrite(const char *path, + dnsmasqDhcpHost *hosts, + unsigned int nhosts) +{ + char *tmp = NULL; + FILE *f; + bool istmp = true; + unsigned int i; + int rc = 0; + + if (nhosts == 0) + return rc; + + if (virAsprintf(&tmp, "%s.new", path) < 0) + return ENOMEM; + + 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; + } + } + + cleanup: + VIR_FREE(tmp); + + return rc; +} + +static int +hostsfileSave(dnsmasqHostsfile *hostsfile) +{ + int err = hostsfileWrite(hostsfile->path, hostsfile->hosts, + hostsfile->nhosts); + + if (err < 0) { + virReportSystemError(err, _("cannot write config file '%s'"), + hostsfile->path); + return -1; + } + + return 0; +} + +static int +hostsfileDelete(dnsmasqHostsfile *hostsfile) +{ + if (!virFileExists(hostsfile->path)) + return 0; + + if (unlink(hostsfile->path) < 0) { + virReportSystemError(errno, _("cannot remove config file '%s'"), + hostsfile->path); + return -1; + } + + return 0; +} + +/** + * dnsmasqContextNew: + * + * Create a new Dnsmasq context + * + * Returns a pointer to the new structure or NULL in case of error + */ +dnsmasqContext * +dnsmasqContextNew(const char *network_name, + const char *config_dir) +{ + dnsmasqContext *ctx; + + if (VIR_ALLOC(ctx) < 0) + return NULL; + + if (!(ctx->hostsfile = hostsfileNew(network_name, config_dir))) + goto error; + + return ctx; + + error: + dnsmasqContextFree(ctx); + return NULL; +} + +/** + * dnsmasqContextFree: + * @ctx: pointer to the dnsmasq context + * + * Free the resources associated with an dnsmasq context + */ +void +dnsmasqContextFree(dnsmasqContext *ctx) +{ + if (!ctx) + return; + + if (ctx->hostsfile) + hostsfileFree(ctx->hostsfile); + + VIR_FREE(ctx); +} + +/** + * dnsmasqAddDhcpHost: + * @ctx: pointer to the dnsmasq context for each network + * @mac: pointer to the string contains mac address of the host + * @ip: pointer to the string contains ip address of the host + * @name: pointer to the string contains hostname of the host or NULL + * + * Add dhcp-host entry. + */ +void +dnsmasqAddDhcpHost(dnsmasqContext *ctx, + const char *mac, + const char *ip, + const char *name) +{ + if (ctx->hostsfile) + hostsfileAdd(ctx->hostsfile, mac, ip, name); +} + +/** + * dnsmasqSave: + * @ctx: pointer to the dnsmasq context for each network + * + * Saves all the configurations associated with a context to disk. + */ +int +dnsmasqSave(const dnsmasqContext *ctx) +{ + if (ctx->hostsfile) + return hostsfileSave(ctx->hostsfile); + + return 0; +} + + +/** + * dnsmasqDelete: + * @ctx: pointer to the dnsmasq context for each network + * + * Delete all the configuration files associated with a context. + */ +int +dnsmasqDelete(const dnsmasqContext *ctx) +{ + if (ctx->hostsfile) + return hostsfileDelete(ctx->hostsfile); + + return 0; +} + +/** + * dnsmasqReload: + * @pid: the pid of the target dnsmasq process + * + * Reloads all the configurations associated to a context + */ +int +dnsmasqReload(pid_t pid) +{ + if (kill(pid, SIGHUP) != 0) { + virReportSystemError(errno, + _("Failed to make dnsmasq (PID: %d) reload config files.\n"), + pid); + return -1; + } + + return 0; +} diff --git a/src/util/dnsmasq.h b/src/util/dnsmasq.h new file mode 100644 index 0000000..121ccad --- /dev/null +++ b/src/util/dnsmasq.h @@ -0,0 +1,59 @@ +/* + * Copyright (C) 2007-2010 Red Hat, Inc. + * Copyright (C) 2010 Satoru SATOH <satoru.satoh@gmail.com> + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + * + * based on iptables.h + */ + +#ifndef __DNSMASQ_H__ +# define __DNSMASQ_H__ + +typedef struct +{ + /* + * Each entry holds a string, "<mac_addr>,<hostname>,<ip_addr>" such as + * "01:23:45:67:89:0a,foo,10.0.0.3". + */ + char *host; + +} dnsmasqDhcpHost; + +typedef struct +{ + unsigned int nhosts; + dnsmasqDhcpHost *hosts; + + char *path; /* Absolute path of dnsmasq's hostsfile. */ +} dnsmasqHostsfile; + +typedef struct +{ + dnsmasqHostsfile *hostsfile; +} dnsmasqContext; + +dnsmasqContext * dnsmasqContextNew(const char *network_name, + const char *config_dir); +void dnsmasqContextFree(dnsmasqContext *ctx); +void dnsmasqAddDhcpHost(dnsmasqContext *ctx, + const char *mac, + const char *ip, + const char *name); +int dnsmasqSave(const dnsmasqContext *ctx); +int dnsmasqDelete(const dnsmasqContext *ctx); +int dnsmasqReload(pid_t pid); + +#endif /* __DNSMASQ_H__ */ -- 1.6.2.5
Satoru SATOH wrote:
Ok, here is updated one. Could you please take a look at this?
Thanks. That looks fine now. modulo one nit: ...
+static int +hostsfileWrite(const char *path, + dnsmasqDhcpHost *hosts, + unsigned int nhosts) +{ + char *tmp = NULL;
This is a dead store, too. char *tmp;
+ FILE *f; + bool istmp = true; + unsigned int i; + int rc = 0; + + if (nhosts == 0) + return rc; + + if (virAsprintf(&tmp, "%s.new", path) < 0) + return ENOMEM; ...
This patch adds build support for the dnsmasq module. Signed-off-by: Satoru SATOH <satoru.satoh@gmail.com> --- po/POTFILES.in | 1 + src/Makefile.am | 1 + src/libvirt_private.syms | 9 +++++++++ 3 files changed, 11 insertions(+), 0 deletions(-) diff --git a/po/POTFILES.in b/po/POTFILES.in index 58e7358..88218bd 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -71,6 +71,7 @@ src/uml/uml_driver.c src/util/authhelper.c src/util/bridge.c src/util/conf.c +src/util/dnsmasq.c src/util/hooks.c src/util/hostusb.c src/util/interface.c diff --git a/src/Makefile.am b/src/Makefile.am index 66dc349..9b3feb0 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -59,6 +59,7 @@ UTIL_SOURCES = \ util/hooks.c util/hooks.h \ util/iptables.c util/iptables.h \ util/ebtables.c util/ebtables.h \ + util/dnsmasq.c util/dnsmasq.h \ util/json.c util/json.h \ util/logging.c util/logging.h \ util/macvtap.c util/macvtap.h \ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 7950bcd..11791a7 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -326,6 +326,15 @@ iptablesRemoveTcpInput; iptablesRemoveUdpInput; +# dnsmasq.h +dnsmasqContextNew; +dnsmasqContextFree; +dnsmasqAddDhcpHost; +dnsmasqSave; +dnsmasqDelete; +dnsmasqReload; + + # libvirt_internal.h virDrvSupportsFeature; virDomainMigratePrepare; -- 1.6.6.1
This patch makes dnsmasq daemon started from libvirtd with a --dhcp-hostsfile option instead of --dhcp-host options for each '//ip/dhcp/host' entries defined in network xml file. Remaining issues: * when and where to save hostsfiles Signed-off-by: Satoru SATOH <satoru.satoh@gmail.com> --- src/network/bridge_driver.c | 78 +++++++++++++++++++++++++++++++++--------- 1 files changed, 61 insertions(+), 17 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 83ab00e..79ebb15 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -57,10 +57,13 @@ #include "iptables.h" #include "bridge.h" #include "logging.h" +#include "dnsmasq.h" #define NETWORK_PID_DIR LOCAL_STATE_DIR "/run/libvirt/network" #define NETWORK_STATE_DIR LOCAL_STATE_DIR "/lib/libvirt/network" +#define DNSMASQ_STATE_DIR LOCAL_STATE_DIR "/lib/dnsmasq" + #define VIR_FROM_THIS VIR_FROM_NETWORK #define networkReportError(code, ...) \ @@ -361,6 +364,29 @@ networkShutdown(void) { static int +networkSaveDnsmasqHostsfile(virNetworkObjPtr network, + dnsmasqContext *dctx, + bool force) +{ + unsigned int i; + + if (! force && virFileExists(dctx->hostsfile->path)) + return 1; + + for (i = 0 ; i < network->def->nhosts ; i++) { + virNetworkDHCPHostDefPtr host = &(network->def->hosts[i]); + if ((host->mac) && (host->ip)) + dnsmasqAddDhcpHost(dctx, host->mac, host->ip, host->name); + } + + if (dnsmasqSave(dctx) < 0) + return 0; + + return 1; +} + + +static int networkBuildDnsmasqArgv(virNetworkObjPtr network, const char *pidfile, const char ***argv) { @@ -401,8 +427,8 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network, (2 * network->def->nranges) + /* --dhcp-range 10.0.0.2,10.0.0.254 */ /* --dhcp-lease-max=xxx if needed */ (network->def->nranges ? 0 : 1) + - /* --dhcp-host 01:23:45:67:89:0a,hostname,10.0.0.3 */ - (2 * network->def->nhosts) + + /* --dhcp-hostsfile=/var/lib/dnsmasq/$NAME.hostsfile */ + (network->def->nhosts > 0 ? 1 : 0) + /* --enable-tftp --tftp-root /srv/tftp */ (network->def->tftproot ? 3 : 0) + /* --dhcp-boot pxeboot.img[,,12.34.56.78] */ @@ -473,22 +499,22 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network, APPEND_ARG(*argv, i++, buf); } - for (r = 0 ; r < network->def->nhosts ; r++) { - virNetworkDHCPHostDefPtr host = &(network->def->hosts[r]); - if ((host->mac) && (host->name)) { - snprintf(buf, sizeof(buf), "%s,%s,%s", - host->mac, host->name, host->ip); - } else if (host->mac) { - snprintf(buf, sizeof(buf), "%s,%s", - host->mac, host->ip); - } else if (host->name) { - snprintf(buf, sizeof(buf), "%s,%s", - host->name, host->ip); - } else - continue; + if (network->def->nhosts > 0) { + dnsmasqContext *dctx = dnsmasqContextNew(network->def->name, DNSMASQ_STATE_DIR); + char *hostsfileArg; - APPEND_ARG(*argv, i++, "--dhcp-host"); - APPEND_ARG(*argv, i++, buf); + if (dctx == NULL) + goto no_memory; + + if (networkSaveDnsmasqHostsfile(network, dctx, false)) { + if (virAsprintf(&hostsfileArg, "--dhcp-hostsfile=%s", dctx->hostsfile->path) < 0) { + dnsmasqContextFree(dctx); + goto no_memory; + } + APPEND_ARG_LIT(*argv, i++, hostsfileArg); + } + + dnsmasqContextFree(dctx); } if (network->def->tftproot) { @@ -1294,6 +1320,15 @@ static virNetworkPtr networkDefine(virConnectPtr conn, const char *xml) { goto cleanup; } + if (network->def->nhosts > 0) { + dnsmasqContext *dctx = dnsmasqContextNew(network->def->name, DNSMASQ_STATE_DIR); + if (dctx == NULL) + goto cleanup; + + networkSaveDnsmasqHostsfile(network, dctx, true); + dnsmasqContextFree(dctx); + } + ret = virGetNetwork(conn, network->def->name, network->def->uuid); cleanup: @@ -1329,6 +1364,15 @@ static int networkUndefine(virNetworkPtr net) { network) < 0) goto cleanup; + if (network->def->nhosts > 0) { + dnsmasqContext *dctx = dnsmasqContextNew(network->def->name, DNSMASQ_STATE_DIR); + if (dctx == NULL) + goto cleanup; + + dnsmasqDelete(dctx); + dnsmasqContextFree(dctx); + } + virNetworkRemoveInactive(&driver->networks, network); network = NULL; -- 1.6.6.1
On Thu, Apr 22, 2010 at 11:41:04PM +0900, Satoru SATOH wrote:
The following series of patches is a prototype implementation of dnsmasq module.
It implements an idea to save dhcp hosts' macaddr vs. ipaddr mappings to static file and make dnsmasq loading it with "--dhcp-hostsfile" option, originally suggested by Dan-san, and can address the problem that too many "--dhcp-host" args hitting ARG_MAX limit I reported last year [1].
This is a updated version reflecting some advices from Jim-san [2] and Eric-san [3].
[1] https://www.redhat.com/archives/libvir-list/2009-October/msg00216.html [2] https://www.redhat.com/archives/libvir-list/2010-April/msg00849.html [3] https://www.redhat.com/archives/libvir-list/2010-April/msg00850.html
Hi Satoru, looking at the patches, those looks fine to me, I may have just a couple of cosmetic comments, but I'm wondering if they should be postponed after 0.8.1, or if it's fine to push them in now. On one hand I would prefer to limit the number of actual features in 0.8.1, but on the other hand you have already submitted this many time so I wonder what you think. When you say "prototype implementation" how confident are you about this code ? So what do you think ? thanks ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On Fri, Apr 23, 2010 at 09:33:20AM +0200, Daniel Veillard wrote:
On Thu, Apr 22, 2010 at 11:41:04PM +0900, Satoru SATOH wrote:
The following series of patches is a prototype implementation of dnsmasq module.
It implements an idea to save dhcp hosts' macaddr vs. ipaddr mappings to static file and make dnsmasq loading it with "--dhcp-hostsfile" option, originally suggested by Dan-san, and can address the problem that too many "--dhcp-host" args hitting ARG_MAX limit I reported last year [1].
This is a updated version reflecting some advices from Jim-san [2] and Eric-san [3].
[1] https://www.redhat.com/archives/libvir-list/2009-October/msg00216.html [2] https://www.redhat.com/archives/libvir-list/2010-April/msg00849.html [3] https://www.redhat.com/archives/libvir-list/2010-April/msg00850.html
Hi Satoru,
looking at the patches, those looks fine to me, I may have just a couple of cosmetic comments, but I'm wondering if they should be postponed after 0.8.1, or if it's fine to push them in now. On one hand I would prefer to limit the number of actual features in 0.8.1, but on the other hand you have already submitted this many time so I wonder what you think. When you say "prototype implementation" how confident are you about this code ?
So what do you think ?
As far as I tested, it works as expected and not aware of any critical issues. So if you're ok, I want to get it merge in. - satoru
On Sat, Apr 24, 2010 at 04:46:33AM +0900, Satoru SATOH wrote:
On Fri, Apr 23, 2010 at 09:33:20AM +0200, Daniel Veillard wrote:
looking at the patches, those looks fine to me, I may have just a couple of cosmetic comments, but I'm wondering if they should be postponed after 0.8.1, or if it's fine to push them in now. On one hand I would prefer to limit the number of actual features in 0.8.1, but on the other hand you have already submitted this many time so I wonder what you think. When you say "prototype implementation" how confident are you about this code ?
So what do you think ?
As far as I tested, it works as expected and not aware of any critical issues. So if you're ok, I want to get it merge in.
Okay, after Jim's thorough review and doing a bit of testing I commited your patch. I changed only a couple of things: - the dead store pointed out by Jim in his last review - the DNSMASQ_STATE_DIR, the host files are managed by libvirt, and even if they are used by dnsmasq, they really need to be stored in a directory owned and managed by libvirt. So I fixed DNSMASQ_STATE_DIR, to be LOCAL_STATE_DIR "/lib/libvirt/network", in practice the same as NETWORK_STATE_DIR. I think its fine because the naming of the files can't clash since they use different suffixes. I tested this on my own boxes and this seems to work as expected, there is just one small bit, assuming one stops a network like in, virsh net-destroy default, the file /var/lib/libvirt/network/default.hostsfile remains while it should really be removed. But it's not a big deal at this point, but I would like to get a fix for this :-) thanks ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On Mon, Apr 26, 2010 at 05:29:02PM +0200, Daniel Veillard wrote:
On Sat, Apr 24, 2010 at 04:46:33AM +0900, Satoru SATOH wrote:
As far as I tested, it works as expected and not aware of any critical issues. So if you're ok, I want to get it merge in.
Okay, after Jim's thorough review and doing a bit of testing I commited your patch. I changed only a couple of things: - the dead store pointed out by Jim in his last review - the DNSMASQ_STATE_DIR, the host files are managed by libvirt, and even if they are used by dnsmasq, they really need to be stored in a directory owned and managed by libvirt. So I fixed DNSMASQ_STATE_DIR, to be LOCAL_STATE_DIR "/lib/libvirt/network", in practice the same as NETWORK_STATE_DIR. I think its fine because the naming of the files can't clash since they use different suffixes.
I tested this on my own boxes and this seems to work as expected, there is just one small bit, assuming one stops a network like in, virsh net-destroy default, the file
/var/lib/libvirt/network/default.hostsfile
remains while it should really be removed. But it's not a big deal at this point, but I would like to get a fix for this :-)
Actually I noticed an error in my testing, while dnsmasq was started with the correct attribute it failed to read the host config file due to permission problems. The patch below uses instead /var/lib/libvirt/dnsmasq directory, created for this purpose and world readable allowing dnsmasq access. Daniel --------------------- Move dnsmasq host file to a separate directory use /var/lib/libvirt/dnsmasq since /var/lib/libvirt/network is unreadable by the dnsmasq binary * src/network/bridge_driver.c: update DNSMASQ_STATE_DIR * src/Makefile.am: create it on make install * libvirt.spec.in: take the new directory into account diff --git a/libvirt.spec.in b/libvirt.spec.in index a8b078a..090f5ee 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -566,6 +566,7 @@ rm -f $RPM_BUILD_ROOT%{_libdir}/python*/site-packages/*.la rm -f $RPM_BUILD_ROOT%{_libdir}/python*/site-packages/*.a %if %{with_network} +install -d -m 0755 $RPM_BUILD_ROOT%{_datadir}/lib/libvirt/dnsmasq/ # We don't want to install /etc/libvirt/qemu/networks in the main %files list # because if the admin wants to delete the default network completely, we don't # want to end up re-incarnating it on every RPM upgrade. @@ -742,6 +743,7 @@ fi %if %{with_network} %dir %{_localstatedir}/run/libvirt/network/ %dir %attr(0700, root, root) %{_localstatedir}/lib/libvirt/network/ +%dir %attr(0755, root, root) %{_localstatedir}/lib/libvirt/dnsmasq/ %endif %if %{with_qemu} diff --git a/src/Makefile.am b/src/Makefile.am index fc64927..d8466f0 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -1035,6 +1035,7 @@ if WITH_UML endif if WITH_NETWORK $(MKDIR_P) "$(DESTDIR)$(localstatedir)/lib/libvirt/network" + $(MKDIR_P) "$(DESTDIR)$(localstatedir)/lib/libvirt/dnsmasq" $(MKDIR_P) "$(DESTDIR)$(localstatedir)/run/libvirt/network" $(MKDIR_P) "$(DESTDIR)$(sysconfdir)/libvirt/qemu/networks/autostart" $(INSTALL_DATA) $(srcdir)/network/default.xml \ diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 22b3927..132392b 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -62,7 +62,7 @@ #define NETWORK_PID_DIR LOCAL_STATE_DIR "/run/libvirt/network" #define NETWORK_STATE_DIR LOCAL_STATE_DIR "/lib/libvirt/network" -#define DNSMASQ_STATE_DIR LOCAL_STATE_DIR "/lib/libvirt/network" +#define DNSMASQ_STATE_DIR LOCAL_STATE_DIR "/lib/libvirt/dnsmasq" #define VIR_FROM_THIS VIR_FROM_NETWORK -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On 04/27/2010 07:51 AM, Daniel Veillard wrote:
Move dnsmasq host file to a separate directory
use /var/lib/libvirt/dnsmasq since /var/lib/libvirt/network is unreadable by the dnsmasq binary
* src/network/bridge_driver.c: update DNSMASQ_STATE_DIR * src/Makefile.am: create it on make install * libvirt.spec.in: take the new directory into account
ACK. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Tue, Apr 27, 2010 at 08:19:30AM -0600, Eric Blake wrote:
On 04/27/2010 07:51 AM, Daniel Veillard wrote:
Move dnsmasq host file to a separate directory
use /var/lib/libvirt/dnsmasq since /var/lib/libvirt/network is unreadable by the dnsmasq binary
* src/network/bridge_driver.c: update DNSMASQ_STATE_DIR * src/Makefile.am: create it on make install * libvirt.spec.in: take the new directory into account
ACK.
thanks, pushed ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
participants (4)
-
Daniel Veillard -
Eric Blake -
Jim Meyering -
Satoru SATOH