11:03 a.m.
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(a)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
11:04 a.m.
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(a)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);
}
11:25 a.m.
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
1:50 p.m.
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
3:17 p.m.
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(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
3:17 p.m.
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
5329
days inactive
5329
days old
5 comments
3 participants
participants (3)
-
Chris Lalancette -
Daniel Veillard -
Eric Blake