[Cluster-devel] [PATCH] mkfs: Remove duplicated code from verify_bsize()

[Andrew Price <anprice@redhat.com>]

Hi Carlos,

On 12/07/11 15:06, Carlos Maiolino wrote:
> Although are_you_sure() function adds some little extra overhead due some
> extra checks, use this function instead of duplicate code is worth.
> The extra security checks and the additional overhead will not be
> noticed by the user.
>
> This is the first try of this patch, but I think the good way to do that
> is to move are_you_sure() and some related functions to libgfs2.

are_you_sure() doesn't really belong in libgfs2 because it's user 
interface code and it has exit() calls (via die()). You might want to 
read bz408631 to see where libgfs2 is heading.

> ---
>   gfs2/mkfs/main_mkfs.c |   12 ++++--------
>   1 files changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/gfs2/mkfs/main_mkfs.c b/gfs2/mkfs/main_mkfs.c
> index b0bb6e3..315f191 100644
> --- a/gfs2/mkfs/main_mkfs.c
> +++ b/gfs2/mkfs/main_mkfs.c
> @@ -26,6 +26,9 @@
>   #include "libgfs2.h"
>   #include "gfs2_mkfs.h"
>
> +/*Function prototypes*/

Minor point, but I personally dislike comments like this. Everyone knows 
what a function prototype looks like :)

> +static void are_you_sure(struct gfs2_sbd *sdp);
> +
>   int discard = 1;
>
>   /**
> @@ -317,14 +320,7 @@ static void verify_bsize(struct gfs2_sbd *sdp)
>   		if (sdp->override)
>   			return;
>
> -		printf( _("\nAre you sure you want to proceed? [y/n] "));
> -		if(!fgets(input, 32, stdin))
> -			die( _("unable to read from stdin\n"));
> -
> -		if (input[0] != 'y')
> -			die( _("aborted\n"));
> -		else
> -			printf("\n");
> +		are_you_sure(sdp);

I'm not sure that it is worth using are_you_sure() here, as it opens the 
device again and calls check_dev_content() (which calls pipe() and then 
fork() ...), which is a lot more overhead than the few lines that you're 
removing here. Perhaps you could change are_you_sure to be more generic 
and move the checking into a separate function?

>   	}
>   }
>

Andy