From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Price Date: Tue, 12 Jul 2011 15:37:58 +0100 Subject: [Cluster-devel] [PATCH] mkfs: Remove duplicated code from verify_bsize() In-Reply-To: <1310479579-12731-1-git-send-email-cmaiolino@redhat.com> References: <1310479579-12731-1-git-send-email-cmaiolino@redhat.com> Message-ID: <4E1C5C46.5050205@redhat.com> List-Id: To: cluster-devel.redhat.com MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit 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