mkfs: Remove duplicated code from verify_bsize()
PATCH 1/1 mkfs: Remove duplicated code from verify_bsize()
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(-)
gfs2/mkfs/main_mkfs.c +4 -8
@@ -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); } }
Signature · 1 line
1.7.5.2
PATCH 1/1 mkfs: Remove duplicated code from verify_bsize()
Hi,
Quoted 8 lines · 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.
---
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.
Quoted 23 lines
gfs2/mkfs/main_mkfs.c | 12 ++++-------- 1 files changed, 4 insertions(+), 8 deletions(-)gfs2/mkfs/main_mkfs.c +3
-2@@ -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))
gfs2/mkfs/main_mkfs.c static void verify_bsize(struct gfs2_sbd *sdp) · line 326
Quoted 10 lines
gfs2/mkfs/main_mkfs.c +1
-6-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.
PATCH 1/1 mkfs: Remove duplicated code from verify_bsize()
In reply to Carlos MaiolinoHi Carlos,
Quoted 7 lines · 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.
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 line 29
Quoted 13 lines
--- gfs2/mkfs/main_mkfs.c | 12 ++++-------- 1 files changed, 4 insertions(+), 8 deletions(-)gfs2/mkfs/main_mkfs.c +1
-0@@ -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 :)
Quoted 8 lines
gfs2/mkfs/main_mkfs.c +2
-0+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 ;
gfs2/mkfs/main_mkfs.c static void verify_bsize(struct gfs2_sbd *sdp) · line 323
Quoted 10 lines
gfs2/mkfs/main_mkfs.c +1
-8-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?
gfs2/mkfs/main_mkfs.c static void verify_bsize(struct gfs2_sbd *sdp) · line 326
Quoted 3 lines
gfs2/mkfs/main_mkfs.c +0
-0} }
Andy
PATCH 1/1 mkfs: Remove duplicated code from verify_bsize()
My apologies for my delay, I was on PTO.
Quoted 4 lines
are_you_sure()doesn't really belong in libgfs2 because it's user interface code and it hasexit()calls (viadie()). You might want to read bz408631 to see where libgfs2 is heading.
Gotcha will look.
Quoted 5 lines
Quoted 1 line
+/*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.
Quoted 7 lines
I'm not sure that it is worth using
are_you_sure()here, as it opens the device again and callscheck_dev_content()(which callspipe()and thenfork()...), which is a lot more overhead than the few lines that you're removing here. Perhaps you could changeare_you_sureto 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?
Signature · 1 line
--Carlos
PATCH 1/1 mkfs: Remove duplicated code from verify_bsize()
Hi,
Quoted 27 lines · Carlos Maiolino
My apologies for my delay, I was on PTO.
Quoted 4 lines
are_you_sure()doesn't really belong in libgfs2 because it's user interface code and it hasexit()calls (viadie()). You might want to read bz408631 to see where libgfs2 is heading.Gotcha will look.
Quoted 5 lines
Quoted 1 line
+/*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.
Quoted 7 lines
I'm not sure that it is worth using
are_you_sure()here, as it opens the device again and callscheck_dev_content()(which callspipe()and thenfork()...), which is a lot more overhead than the few lines that you're removing here. Perhaps you could changeare_you_sureto be more generic and move the checking into a separate function?Looks like both (Steve and Andy) agrees to remove
open()call fromare_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.
Quoted 4 lines
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.