Re: [libvirt] [PATCH 1/4] Make avahi startup more robust.

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 !