[PATCH v2] makedumpfile: code cleanup: set_bitmap

[Petr Tesarik]

On Thu, 24 Apr 2014 17:11:13 +0800
Wang Nan <wangnan0 at huawei.com> wrote:

> This patch makes set_bitmap() to call sync_bitmap() instead rewrite
> identical code to do same thing.
> 
> Change from v1:
> 
> - fix a simple mistake:
>   sync_bitmap() returns TRUE(1) when it succeeds, so use
>   (!sync_bitmap()) for checking.

Hi Wang Nan,

I like your change. See my comments below:

> Signed-off-by: Wang Nan <wangnan0 at huawei.com>
> Cc: Atsushi Kumagai <kumagai-atsushi at mxc.nes.nec.co.jp>
> Cc: kexec at lists.infradead.org
> Cc: Geng Hui <hui.geng at huawei.com>
> ---
>  makedumpfile.c | 70 ++++++++++++++++++++++++++--------------------------------
>  1 file changed, 31 insertions(+), 39 deletions(-)
> 
> diff --git a/makedumpfile.c b/makedumpfile.c
> index ce4a866..cea37a2 100644
> --- a/makedumpfile.c
> +++ b/makedumpfile.c
> @@ -3309,6 +3309,34 @@ initialize_2nd_bitmap(struct dump_bitmap *bitmap)
>  }
>  
>  int
> +sync_bitmap(struct dump_bitmap *bitmap)
> +{
> +	off_t offset;
> +	offset = bitmap->offset + BUFSIZE_BITMAP * bitmap->no_block;
> +
> +	/*
> +	 * The bitmap buffer is not dirty, and it is not necessary
> +	 * to write out it.
> +	 */
> +	if (bitmap->no_block < 0)
> +		return TRUE;
> +
> +	if (lseek(bitmap->fd, offset, SEEK_SET) < 0 ) {
> +		ERRMSG("Can't seek the bitmap(%s). %s\n",
> +		    bitmap->file_name, strerror(errno));
> +		return FALSE;
> +	}
> +	if (write(bitmap->fd, bitmap->buf, BUFSIZE_BITMAP)
> +	    != BUFSIZE_BITMAP) {
> +		ERRMSG("Can't write the bitmap(%s). %s\n",
> +		    bitmap->file_name, strerror(errno));
> +		return FALSE;
> +	}
> +	return TRUE;
> +}
> +
> +
> +int
>  set_bitmap(struct dump_bitmap *bitmap, unsigned long long pfn,
>      int val)
>  {
> @@ -3317,20 +3345,11 @@ set_bitmap(struct dump_bitmap *bitmap, unsigned long long pfn,
>  	old_offset = bitmap->offset + BUFSIZE_BITMAP * bitmap->no_block;
>  	new_offset = bitmap->offset + BUFSIZE_BITMAP * (pfn / PFN_BUFBITMAP);

After applying this patch, both old_offset and new_offset are
calculated only to compare for equality. And new_offset is in fact
computed twice (once in set_bitmap and once in sync_bitmap).

This could be cleaned up by replacing the offsets with:

	int new_no_block = pfn / PFN_BUFBITMAP;

Then change all occurrences of if (old_offset != new_offset) to:

	if (bitmap->no_block != new_no_block)

and finally re-use new_no_block in the assignment to bitmap->no_block
near the end of the function, like this:

-		bitmap->no_block = pfn / PFN_BUFBITMAP;
+		bitmap->no_block = new_no_block;

Petr T

>  
> -	if (0 <= bitmap->no_block && old_offset != new_offset) {
> -		if (lseek(bitmap->fd, old_offset, SEEK_SET) < 0 ) {
> -			ERRMSG("Can't seek the bitmap(%s). %s\n",
> -			    bitmap->file_name, strerror(errno));
> -			return FALSE;
> -		}
> -		if (write(bitmap->fd, bitmap->buf, BUFSIZE_BITMAP)
> -		    != BUFSIZE_BITMAP) {
> -			ERRMSG("Can't write the bitmap(%s). %s\n",
> -			    bitmap->file_name, strerror(errno));
> +	if (old_offset != new_offset) {
> +		if (!sync_bitmap(bitmap)) {
> +			ERRMSG("Can't sync bitmap\n");
>  			return FALSE;
>  		}
> -	}
> -	if (old_offset != new_offset) {
>  		if (lseek(bitmap->fd, new_offset, SEEK_SET) < 0 ) {
>  			ERRMSG("Can't seek the bitmap(%s). %s\n",
>  			    bitmap->file_name, strerror(errno));
> @@ -3386,33 +3405,6 @@ set_bitmap_cyclic(char *bitmap, unsigned long long pfn, int val, struct cycle *c
>  }
>  
>  int
> -sync_bitmap(struct dump_bitmap *bitmap)
> -{
> -	off_t offset;
> -	offset = bitmap->offset + BUFSIZE_BITMAP * bitmap->no_block;
> -
> -	/*
> -	 * The bitmap buffer is not dirty, and it is not necessary
> -	 * to write out it.
> -	 */
> -	if (bitmap->no_block < 0)
> -		return TRUE;
> -
> -	if (lseek(bitmap->fd, offset, SEEK_SET) < 0 ) {
> -		ERRMSG("Can't seek the bitmap(%s). %s\n",
> -		    bitmap->file_name, strerror(errno));
> -		return FALSE;
> -	}
> -	if (write(bitmap->fd, bitmap->buf, BUFSIZE_BITMAP)
> -	    != BUFSIZE_BITMAP) {
> -		ERRMSG("Can't write the bitmap(%s). %s\n",
> -		    bitmap->file_name, strerror(errno));
> -		return FALSE;
> -	}
> -	return TRUE;
> -}
> -
> -int
>  sync_1st_bitmap(void)
>  {
>  	return sync_bitmap(info->bitmap1);