[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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

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)

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



[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]