[Cluster-devel] [PATCH 1/3] mkfs: Use rpmatch() to yes/no questions

[Cluster-devel] [PATCH 1/3] mkfs: Use rpmatch() to yes/no questions

[Carlos Maiolino]

This patch changes the are_you_sure() function
to make use of rpmatch() to identify the user's
answer in the language set in the i18n
Also add 2 another translatable strings

Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---
 gfs2/mkfs/main_mkfs.c |   34 ++++++++++++++++++++++++----------
 1 files changed, 24 insertions(+), 10 deletions(-)

diff --git a/gfs2/mkfs/main_mkfs.c b/gfs2/mkfs/main_mkfs.c
index 4751f19..cc678fe 100644
--- a/gfs2/mkfs/main_mkfs.c
+++ b/gfs2/mkfs/main_mkfs.c
@@ -78,7 +78,7 @@ static int discard_blocks(struct gfs2_sbd *sdp)
 	range[0] = 0;
 	range[1] = sdp->device.length * sdp->bsize;
 	if (sdp->debug)
-		printf("Issuing discard ioctl: range: %llu - %llu...",
+		printf(_("Issuing discard ioctl: range: %llu - %llu..."),
 		       (unsigned long long)range[0],
 		       (unsigned long long)range[1]);
 	if (ioctl(sdp->device_fd, BLKDISCARD, &range) < 0) {
@@ -87,7 +87,7 @@ static int discard_blocks(struct gfs2_sbd *sdp)
 		return errno;
 	}
 	if (sdp->debug)
-		printf("Successful.\n");
+		printf(_("Successful.\n"));
         return 0;
 }
 
@@ -456,8 +456,11 @@ fail:
 
 static void are_you_sure(struct gfs2_sbd *sdp)
 {
-	char input[32];
+	char *line = NULL;
+	size_t len = 0;
 	int fd;
+	int ret = -1;
+	int res = 0;
 
 	fd = open(sdp->device_name, O_RDONLY|O_CLOEXEC);
 	if (fd < 0)
@@ -465,14 +468,25 @@ static void are_you_sure(struct gfs2_sbd *sdp)
 	printf( _("This will destroy any data on %s.\n"), sdp->device_name);
 	check_dev_content(sdp->device_name);
 	close(fd);
-	printf( _("\nAre you sure you want to proceed? [y/n] "));
-	if(!fgets(input, 32, stdin))
-		die( _("unable to read from stdin\n"));
+	
+	do{
+		printf( _("Are you sure you want to proceed? [y/n]"));
+		ret = getline(&line, &len, stdin);
+		res = rpmatch(line);
+		
+		if (res > 0){
+			free(line);
+			return;
+		}
+		if (!res){
+			printf("\n");
+			die( _("aborted\n"));
+		}
+		
+	}while(ret >= 0);
 
-	if (input[0] != 'y')
-		die( _("aborted\n"));
-	else
-		printf("\n");
+	if(line)
+		free(line);
 }
 
 /**
-- 
1.7.6

[Cluster-devel] [PATCH 2/3] mkfs: Remove unneeded open/close fd test from are_you_sure()

[Carlos Maiolino]

are_you_sure() function should not make an open/close
test on the underlying device. The pourpose of this
function is just to ask a yes/no question to the user.
If we need a function to do this test, we should create
a new function with its specific pourpose.
This patch also fix a call to are_you_sure() function and
move the are_you_sure() function to the begining of the
file, allowing it to be used in another part of the code

Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---
 gfs2/mkfs/main_mkfs.c |   75 ++++++++++++++++++++++--------------------------
 1 files changed, 34 insertions(+), 41 deletions(-)

diff --git a/gfs2/mkfs/main_mkfs.c b/gfs2/mkfs/main_mkfs.c
index cc678fe..8802ef7 100644
--- a/gfs2/mkfs/main_mkfs.c
+++ b/gfs2/mkfs/main_mkfs.c
@@ -289,6 +289,39 @@ static void test_locking(char *lockproto, char *locktable)
 	}
 }
 
+/**
+ * are_you_sure - protect lusers from themselves
+ * @sdp: the command line
+ *
+ */
+
+static void are_you_sure(void)
+{
+	char *line = NULL;
+	size_t len = 0;
+	int ret = -1;
+	int res = 0;
+
+	do{
+		printf( _("Are you sure you want to proceed? [y/n]"));
+		ret = getline(&line, &len, stdin);
+		res = rpmatch(line);
+		
+		if (res > 0){
+			free(line);
+			return;
+		}
+		if (!res){
+			printf("\n");
+			die( _("aborted\n"));
+		}
+		
+	}while(ret >= 0);
+
+	if(line)
+		free(line);
+}
+
 static void verify_bsize(struct gfs2_sbd *sdp)
 {
 	unsigned int x;
@@ -448,46 +481,6 @@ fail:
 	exit(execv(args[0], args));
 }
 
-/**
- * are_you_sure - protect lusers from themselves
- * @sdp: the command line
- *
- */
-
-static void are_you_sure(struct gfs2_sbd *sdp)
-{
-	char *line = NULL;
-	size_t len = 0;
-	int fd;
-	int ret = -1;
-	int res = 0;
-
-	fd = open(sdp->device_name, O_RDONLY|O_CLOEXEC);
-	if (fd < 0)
-		die( _("Error: device %s not found.\n"), sdp->device_name);
-	printf( _("This will destroy any data on %s.\n"), sdp->device_name);
-	check_dev_content(sdp->device_name);
-	close(fd);
-	
-	do{
-		printf( _("Are you sure you want to proceed? [y/n]"));
-		ret = getline(&line, &len, stdin);
-		res = rpmatch(line);
-		
-		if (res > 0){
-			free(line);
-			return;
-		}
-		if (!res){
-			printf("\n");
-			die( _("aborted\n"));
-		}
-		
-	}while(ret >= 0);
-
-	if(line)
-		free(line);
-}
 
 /**
  * print_results - print out summary information
@@ -576,7 +569,7 @@ void main_mkfs(int argc, char *argv[])
 	}
 
 	if (!sdp->override)
-		are_you_sure(sdp);
+		are_you_sure();
 
 	if (!S_ISREG(st_buf.st_mode) && device_topology(sdp)) {
 		fprintf(stderr, _("Device topology error\n"));
-- 
1.7.6

[Cluster-devel] [PATCH 3/3] mkfs: remove duplicated code to ask yes/no question

[Carlos Maiolino]

This patch takes advantage of the are_you_sure() function
to ask a yes/no question within verify_bsize() function

Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---
 gfs2/mkfs/main_mkfs.c |    9 +--------
 1 files changed, 1 insertions(+), 8 deletions(-)

diff --git a/gfs2/mkfs/main_mkfs.c b/gfs2/mkfs/main_mkfs.c
index 8802ef7..0bd1410 100644
--- a/gfs2/mkfs/main_mkfs.c
+++ b/gfs2/mkfs/main_mkfs.c
@@ -350,14 +350,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();
 	}
 }
 
-- 
1.7.6

[Cluster-devel] [PATCH 3/3] mkfs: remove duplicated code to ask yes/no question

[Bob Peterson]

----- Original Message -----
| This patch takes advantage of the are_you_sure() function
| to ask a yes/no question within verify_bsize() function
| 

Hi,

Looks good.

Regards,

Bob Peterson
Red Hat File Systems

[Cluster-devel] [PATCH 3/3] mkfs: remove duplicated code to ask yes/no question

[Steven Whitehouse]

Hi,

Modulo my comment on the first patch, otherwise these look good,

Steve.

On Tue, 2011-09-27 at 13:08 -0300, Carlos Maiolino wrote:
> This patch takes advantage of the are_you_sure() function
> to ask a yes/no question within verify_bsize() function
> 
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---
>  gfs2/mkfs/main_mkfs.c |    9 +--------
>  1 files changed, 1 insertions(+), 8 deletions(-)
> 
> diff --git a/gfs2/mkfs/main_mkfs.c b/gfs2/mkfs/main_mkfs.c
> index 8802ef7..0bd1410 100644
> --- a/gfs2/mkfs/main_mkfs.c
> +++ b/gfs2/mkfs/main_mkfs.c
> @@ -350,14 +350,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();
>  	}
>  }
>  

[Cluster-devel] [PATCH 2/3] mkfs: Remove unneeded open/close fd test from are_you_sure()

[Bob Peterson]

----- Original Message -----
| are_you_sure() function should not make an open/close
| test on the underlying device. The pourpose of this
| function is just to ask a yes/no question to the user.
| If we need a function to do this test, we should create
| a new function with its specific pourpose.
| This patch also fix a call to are_you_sure() function and
| move the are_you_sure() function to the begining of the
| file, allowing it to be used in another part of the code
| 
| Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>

Hi,

This all looks good and makes sense.  I am concerned
that removing the open/close might change the behavior
when errors occur: For example, if you try to mkfs.gfs2
for a device you don't have permission to.  Or for a
device that doesn't exist.  Please test those two cases
to make sure nothing weird happens.

Regards,

Bob Peterson
Red Hat File Systems

[Cluster-devel] [PATCH 2/3] mkfs: Remove unneeded open/close fd test from are_you_sure()

[Carlos Maiolino]

Hi Bob,

> Hi,
> 
> This all looks good and makes sense.  I am concerned
> that removing the open/close might change the behavior
> when errors occur: For example, if you try to mkfs.gfs2
> for a device you don't have permission to.  Or for a
> device that doesn't exist.  Please test those two cases
> to make sure nothing weird happens.
> 
Maybe my mistake to not have written it before, but, I've checked the 
are_you_sure() calls into the code, and before any call to are_you_sure()
we have a open/close test, so, we had the are_you_sure() doing a second
test.

Portions of code in the main_mkfs() function (almost at the begining of the function):

553         sdp->device_fd = open(sdp->device_name, O_RDWR | O_CLOEXEC);
554         if (sdp->device_fd < 0)
555                 die( _("can't open device %s: %s\n"),
556                     sdp->device_name, strerror(errno));
557
558         if (fstat(sdp->device_fd, &st_buf) < 0) {
559                 fprintf(stderr, _("could not fstat fd %d: %s\n"),
560                         sdp->device_fd, strerror(errno));
561                 exit(-1);
562         }
563 
564         if (!sdp->override)
565                 are_you_sure();
.
.
591         verify_bsize(sdp);


After that, we have a call to are_you_sure() inside the verify_bsize()
function, but, the verify_bsize() is called once, in the main_mkfs() too,
after the check specified on the code snippet above. 
What do you think? Are these 3 patches ok to push?

-- 
--Carlos

[Cluster-devel] [PATCH 2/3] mkfs: Remove unneeded open/close fd test from are_you_sure()

[Bob Peterson]

----- Original Message -----
| Hi Bob,
| 
| Maybe my mistake to not have written it before, but, I've checked the
| are_you_sure() calls into the code, and before any call to
| are_you_sure()
| we have a open/close test, so, we had the are_you_sure() doing a
| second
| test.
| 
| Portions of code in the main_mkfs() function (almost at the begining
| of the function):

Hi Carlos,

Thanks for clearing that up.  I should have looked myself; sorry.
Yes, the patches look good to push as far as I'm concerned.

Regards,

Bob Peterson
Red Hat File System

[Cluster-devel] [PATCH 1/3] mkfs: Use rpmatch() to yes/no questions

[Bob Peterson]

----- Original Message -----
| This patch changes the are_you_sure() function
| to make use of rpmatch() to identify the user's
| answer in the language set in the i18n
| Also add 2 another translatable strings
| 
| Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
| ---
Hi,

Looks good.

Regards,

Bob Peterson
Red Hat File Systems

[Cluster-devel] [PATCH 1/3] mkfs: Use rpmatch() to yes/no questions

[Steven Whitehouse]

Hi,

On Tue, 2011-09-27 at 13:08 -0300, Carlos Maiolino wrote:
> This patch changes the are_you_sure() function
> to make use of rpmatch() to identify the user's
> answer in the language set in the i18n
> Also add 2 another translatable strings
> 
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---
>  gfs2/mkfs/main_mkfs.c |   34 ++++++++++++++++++++++++----------
>  1 files changed, 24 insertions(+), 10 deletions(-)
> 
> diff --git a/gfs2/mkfs/main_mkfs.c b/gfs2/mkfs/main_mkfs.c
> index 4751f19..cc678fe 100644
> --- a/gfs2/mkfs/main_mkfs.c
> +++ b/gfs2/mkfs/main_mkfs.c
> @@ -78,7 +78,7 @@ static int discard_blocks(struct gfs2_sbd *sdp)
>  	range[0] = 0;
>  	range[1] = sdp->device.length * sdp->bsize;
>  	if (sdp->debug)
> -		printf("Issuing discard ioctl: range: %llu - %llu...",
> +		printf(_("Issuing discard ioctl: range: %llu - %llu..."),
>  		       (unsigned long long)range[0],
>  		       (unsigned long long)range[1]);
>  	if (ioctl(sdp->device_fd, BLKDISCARD, &range) < 0) {
> @@ -87,7 +87,7 @@ static int discard_blocks(struct gfs2_sbd *sdp)
>  		return errno;
>  	}
>  	if (sdp->debug)
> -		printf("Successful.\n");
> +		printf(_("Successful.\n"));
Maybe just say "Success" here? Can this be made to match other similar
strings?

>          return 0;
>  }
>  
> @@ -456,8 +456,11 @@ fail:
>  
>  static void are_you_sure(struct gfs2_sbd *sdp)
>  {
> -	char input[32];
> +	char *line = NULL;
> +	size_t len = 0;
>  	int fd;
> +	int ret = -1;
> +	int res = 0;
>  
>  	fd = open(sdp->device_name, O_RDONLY|O_CLOEXEC);
>  	if (fd < 0)
> @@ -465,14 +468,25 @@ static void are_you_sure(struct gfs2_sbd *sdp)
>  	printf( _("This will destroy any data on %s.\n"), sdp->device_name);
>  	check_dev_content(sdp->device_name);
>  	close(fd);
> -	printf( _("\nAre you sure you want to proceed? [y/n] "));
> -	if(!fgets(input, 32, stdin))
> -		die( _("unable to read from stdin\n"));
> +	
> +	do{
> +		printf( _("Are you sure you want to proceed? [y/n]"));
> +		ret = getline(&line, &len, stdin);
> +		res = rpmatch(line);
> +		
> +		if (res > 0){
> +			free(line);
> +			return;
> +		}
> +		if (!res){
> +			printf("\n");
> +			die( _("aborted\n"));
Please don't add more die() calls. Could we replace this with another
message? Don't be afraid to do:

   errno = ESOMETHING;
   perror();

or to use strerror() when required,

Steve.