[libvirt] [PATCH 1/4] Make avahi startup more robust.
If the hostname of the current virtualization machine could not be resolved, then libvirtd would fail to start. However, for disconnected operation (on a laptop, for instance) the hostname may very legitimately not be resolvable. This patch makes it so that if we can't resolve the hostname, avahi doesn't fail, it just uses a less useful MDNS string. Signed-off-by: Chris Lalancette <clalance@redhat.com> --- daemon/libvirtd.c | 19 +++++++++++++------ 1 files changed, 13 insertions(+), 6 deletions(-) diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index 863bf21..69106ee 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -998,22 +998,29 @@ static int qemudNetworkInit(struct qemud_server *server) { struct libvirtd_mdns_group *group; struct qemud_socket *sock; int port = 0; + int ret; server->mdns = libvirtd_mdns_new(); if (!mdns_name) { - char groupname[64], *localhost, *tmp; + char *groupname, *localhost, *tmp; /* Extract the host part of the potentially FQDN */ localhost = virGetHostname(NULL); if (localhost == NULL) + ret = virAsprintf(&groupname, "Virtualization Host"); + else { + if ((tmp = strchr(localhost, '.'))) + *tmp = '\0'; + ret = virAsprintf(&groupname, "Virtualization Host %s", + localhost); + } + if (ret < 0) { + virReportOOMError(); goto cleanup; - - if ((tmp = strchr(localhost, '.'))) - *tmp = '\0'; - snprintf(groupname, sizeof(groupname)-1, "Virtualization Host %s", localhost); - groupname[sizeof(groupname)-1] = '\0'; + } group = libvirtd_mdns_add_group(server->mdns, groupname); VIR_FREE(localhost); + VIR_FREE(groupname); } else { group = libvirtd_mdns_add_group(server->mdns, mdns_name); } -- 1.6.6.1
Grr. Please ignore the subject line. This is not PATCH 1/4, it's a standalone patch. I screwed up when using git format-patch. Chris Lalancette On 04/21/2010 12:03 PM, Chris Lalancette wrote:
If the hostname of the current virtualization machine could not be resolved, then libvirtd would fail to start. However, for disconnected operation (on a laptop, for instance) the hostname may very legitimately not be resolvable. This patch makes it so that if we can't resolve the hostname, avahi doesn't fail, it just uses a less useful MDNS string.
Signed-off-by: Chris Lalancette <clalance@redhat.com> --- daemon/libvirtd.c | 19 +++++++++++++------ 1 files changed, 13 insertions(+), 6 deletions(-)
diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index 863bf21..69106ee 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -998,22 +998,29 @@ static int qemudNetworkInit(struct qemud_server *server) { struct libvirtd_mdns_group *group; struct qemud_socket *sock; int port = 0; + int ret;
server->mdns = libvirtd_mdns_new();
if (!mdns_name) { - char groupname[64], *localhost, *tmp; + char *groupname, *localhost, *tmp; /* Extract the host part of the potentially FQDN */ localhost = virGetHostname(NULL); if (localhost == NULL) + ret = virAsprintf(&groupname, "Virtualization Host"); + else { + if ((tmp = strchr(localhost, '.'))) + *tmp = '\0'; + ret = virAsprintf(&groupname, "Virtualization Host %s", + localhost); + } + if (ret < 0) { + virReportOOMError(); goto cleanup; - - if ((tmp = strchr(localhost, '.'))) - *tmp = '\0'; - snprintf(groupname, sizeof(groupname)-1, "Virtualization Host %s", localhost); - groupname[sizeof(groupname)-1] = '\0'; + } group = libvirtd_mdns_add_group(server->mdns, groupname); VIR_FREE(localhost); + VIR_FREE(groupname); } else { group = libvirtd_mdns_add_group(server->mdns, mdns_name); }
On 04/21/2010 10:03 AM, Chris Lalancette wrote:
If the hostname of the current virtualization machine could not be resolved, then libvirtd would fail to start. However, for disconnected operation (on a laptop, for instance) the hostname may very legitimately not be resolvable. This patch makes it so that if we can't resolve the hostname, avahi doesn't fail, it just uses a less useful MDNS string.
ACK on the concept, but fix the corner-case memory leak before pushing.
if (!mdns_name) { - char groupname[64], *localhost, *tmp; + char *groupname, *localhost, *tmp; /* Extract the host part of the potentially FQDN */ localhost = virGetHostname(NULL);
Here, localhost can be allocated...
if (localhost == NULL) + ret = virAsprintf(&groupname, "Virtualization Host"); + else { + if ((tmp = strchr(localhost, '.'))) + *tmp = '\0'; + ret = virAsprintf(&groupname, "Virtualization Host %s", + localhost);
then groupname fails...
+ } + if (ret < 0) { + virReportOOMError(); goto cleanup;
...and we leak localhost.
- - if ((tmp = strchr(localhost, '.'))) - *tmp = '\0'; - snprintf(groupname, sizeof(groupname)-1, "Virtualization Host %s", localhost); - groupname[sizeof(groupname)-1] = '\0'; + } group = libvirtd_mdns_add_group(server->mdns, groupname); VIR_FREE(localhost); + VIR_FREE(groupname);
But on success, there is no leak. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 04/21/2010 12:25 PM, Eric Blake wrote:
On 04/21/2010 10:03 AM, Chris Lalancette wrote:
If the hostname of the current virtualization machine could not be resolved, then libvirtd would fail to start. However, for disconnected operation (on a laptop, for instance) the hostname may very legitimately not be resolvable. This patch makes it so that if we can't resolve the hostname, avahi doesn't fail, it just uses a less useful MDNS string.
ACK on the concept, but fix the corner-case memory leak before pushing.
if (!mdns_name) { - char groupname[64], *localhost, *tmp; + char *groupname, *localhost, *tmp; /* Extract the host part of the potentially FQDN */ localhost = virGetHostname(NULL);
Here, localhost can be allocated...
if (localhost == NULL) + ret = virAsprintf(&groupname, "Virtualization Host"); + else { + if ((tmp = strchr(localhost, '.'))) + *tmp = '\0'; + ret = virAsprintf(&groupname, "Virtualization Host %s", + localhost);
then groupname fails...
+ } + if (ret < 0) { + virReportOOMError(); goto cleanup;
...and we leak localhost.
- - if ((tmp = strchr(localhost, '.'))) - *tmp = '\0'; - snprintf(groupname, sizeof(groupname)-1, "Virtualization Host %s", localhost); - groupname[sizeof(groupname)-1] = '\0'; + } group = libvirtd_mdns_add_group(server->mdns, groupname); VIR_FREE(localhost); + VIR_FREE(groupname);
But on success, there is no leak.
Oops, good call. I'll respin the patch with that change. -- Chris Lalancette
On Wed, Apr 21, 2010 at 02:50:18PM -0400, Chris Lalancette wrote:
On 04/21/2010 12:25 PM, Eric Blake wrote:
On 04/21/2010 10:03 AM, Chris Lalancette wrote:
If the hostname of the current virtualization machine could not be resolved, then libvirtd would fail to start. However, for disconnected operation (on a laptop, for instance) the hostname may very legitimately not be resolvable. This patch makes it so that if we can't resolve the hostname, avahi doesn't fail, it just uses a less useful MDNS string.
ACK on the concept, but fix the corner-case memory leak before pushing.
if (!mdns_name) { - char groupname[64], *localhost, *tmp; + char *groupname, *localhost, *tmp; /* Extract the host part of the potentially FQDN */ localhost = virGetHostname(NULL);
Here, localhost can be allocated...
if (localhost == NULL) + ret = virAsprintf(&groupname, "Virtualization Host"); + else { + if ((tmp = strchr(localhost, '.'))) + *tmp = '\0'; + ret = virAsprintf(&groupname, "Virtualization Host %s", + localhost);
then groupname fails...
+ } + if (ret < 0) { + virReportOOMError(); goto cleanup;
...and we leak localhost.
- - if ((tmp = strchr(localhost, '.'))) - *tmp = '\0'; - snprintf(groupname, sizeof(groupname)-1, "Virtualization Host %s", localhost); - groupname[sizeof(groupname)-1] = '\0'; + } group = libvirtd_mdns_add_group(server->mdns, groupname); VIR_FREE(localhost); + VIR_FREE(groupname);
But on success, there is no leak.
Oops, good call. I'll respin the patch with that change.
Okay, but this is an important patch, I tested this too, without it if one cannot resolve the hostname at boot time, libvirtd service fails to start, while it properly starts with that patch applied. I tested it myself on one of my boxes where I had the trouble. So ACK once the leak is fixed ! 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 04/21/2010 04:17 PM, Daniel Veillard wrote:
On Wed, Apr 21, 2010 at 02:50:18PM -0400, Chris Lalancette wrote:
On 04/21/2010 12:25 PM, Eric Blake wrote:
On 04/21/2010 10:03 AM, Chris Lalancette wrote:
If the hostname of the current virtualization machine could not be resolved, then libvirtd would fail to start. However, for disconnected operation (on a laptop, for instance) the hostname may very legitimately not be resolvable. This patch makes it so that if we can't resolve the hostname, avahi doesn't fail, it just uses a less useful MDNS string.
ACK on the concept, but fix the corner-case memory leak before pushing.
if (!mdns_name) { - char groupname[64], *localhost, *tmp; + char *groupname, *localhost, *tmp; /* Extract the host part of the potentially FQDN */ localhost = virGetHostname(NULL);
Here, localhost can be allocated...
if (localhost == NULL) + ret = virAsprintf(&groupname, "Virtualization Host"); + else { + if ((tmp = strchr(localhost, '.'))) + *tmp = '\0'; + ret = virAsprintf(&groupname, "Virtualization Host %s", + localhost);
then groupname fails...
+ } + if (ret < 0) { + virReportOOMError(); goto cleanup;
...and we leak localhost.
- - if ((tmp = strchr(localhost, '.'))) - *tmp = '\0'; - snprintf(groupname, sizeof(groupname)-1, "Virtualization Host %s", localhost); - groupname[sizeof(groupname)-1] = '\0'; + } group = libvirtd_mdns_add_group(server->mdns, groupname); VIR_FREE(localhost); + VIR_FREE(groupname);
But on success, there is no leak.
Oops, good call. I'll respin the patch with that change.
Okay, but this is an important patch, I tested this too, without it if one cannot resolve the hostname at boot time, libvirtd service fails to start, while it properly starts with that patch applied. I tested it myself on one of my boxes where I had the trouble.
So ACK once the leak is fixed !
I fixed the leak and pushed it. Thanks. -- Chris Lalancette
participants (3)
-
Chris Lalancette -
Daniel Veillard -
Eric Blake