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

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 !

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/


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