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

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

[Carlos Maiolino]

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.
---
 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*/
+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);
 	}
 }
 
-- 
1.7.5.2

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

[Steven Whitehouse]

Hi,

On Tue, 2011-07-12 at 11:06 -0300, 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.
> ---
This looks like another bit of code that should be simplified. Why does
are_you_sure() open the device like that I wonder? Its just been opened
immediately above that in the code, so we can remove that opening of the
device, it does nothing useful.

>  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*/
> +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);
>  	}
>  }
>  

This is a change of behaviour. Do we really want to run
check_dev_content() in this case? I suspect not. This is something which
could usefully be made into common code (the "are you sure" bit) but
please don't move it into libgfs2 as we are trying to keep all user
interface code out of there. A useful thing to do would be to work out
how to get at the glibc translations for useful phrases, such as "yes"
and "no" and rewrite the function using those,

Steve.

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

[Andrew Price]

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

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

[Carlos Maiolino]

My apologies for my delay, I was on PTO.

> 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.
> 
Gotcha will look.
> >+/*Function prototypes*/
> 
> Minor point, but I personally dislike comments like this. Everyone
> knows what a function prototype looks like :)
> 
Indeed. I added it mainly to "make it clear" but I agree in do not add
this comment.

> 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?
> 
Looks like both (Steve and Andy) agrees to remove open() call from
are_you_sure() function, so, looks that's the best way to fix this up
and maybe as Andy suggestion set up another function to test device open
if necessary.

My concern here btw, is if there is any problem in add the function prototype 
to the beginning of the file.

What you guys think?
-- 
--Carlos

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

[Steven Whitehouse]

Hi,

On Mon, 2011-07-18 at 15:44 -0300, Carlos Maiolino wrote:
> My apologies for my delay, I was on PTO.
> 
> > 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.
> > 
> Gotcha will look.
> > >+/*Function prototypes*/
> > 
> > Minor point, but I personally dislike comments like this. Everyone
> > knows what a function prototype looks like :)
> > 
> Indeed. I added it mainly to "make it clear" but I agree in do not add
> this comment.
> 
> > 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?
> > 
> Looks like both (Steve and Andy) agrees to remove open() call from
> are_you_sure() function, so, looks that's the best way to fix this up
> and maybe as Andy suggestion set up another function to test device open
> if necessary.
> 
> My concern here btw, is if there is any problem in add the function prototype 
> to the beginning of the file.
> 
> What you guys think?
No, I can't see any problem with that, it sounds good to me,

Steve.