Re: [PATCH V3 04/11] Show all bitmaps while examining bitmap

[NeilBrown <neilb@suse.de>]

[-- Attachment #1: Type: text/plain, Size: 4817 bytes --]

On Wed, 20 May 2015 11:20:36 +0800 Guoqing Jiang <gqjiang@suse.com> wrote:

> This adds capability of exmining bitmaps corresponding to all
> nodes/slots on the device.
> 
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> Signed-off-by: Guoqing Jiang <gqjiang@suse.com>
> ---
>  bitmap.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++----------------
>  1 file changed, 54 insertions(+), 18 deletions(-)
> 
> diff --git a/bitmap.c b/bitmap.c
> index 920033a..bccc67c 100644
> --- a/bitmap.c
> +++ b/bitmap.c
> @@ -260,7 +260,7 @@ int ExamineBitmap(char *filename, int brief, struct supertype *st)
>  	int rv = 1;
>  	char buf[64];
>  	int swap;
> -	int fd;
> +	int fd, i;
>  	__u32 uuid32[4];
>  
>  	fd = bitmap_file_open(filename, &st);
> @@ -317,23 +317,59 @@ int ExamineBitmap(char *filename, int brief, struct supertype *st)
>  		       uuid32[2],
>  		       uuid32[3]);
>  
> -	printf("          Events : %llu\n", (unsigned long long)sb->events);
> -	printf("  Events Cleared : %llu\n", (unsigned long long)sb->events_cleared);
> -	printf("           State : %s\n", bitmap_state(sb->state));
> -	printf("       Chunksize : %s\n", human_chunksize(sb->chunksize));
> -	printf("          Daemon : %ds flush period\n", sb->daemon_sleep);
> -	if (sb->write_behind)
> -		sprintf(buf, "Allow write behind, max %d", sb->write_behind);
> -	else
> -		sprintf(buf, "Normal");
> -	printf("      Write Mode : %s\n", buf);
> -	printf("       Sync Size : %llu%s\n", (unsigned long long)sb->sync_size/2,
> -					human_size(sb->sync_size * 512));
> -	if (brief)
> -		goto free_info;
> -	printf("          Bitmap : %llu bits (chunks), %llu dirty (%2.1f%%)\n",
> -			info->total_bits, info->dirty_bits,
> -			100.0 * info->dirty_bits / (info->total_bits?:1));
> +	if (sb->nodes == 0) {
> +		printf("          Events : %llu\n", (unsigned long long)sb->events);
> +		printf("  Events Cleared : %llu\n", (unsigned long long)sb->events_cleared);
> +		printf("           State : %s\n", bitmap_state(sb->state));
> +		printf("       Chunksize : %s\n", human_chunksize(sb->chunksize));
> +		printf("          Daemon : %ds flush period\n", sb->daemon_sleep);
> +		if (sb->write_behind)
> +			sprintf(buf, "Allow write behind, max %d", sb->write_behind);
> +		else
> +			sprintf(buf, "Normal");
> +		printf("      Write Mode : %s\n", buf);
> +		printf("       Sync Size : %llu%s\n", (unsigned long long)sb->sync_size/2,
> +						human_size(sb->sync_size * 512));
> +		if (brief)
> +			goto free_info;
> +		printf("          Bitmap : %llu bits (chunks), %llu dirty (%2.1f%%)\n",
> +				info->total_bits, info->dirty_bits,
> +				100.0 * info->dirty_bits / (info->total_bits?:1));
> +	} else {
> +		printf("       Chunksize : %s\n", human_chunksize(sb->chunksize));
> +		printf("          Daemon : %ds flush period\n", sb->daemon_sleep);
> +		if (sb->write_behind)
> +			sprintf(buf, "Allow write behind, max %d", sb->write_behind);
> +		else
> +			sprintf(buf, "Normal");
> +		printf("      Write Mode : %s\n", buf);
> +		printf("       Sync Size : %llu%s\n", (unsigned long long)sb->sync_size/2,
> +						human_size(sb->sync_size * 512));

This is too much duplication.  Duplicate code leads to maintenance errors.

if (sb->nodes == 0)
    print Events/ Events cleared / state

print chunksuze, daemon flush period, write mode sync size

if (sb->nodes > 0) {
   print Events/ Events cleared etc for each bitmap 
}


> +		printf("   Cluster nodes : %d\n", sb->nodes);
> +		printf("    Cluster name : %s\n", sb->cluster_name);

sb->cluster_name is not nul terminated if it is exactly 64 bytes long.
You need "%64s" ... and you need to test that case.


> +		i = 0;
> +		do {

Please make this a regular
   for(i = 0; i < nodes; i++)
loop.  do/while should  only be used when some non-trivial computation is
needed before the condition can be tested.


> +			if (i) {
> +				free(info);
> +				info = bitmap_fd_read(fd, brief);
> +				sb = &info->sb;
> +			}
> +			if (sb->magic != BITMAP_MAGIC)
> +				pr_err("invalid bitmap magic 0x%x, the bitmap file appears to be corrupted\n", sb->magic);
> +
> +			printf("       Node Slot : %d\n", i);
> +			printf("          Events : %llu\n", (unsigned long long)sb->events);
> +			printf("  Events Cleared : %llu\n", (unsigned long long)sb->events_cleared);
> +			printf("           State : %s\n", bitmap_state(sb->state));
> +			if (brief)
> +				continue;
> +			printf("          Bitmap : %llu bits (chunks), %llu dirty (%2.1f%%)\n",
> +					info->total_bits, info->dirty_bits,
> +					100.0 * info->dirty_bits / (info->total_bits?:1));
> +
> +		} while (++i < (int)sb->nodes);
> +	}
> +
>  free_info:
>  	free(info);
>  	return rv;

Thanks,
NeilBrown


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]