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

[Paul Evans <pevans@redhat.com>]

Hi Andy,

On 25/09/15 09:38, Andrew Price wrote:
> Hi Paul,
>
> Some comments below...
>
> 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.
>

I shall move this out of libgfs2 and straight into mkfs.

> 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)).

Cheers for pointing this out, everything seemed to be giving correct 
output without the "^H^H^H" with my default editor but on closer 
inspection with others it showed up for me also.

>
> 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.

Taken note.

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

I would agree that a running rgrp count/total would be a more natural 
progress but I did not see a way to check the total number of resource 
groups that were needed. It appears that the total is only known after 
all the rgrps have all been built and placed (sdp->rgrps is only updated 
after the resource group has been placed).

If I have missed out on another location to find the total number of 
resource groups to be built beforehand I am more than happy to move away 
from the rgrp address here.

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

Agreed.

>
> 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.

Yeah, on a second look through these are a little pointless in their 
current state, especially being void functions. I'll look at removing these.

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

I shall work towards getting a V2 of the patch with the changed suggested.

Cheers,

Paul