[Cluster-devel] [PATCH] mkfs.gfs2: Add a progress indicator to mkfs.gfs2

[Andrew Price <anprice@redhat.com>]

Hi Paul,

Some comments below...

On 24/09/15 19:08, Paul Evans wrote:
> Add a progress indicator to libgfs2 based on an adapted version of
> the simple progress indicator used in e2fsprogs.
>
> Update mkfs.gfs2 to make use of this progress bar allowing progress
> to be reported during the creation of journals and resource groups
> during filesystem creation.
>
> Added unit tests into the test suite to validate progress bar
> functions.
>
> Signed-off-by: Paul Evans <pevans@redhat.com>
> ---
>   gfs2/libgfs2/Makefile.am |   3 +-
>   gfs2/libgfs2/libgfs2.h   |  12 ++++++
>   gfs2/libgfs2/progress.c  |  90 ++++++++++++++++++++++++++++++++++++++++
>   gfs2/mkfs/main_mkfs.c    |  31 ++++++++++++++
>   tests/Makefile.am        |  10 ++++-
>   tests/check_progress.c   | 104 +++++++++++++++++++++++++++++++++++++++++++++++
>   tests/libgfs2.at         |   4 ++
>   7 files changed, 252 insertions(+), 2 deletions(-)
>   create mode 100644 gfs2/libgfs2/progress.c
>   create mode 100644 tests/check_progress.c
>
> diff --git a/gfs2/libgfs2/Makefile.am b/gfs2/libgfs2/Makefile.am
> index 2b7aa16..25ae4f7 100644
> --- a/gfs2/libgfs2/Makefile.am
> +++ b/gfs2/libgfs2/Makefile.am
> @@ -42,7 +42,8 @@ libgfs2_la_SOURCES = \
>   	meta.c \
>   	lang.c \
>   	parser.y \
> -	lexer.l
> +	lexer.l \
> +	progress.c
>
>   libgfs2_la_CPPFLAGS = \
>   	-D_FILE_OFFSET_BITS=64 \
> diff --git a/gfs2/libgfs2/libgfs2.h b/gfs2/libgfs2/libgfs2.h
> index ccae721..a500b73 100644
> --- a/gfs2/libgfs2/libgfs2.h
> +++ b/gfs2/libgfs2/libgfs2.h
> @@ -791,6 +791,18 @@ extern int lgfs2_lang_result_print(struct lgfs2_lang_result *result);
>   extern void lgfs2_lang_result_free(struct lgfs2_lang_result **result);
>   extern void lgfs2_lang_free(struct lgfs2_lang_state **state);
>
> +/* Progress bar functions */
> +
> +struct lgfs2_progress_bar {
> +	uint64_t max;
> +	int max_digits;
> +	int skip_progress;
> +};
> +
> +extern void lgfs2_progress_init(struct lgfs2_progress_bar *progress, uint64_t max, const char *message, int quiet);
> +extern void lgfs2_progress_update(struct lgfs2_progress_bar *progress, uint64_t value);
> +extern void lgfs2_progress_close(struct lgfs2_progress_bar *progress, const char *message);
> +
>   __END_DECLS
>
>   #endif /* __LIBGFS2_DOT_H__ */

We shouldn't put user interface code in libgfs2, so this should all be 
moved into gfs2/mkfs/ really. The API looks nice and clean to me, though.

> diff --git a/gfs2/libgfs2/progress.c b/gfs2/libgfs2/progress.c
> new file mode 100644
> index 0000000..eb41fa5
> --- /dev/null
> +++ b/gfs2/libgfs2/progress.c
> @@ -0,0 +1,90 @@
> +/**
> + * Progress bar to give updates for operations in gfs2-utils.
> + * Adapted from the simple progress bar in e2fsprogs progress.c
> + *
> + */
> +
> +#include <time.h>
> +#include <string.h>
> +
> +#include "libgfs2.h"
> +
> +static char spaces[44], backspaces[44];
> +static time_t last_update;
> +
> +static int number_of_digits(int value)
> +{
> +	int digits = 0;
> +
> +	do {
> +		value /= 10;
> +		digits++;
> +	} while (value != 0);
> +
> +	return digits;
> +}
> +
> +void lgfs2_progress_init(struct lgfs2_progress_bar *progress, uint64_t max, const char *message, int quiet)
> +{
> +	/**
> +	 * NOTE:
> +	 *
> +	 * Default operation is to output the progress indication
> +	 * in full. Although we will honor the quiet flag in the
> +	 * application, if this is set we skip progress bar any
> +	 * update operations and output.
> +	 *
> +	 */
> +
> +	memset(spaces, ' ', sizeof(spaces)-1);
> +	spaces[sizeof(spaces)-1] = 0;
> +
> +	memset(backspaces, '\b', sizeof(backspaces)-1);
> +	backspaces[sizeof(backspaces)-1] = 0;
> +
> +	memset(progress, 0, sizeof(*progress));
> +
> +	if (quiet) {
> +		progress->skip_progress++;
> +		return;
> +	}
> +
> +	progress->max = max;
> +	progress->max_digits = number_of_digits(max);
> +
> +	if (message) {
> +		fputs(message, stdout);
> +		fflush(stdout);
> +	}
> +	last_update = 0;
> +}
> +
> +extern void lgfs2_progress_update(struct lgfs2_progress_bar *progress, uint64_t value)
> +{
> +	time_t current_time;
> +
> +	if (progress->skip_progress)
> +		return;
> +
> +	current_time = time(0);
> +	if (current_time == last_update)
> +		return;
> +	last_update = current_time;
> +
> +	printf("[%*"PRIu64"/%*"PRIu64"]", progress->max_digits, value,
> +		progress->max_digits, progress->max);
> +
> +	fprintf(stdout, "%.*s", (2 * progress->max_digits) + 3, backspaces);

I tested the patch and it didn't show any progress messages for me, it 
only showed the "Done" message. stdout is line-buffered so flushing it 
at this point should fix that.

Also, when I redirected the output to a file it still printed all of the 
backspace characters (which my editor displays as ^H^H^H...) so perhaps 
it would be a good idea to skip printing the updates if 
(!isatty(STDOUT_FILENO)).

> +}
> +
> +extern void lgfs2_progress_close(struct lgfs2_progress_bar *progress, const char *message)
> +{
> +	if (progress->skip_progress)
> +		return;
> +
> +	fprintf(stdout, "%.*s", (2 * progress->max_digits) + 3, spaces);
> +	fprintf(stdout, "%.*s", (2 * progress->max_digits) + 3, backspaces);
> +
> +	if (message)
> +		fputs(message, stdout);
> +}
> diff --git a/gfs2/mkfs/main_mkfs.c b/gfs2/mkfs/main_mkfs.c
> index d3d8edf..245ddba 100644
> --- a/gfs2/mkfs/main_mkfs.c
> +++ b/gfs2/mkfs/main_mkfs.c
> @@ -653,10 +653,14 @@ static int add_rgrp(lgfs2_rgrps_t rgs, uint64_t *addr, uint32_t len, lgfs2_rgrp_
>
>   static int place_journals(struct gfs2_sbd *sdp, lgfs2_rgrps_t rgs, struct mkfs_opts *opts, uint64_t *rgaddr)
>   {
> +	struct lgfs2_progress_bar progress;
>   	uint64_t jfsize = lgfs2_space_for_data(sdp, sdp->bsize, opts->jsize << 20);
>   	uint32_t rgsize = lgfs2_rgsize_for_data(jfsize, sdp->bsize);
>   	unsigned j;
>
> +	/* Initialise a progress bar for resource group creation. */
> +	lgfs2_progress_init(&progress, opts->journals, _("\nAdding journals: "), opts->quiet);

It's a good convention to avoid putting '\n' at the beginning of 
strings, that way it's easier to keep track of who "owns" the newline so 
that they're not duplicated or omitted when different streams/levels of 
output are involved. It likely simplifies translation too. It looks like 
this is intended to leave a blank line though, and I would rather it not 
do that.

> +
>   	/* We'll build the jindex later so remember where we put the journals */
>   	mkfs_journals = calloc(opts->journals, sizeof(*mkfs_journals));
>   	if (mkfs_journals == NULL)
> @@ -668,6 +672,9 @@ static int place_journals(struct gfs2_sbd *sdp, lgfs2_rgrps_t rgs, struct mkfs_o
>   		lgfs2_rgrp_t rg;
>   		struct gfs2_inode in = {0};
>
> +		/* Update progress bar for journal creation. */
> +		lgfs2_progress_update(&progress, (j + 1));
> +
>   		if (opts->debug)
>   			printf(_("Placing resource group for journal%u\n"), j);
>
> @@ -715,12 +722,14 @@ static int place_journals(struct gfs2_sbd *sdp, lgfs2_rgrps_t rgs, struct mkfs_o
>   		}
>   		mkfs_journals[j] = in.i_di.di_num;
>   	}
> +	lgfs2_progress_close(&progress, _("Done\n"));
>
>   	return 0;
>   }
>
>   static int place_rgrps(struct gfs2_sbd *sdp, lgfs2_rgrps_t rgs, struct mkfs_opts *opts)
>   {
> +        struct lgfs2_progress_bar progress;

Should be indented with tab.

>   	uint64_t rgaddr = lgfs2_rgrp_align_addr(rgs, sdp->sb_addr + 1);
>   	uint32_t rgblks = ((opts->rgsize << 20) / sdp->bsize);
>   	int result;
> @@ -729,6 +738,9 @@ static int place_rgrps(struct gfs2_sbd *sdp, lgfs2_rgrps_t rgs, struct mkfs_opts
>   	if (result != 0)
>   		return result;
>
> +	/* Initialise a progress bar for resource group creation (after journal creation). */
> +        lgfs2_progress_init(&progress, (opts->dev.size / sdp->bsize), _("Building resource groups: "), opts->quiet);

Ditto.

> +
>   	lgfs2_rgrps_plan(rgs, sdp->device.length - rgaddr, rgblks);
>
>   	while (1) {
> @@ -744,7 +756,12 @@ static int place_rgrps(struct gfs2_sbd *sdp, lgfs2_rgrps_t rgs, struct mkfs_opts
>   			fprintf(stderr, _("Failed to build resource groups\n"));
>   			return result;
>   		}
> +
> +		/* Update progress bar with resource group address. */
> +		lgfs2_progress_update(&progress, rgaddr);

Would a running rgrp count/total be a more natural progress indicator 
than the rgrp address I wonder?

>   	}
> +	lgfs2_progress_close(&progress, _("Done\n"));
> +
>   	return 0;
>   }
>
> @@ -949,11 +966,17 @@ int main(int argc, char *argv[])
>   		fprintf(stderr, _("Error building '%s': %s\n"), "rindex", strerror(errno));
>   		exit(EXIT_FAILURE);
>   	}
> +	if (!opts.quiet) {
> +		printf("%s", _("Creating quota file: "));
> +		fflush(stdout);
> +	}
>   	error = build_quota(&sbd);
>   	if (error) {
>   		fprintf(stderr, _("Error building '%s': %s\n"), "quota", strerror(errno));
>   		exit(EXIT_FAILURE);
>   	}
> +	if (!opts.quiet)
> +		printf("%s", _("Done\n"));
>
>   	build_root(&sbd);
>   	sb.sb_root_dir = sbd.md.rooti->i_di.di_num;
> @@ -973,6 +996,11 @@ int main(int argc, char *argv[])
>
>   	lgfs2_rgrps_free(&rgs);
>
> +	if (!opts.quiet) {
> +		printf("%s", _("Writing superblock and syncing: "));
> +		fflush(stdout);
> +	}
> +
>   	error = lgfs2_sb_write(&sb, opts.dev.fd, sbd.bsize);
>   	if (error) {
>   		perror(_("Failed to write superblock\n"));
> @@ -992,6 +1020,9 @@ int main(int argc, char *argv[])
>   	}
>
>   	if (!opts.quiet)
> +		printf("%s", _("Done\n\n"));

If the second '\n' is needed, it would be better to print it separately 
so that translators only need to translate "Done\n" and not "Done\n\n" 
as well :) (But see my earlier comment on blank lines).

> +
> +	if (!opts.quiet)
>   		print_results(&sb, &opts, sbd.rgrps, sbd.fssize);

This if-statement could be merged with the previous one.

>
>   	return 0;
> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index 5a37319..b201448 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -14,7 +14,8 @@ CLEANFILES = testvol
>   if HAVE_CHECK
>   UNIT_TESTS = \
>   	check_meta \
> -	check_rgrp
> +	check_rgrp \
> +	check_progress
>   UNIT_SOURCES = \
>   	$(top_srcdir)/gfs2/libgfs2/libgfs2.h
>   UNIT_CFLAGS = \
> @@ -44,6 +45,13 @@ check_rgrp_SOURCES = \
>   check_rgrp_CFLAGS = $(UNIT_CFLAGS)
>   check_rgrp_LDADD = $(UNIT_LDADD)
>   check_rgrp_CPPFLAGS = $(UNIT_CPPFLAGS)
> +
> +check_progress_SOURCES = \
> +	$(UNIT_SOURCES) \
> +	check_progress.c
> +check_progress_CFLAGS = $(UNIT_CFLAGS)
> +check_progress_LDADD = $(UNIT_LDADD)
> +check_progress_CPPFLAGS = $(UNIT_CPPFLAGS)
>   endif
>
>   # The `:;' works around a Bash 3.2 bug when the output is not writable.
> diff --git a/tests/check_progress.c b/tests/check_progress.c
> new file mode 100644
> index 0000000..a266788
> --- /dev/null
> +++ b/tests/check_progress.c
> @@ -0,0 +1,104 @@
> +#include <check.h>
> +#include <stdint.h>
> +
> +#include "libgfs2.h"
> +
> +// TODO: Remove this when the extern is removed from libgfs2
> +void print_it(const char *label, const char *fmt, const char *fmt2, ...) {}
> +
> +START_TEST(test_progress_init)
> +{
> +	struct lgfs2_progress_bar progress;
> +	int quiet = 0;
> +
> +	lgfs2_progress_init(&progress, 15, "\nTest 0: ", quiet);
> +	
> +	fail_unless(progress.max == 15);
> +}
> +END_TEST
> +
> +START_TEST(test_progress_init_quiet)
> +{
> +	struct lgfs2_progress_bar progress;
> +	int quiet = 1;
> +
> +	lgfs2_progress_init(&progress, 15, "\nTest 1: ", quiet);
> +
> +	fail_unless(progress.skip_progress == 1);
> +}
> +END_TEST
> +
> +START_TEST(test_progress_init_max)
> +{
> +	struct lgfs2_progress_bar progress;
> +	int quiet = 0;
> +
> +	lgfs2_progress_init(&progress, UINT64_MAX, "\nTest 2: ", quiet);
> +
> +	fail_unless(progress.max == UINT64_MAX);
> +}
> +END_TEST
> +
> +START_TEST(test_progress_init_max_plus)
> +{
> +	struct lgfs2_progress_bar progress;
> +	int quiet = 0;
> +
> +	lgfs2_progress_init(&progress, UINT64_MAX + 1, "\nTest 3: ", quiet);
> +
> +	fail_unless(progress.max == 0);
> +}
> +END_TEST
> +
> +START_TEST(test_progress_init_min)
> +{
> +	struct lgfs2_progress_bar progress;
> +	int quiet = 0;
> +
> +	lgfs2_progress_init(&progress, 0, "\nTest 4: ", quiet);
> +
> +	fail_unless(progress.max == 0);
> +}
> +END_TEST
> +
> +START_TEST(test_progress_init_min_minus)
> +{
> +	struct lgfs2_progress_bar progress;
> +	int quiet = 0;
> +
> +	lgfs2_progress_init(&progress, -1, "\nTest 4: ", quiet);
> +
> +	fail_unless(progress.max == UINT64_MAX);
> +}
> +END_TEST

I appreciate the inclusion of tests but I'm not really sure what they're 
meant to be testing here. As they're all void functions (which is fine) 
there are no failure modes to test and passing the different values is 
just demonstrating that assignments are happening in progress_init, 
which should be apparent by looking at the code and the output. I guess 
checking the output of progress_update and progress_close based on what 
was passed to progress_init earlier might be useful but writing tests 
like that is probably more trouble than it's worth in this case.

Overall, I think it's a good approach and I think users will like it too.

Thanks,
Andy

> +
> +static Suite * libgfs2_suite(void)
> +{
> +
> +	Suite *s = suite_create("libgfs2");
> +
> +	TCase *tc_progress = tcase_create("progress");
> +
> +	tcase_add_test(tc_progress, test_progress_init);
> +	tcase_add_test(tc_progress, test_progress_init_quiet);
> +	tcase_add_test(tc_progress, test_progress_init_max);
> +	tcase_add_test(tc_progress, test_progress_init_max_plus);
> +	tcase_add_test(tc_progress, test_progress_init_min);
> +	tcase_add_test(tc_progress, test_progress_init_min_minus);
> +
> +	tcase_set_timeout(tc_progress, 120);
> +	suite_add_tcase(s, tc_progress);
> +
> +	return s;
> +}
> +
> +int main(void)
> +{
> +	int failures;
> +	Suite *s = libgfs2_suite();
> +	SRunner *sr = srunner_create(s);
> +	srunner_run_all(sr, CK_NORMAL);
> +	failures = srunner_ntests_failed(sr);
> +	srunner_free(sr);
> +	return failures ? EXIT_FAILURE : EXIT_SUCCESS;
> +}
> diff --git a/tests/libgfs2.at b/tests/libgfs2.at
> index 1d2243c..9b258ec 100644
> --- a/tests/libgfs2.at
> +++ b/tests/libgfs2.at
> @@ -7,3 +7,7 @@ AT_CLEANUP
>   GFS_UNIT_TEST([rgrp.c],[libgfs2])
>   AT_CHECK([check_rgrp], 0, [ignore], [ignore])
>   AT_CLEANUP
> +
> +GFS_UNIT_TEST([progress.c], [libgfs2])
> +AT_CHECK([check_progress], 0, [ignore], [ignore])
> +AT_CLEANUP
>