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

Re: [libvirt] [PATCH] openvz: fixed two memory leaks on migration code



Rather sparse commit message.

On 16.09.2014 04:22, Hongbin Lu wrote:
> ---
>   src/openvz/openvz_driver.c |    8 ++++++--
>   1 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c
> index 57b3c22..3147311 100644
> --- a/src/openvz/openvz_driver.c
> +++ b/src/openvz/openvz_driver.c
> @@ -2364,7 +2364,10 @@ openvzDomainMigratePrepare3Params(virConnectPtr dconn,
>       }
>   
>    done:
> -    virURIFree(uri);
> +    if (!uri_in)
> +        VIR_FREE(hostname);
> +    else
> +        virURIFree(uri);
>       if (vm)
>           virObjectUnlock(vm);
>       return ret;

While this is technically correct, I find it hard to see at first glance why hostname can't be freed in case of uri_in != NULL. Moreover - and I should raised that in the previous review I did - hostname is once used as const char * while it may be used as char *. This is inconsistency that I don't like so I'm squashing this in:

diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c
index 3147311..6c73eaf 100644
--- a/src/openvz/openvz_driver.c
+++ b/src/openvz/openvz_driver.c
@@ -2286,7 +2286,8 @@ openvzDomainMigratePrepare3Params(virConnectPtr dconn,
     const char *uri_in = NULL;
     virDomainDefPtr def = NULL;
     virDomainObjPtr vm = NULL;
-    char *hostname = NULL;
+    char *my_hostname = NULL;
+    const char *hostname = NULL;
     virURIPtr uri = NULL;
     int ret = -1;
 
@@ -2321,10 +2322,10 @@ openvzDomainMigratePrepare3Params(virConnectPtr dconn,
     def = NULL;
 
     if (!uri_in) {
-        if ((hostname = virGetHostname()) == NULL)
+        if ((my_hostname = virGetHostname()) == NULL)
             goto error;
 
-        if (STRPREFIX(hostname, "localhost")) {
+        if (STRPREFIX(my_hostname, "localhost")) {
             virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
                            _("hostname on destination resolved to localhost,"
                              " but migration requires an FQDN"));
@@ -2364,10 +2365,8 @@ openvzDomainMigratePrepare3Params(virConnectPtr dconn,
     }
 
  done:
-    if (!uri_in)
-        VIR_FREE(hostname);
-    else
-        virURIFree(uri);
+    VIR_FREE(my_hostname);
+    virURIFree(uri);
     if (vm)
         virObjectUnlock(vm);
     return ret;

> @@ -2385,7 +2388,7 @@ openvzDomainMigratePerform3Params(virDomainPtr domain,
>       virDomainObjPtr vm = NULL;
>       const char *uri_str = NULL;
>       virURIPtr uri = NULL;
> -    virCommandPtr cmd = virCommandNew(VZMIGRATE);
> +    virCommandPtr cmd = NULL;
>       int ret = -1;
>   
>       virCheckFlags(OPENVZ_MIGRATION_FLAGS, -1);
> @@ -2412,6 +2415,7 @@ openvzDomainMigratePerform3Params(virDomainPtr domain,
>       if (uri == NULL || uri->server == NULL)
>           goto cleanup;
>   
> +    cmd = virCommandNew(VZMIGRATE);
>       if (flags & VIR_MIGRATE_LIVE)
>           virCommandAddArg(cmd, "--live");
>       virCommandAddArg(cmd, uri->server);
> 

ACKed, fixed & pushed. Thanks for nailing this down.

Michal


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