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

Re: [libvirt] [PATCH] xm_internal.c: remove dead store and associated leak



On Mon, Sep 07, 2009 at 05:41:18PM +0200, Jim Meyering wrote:
> This one took a little thinking.
> 
> I'm not 100% sure that the comment in my log is sufficient
> justification for removing the virGetDomain call.  If we leave it,
> we'll have to free it and remove the XXX comment.
> 
> >From 324e07f17e6a2ed6df236af955adc693670d1b16 Mon Sep 17 00:00:00 2001
> From: Jim Meyering <meyering redhat com>
> Date: Fri, 4 Sep 2009 19:46:59 +0200
> Subject: [PATCH] xm_internal.c: remove dead store and associated leak
> 
> * src/xm_internal.c (xenXMDomainDefineXML): Dead store to "olddomain" --
> and comment ;-) highlighted that the virGetDomain call represented a
> leak.  It was also useless, seeing as how preceding call to
> virHashLookup(priv->nameConfigMap found def->name.
> ---
>  src/xm_internal.c |    5 -----
>  1 files changed, 0 insertions(+), 5 deletions(-)
> 
> diff --git a/src/xm_internal.c b/src/xm_internal.c
> index de3aca9..e7f6a55 100644
> --- a/src/xm_internal.c
> +++ b/src/xm_internal.c
> @@ -2532,7 +2532,6 @@ cleanup:
>   */
>  virDomainPtr xenXMDomainDefineXML(virConnectPtr conn, const char *xml) {
>      virDomainPtr ret;
> -    virDomainPtr olddomain;
>      char filename[PATH_MAX];
>      const char * oldfilename;
>      virDomainDefPtr def = NULL;
> @@ -2578,10 +2577,6 @@ virDomainPtr xenXMDomainDefineXML(virConnectPtr conn, const char *xml) {
>              goto error;
>          }
> 
> -        /* XXX wtf.com is this line for - it appears to be amemory leak */
> -        if (!(olddomain = virGetDomain(conn, def->name, entry->def->uuid)))
> -            goto error;
> -
>          /* Remove the name -> filename mapping */
>          if (virHashRemoveEntry(priv->nameConfigMap, def->name, NULL) < 0) {
>              xenXMError(conn, VIR_ERR_INTERNAL_ERROR,

  Hum .... not so fast :-)

so we are defining a domain, it's in the config map, but if we try to
retrieve it it doesn't show up ...
I wonder if it wasn't a check to preserve files in /etc/xen which
were not domain descriptions, so that someone would not override
a non-domain config file.

  Historical, and nasty, I would instead call virUnrefDomain(olddomain);
just after (assuming the virGetDomain didn't find NULL).

  But that would need testing, I guess :-\

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]