[PATCH 4/6] vg mempool: fix archiver code

[PATCH 4/6] vg mempool: fix archiver code

[Milan Broz]

Properly release VG memory pool in archiver code.

Signed-off-by: Milan Broz <mbroz@redhat.com>
---
 lib/format_text/archive.c  |    2 +-
 lib/format_text/archiver.c |   13 +++++++++----
 2 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/lib/format_text/archive.c b/lib/format_text/archive.c
index 3e5b0c4..0077f14 100644
--- a/lib/format_text/archive.c
+++ b/lib/format_text/archive.c
@@ -326,7 +326,7 @@ static void _display_archive(struct cmd_context *cmd, struct archive_file *af)
 	log_print("Description:\t%s", desc ? : "<No description>");
 	log_print("Backup Time:\t%s", ctime(&when));
 
-	dm_pool_free(cmd->mem, vg);
+	vg_release(vg);
 	tf->fmt->ops->destroy_instance(tf);
 }
 
diff --git a/lib/format_text/archiver.c b/lib/format_text/archiver.c
index e221500..61d2441 100644
--- a/lib/format_text/archiver.c
+++ b/lib/format_text/archiver.c
@@ -326,6 +326,7 @@ int backup_restore_from_file(struct cmd_context *cmd, const char *vg_name,
 			     const char *file)
 {
 	struct volume_group *vg;
+	int r = 0;
 
 	/*
 	 * Read in the volume group from the text file.
@@ -336,10 +337,11 @@ int backup_restore_from_file(struct cmd_context *cmd, const char *vg_name,
 	/*
 	 * If PV is missing, there is already message from read above
 	 */
-	if (vg_missing_pv_count(vg))
-		return_0;
+	if (!vg_missing_pv_count(vg))
+		r = backup_restore_vg(cmd, vg);
 
-	return backup_restore_vg(cmd, vg);
+	vg_release(vg);
+	return r;
 }
 
 int backup_restore(struct cmd_context *cmd, const char *vg_name)
@@ -414,12 +416,15 @@ void check_current_backup(struct volume_group *vg)
 	    (vg->seqno == vg_backup->seqno) &&
 	    (id_equal(&vg->id, &vg_backup->id))) {
 		log_suppress(old_suppress);
+		vg_release(vg_backup);
 		return;
 	}
 	log_suppress(old_suppress);
 
-	if (vg_backup)
+	if (vg_backup) {
 		archive(vg_backup);
+		vg_release(vg_backup);
+	}
 	archive(vg);
 	backup(vg);
 }

[PATCH 4/6] vg mempool: fix archiver code

[Petr Rockai]

Milan Broz <mbroz@redhat.com> writes:
> Properly release VG memory pool in archiver code.
>
> Signed-off-by: Milan Broz <mbroz@redhat.com>
Acked-By: Petr Ro?kai <prockai@redhat.com>

Please also see inline comments below.

> diff --git a/lib/format_text/archive.c b/lib/format_text/archive.c
> index 3e5b0c4..0077f14 100644
> --- a/lib/format_text/archive.c
> +++ b/lib/format_text/archive.c
> @@ -326,7 +326,7 @@ static void _display_archive(struct cmd_context *cmd, struct archive_file *af)
>  	log_print("Description:\t%s", desc ? : "<No description>");
>  	log_print("Backup Time:\t%s", ctime(&when));
>  
> -	dm_pool_free(cmd->mem, vg);
> +	vg_release(vg);
Check. Actually looks much more robust. While looking through the code, I have
noticed that there is likely still a leak in there though, since
create_text_context will call dm_pool_alloc/dm_pool_strdup a few times. If I
understand dm_pool_free correctly, this only frees the part of the pool from
the "vg" pointer till end of the pool, so the leak has been there even
before. You might want to fix that by calling dm_pool_free(cmd->mem, context)
besides the vg_release. (This all needs double-checking though.)

>  	tf->fmt->ops->destroy_instance(tf);
>  }
>  
> diff --git a/lib/format_text/archiver.c b/lib/format_text/archiver.c
> index e221500..61d2441 100644
> --- a/lib/format_text/archiver.c
> +++ b/lib/format_text/archiver.c
> @@ -326,6 +326,7 @@ int backup_restore_from_file(struct cmd_context *cmd, const char *vg_name,
>  			     const char *file)
>  {
>  	struct volume_group *vg;
> +	int r = 0;
>  
>  	/*
>  	 * Read in the volume group from the text file.
> @@ -336,10 +337,11 @@ int backup_restore_from_file(struct cmd_context *cmd, const char *vg_name,
>  	/*
>  	 * If PV is missing, there is already message from read above
>  	 */
> -	if (vg_missing_pv_count(vg))
> -		return_0;
> +	if (!vg_missing_pv_count(vg))
> +		r = backup_restore_vg(cmd, vg);
>  
> -	return backup_restore_vg(cmd, vg);
> +	vg_release(vg);
> +	return r;
Check.

>  }
>  
>  int backup_restore(struct cmd_context *cmd, const char *vg_name)
> @@ -414,12 +416,15 @@ void check_current_backup(struct volume_group *vg)
>  	    (vg->seqno == vg_backup->seqno) &&
>  	    (id_equal(&vg->id, &vg_backup->id))) {
>  		log_suppress(old_suppress);
> +		vg_release(vg_backup);
>  		return;
>  	}
>  	log_suppress(old_suppress);
>  
> -	if (vg_backup)
> +	if (vg_backup) {
>  		archive(vg_backup);
> +		vg_release(vg_backup);
> +	}
>  	archive(vg);
>  	backup(vg);
>  }
Check.

-- 
Peter Rockai | me()mornfall!net | prockai()redhat!com
 http://blog.mornfall.net | http://web.mornfall.net

"In My Egotistical Opinion, most people's C programs should be
 indented six feet downward and covered with dirt."
     -- Blair P. Houghton on the subject of C program indentation

[PATCH 4/6] vg mempool: fix archiver code

[Milan Broz]

Petr Rockai wrote:
> Milan Broz <mbroz@redhat.com> writes:
>> +	vg_release(vg);
> Check. Actually looks much more robust. While looking through the code, I have
> noticed that there is likely still a leak in there though, since
> create_text_context will call dm_pool_alloc/dm_pool_strdup a few times. If I
> understand dm_pool_free correctly, this only frees the part of the pool from
> the "vg" pointer till end of the pool, so the leak has been there even
> before. You might want to fix that by calling dm_pool_free(cmd->mem, context)
> besides the vg_release. (This all needs double-checking though.)

There are more such places in code for sure, some structures are even allocated
from global mempool still. (I have at least one more patch in my queue).

But this can be fixed in subsequent patches - my main intention was to put
vg_release to correct places in code (otherwise it will leak the whole pool).

Milan