[PATCH] a few small mconf improvements

[PATCH] a few small mconf improvements

[Jesper Juhl]

Hi,

I just took a look at scripts/kconfig/mconf.c and found a few tiny things 
that I feel could be improved. So below is a patch for those.

The patch makes the following changes : 

 - use EXIT_SUCCESS/EXIT_FAILURE in place of 0/1.

 - rename main() arguments from "ac"/"av" to the more common "argc"/"argv".

 - when unlinking lxdialog.scrltmp, the return value of unlink() is not 
   checked. The patch adds a check of the return value and bails out if 
   unlink() fails for any reason other than ENOENT.

 - if the sscanf() call in conf() fails and stat==0 && type=='t', then 
   we'll end up dereferencing a NULL 'sym' in sym_is_choice(). The patch 
   adds a NULL check of 'sym' to that path and bails out with a big fat 
   error message if that should ever happen (better than just crashing 
   IMHO).

 - change the type if the 'i' variable in conf() from 'int' to 
   'unsigned int' to avoid a "comparison between signed and unsigned" 
   warning if building with gcc -W. Given the use of 'i', 'unsigned' is 
   also the logical type to use.

Please consider for inclusion.


Signed-off-by: Jesper Juhl <jesper.juhl@gmail.com>
---

 scripts/kconfig/mconf.c |   27 +++++++++++++++++++--------
 1 files changed, 19 insertions(+), 8 deletions(-)

--- linux-2.6.17-rc3-git12-orig/scripts/kconfig/mconf.c	2006-03-20 06:53:29.000000000 +0100
+++ linux-2.6.17-rc3-git12/scripts/kconfig/mconf.c	2006-05-07 17:34:47.000000000 +0200
@@ -311,7 +311,7 @@ static void init_wsize(void)
 	if (rows < 19 || cols < 80) {
 		fprintf(stderr, N_("Your display is too small to run Menuconfig!\n"));
 		fprintf(stderr, N_("It must be at least 19 lines by 80 columns.\n"));
-		exit(1);
+		exit(EXIT_FAILURE);
 	}
 
 	rows -= 4;
@@ -505,7 +505,7 @@ static int exec_conf(void)
 	}
 	if (WIFSIGNALED(stat)) {
 		printf("\finterrupted(%d)\n", WTERMSIG(stat));
-		exit(1);
+		exit(EXIT_FAILURE);
 	}
 #if 0
 	printf("\fexit state: %d\nexit data: '%s'\n", WEXITSTATUS(stat), input_buf);
@@ -514,7 +514,7 @@ static int exec_conf(void)
 	sigpending(&sset);
 	if (sigismember(&sset, SIGINT)) {
 		printf("\finterrupted\n");
-		exit(1);
+		exit(EXIT_FAILURE);
 	}
 	sigprocmask(SIG_SETMASK, &osset, NULL);
 
@@ -718,9 +718,14 @@ static void conf(struct menu *menu)
 	const char *prompt = menu_get_prompt(menu);
 	struct symbol *sym;
 	char active_entry[40];
-	int stat, type, i;
+	int stat, type;
+	unsigned int i;
 
-	unlink("lxdialog.scrltmp");
+	if (unlink("lxdialog.scrltmp") == -1 && errno != ENOENT) {
+		fprintf(stderr, "mconf error, unable to unlink "
+			"lxdialog.scrltmp : %s\n", strerror(errno));
+		exit(EXIT_FAILURE);
+	}
 	active_entry[0] = 0;
 	while (1) {
 		cprint_init();
@@ -777,6 +782,12 @@ static void conf(struct menu *menu)
 					conf(submenu);
 				break;
 			case 't':
+				if (!sym) {
+					fprintf(stderr, "mconf error: "
+						"expected a symbol, got NULL."
+						" Something's badly broken\n");
+					exit(EXIT_FAILURE);
+				}
 				if (sym_is_choice(sym) && sym_get_tristate_value(sym) == yes)
 					conf_choice(submenu);
 				else if (submenu->prompt->type == P_MENU)
@@ -1038,7 +1049,7 @@ static void conf_cleanup(void)
 	unlink("lxdialog.scrltmp");
 }
 
-int main(int ac, char **av)
+int main(int argc, char **argv)
 {
 	struct symbol *sym;
 	char *mode;
@@ -1048,7 +1059,7 @@ int main(int ac, char **av)
 	bindtextdomain(PACKAGE, LOCALEDIR);
 	textdomain(PACKAGE);
 
-	conf_parse(av[1]);
+	conf_parse(argv[1]);
 	conf_read(NULL);
 
 	sym = sym_lookup("KERNELVERSION", 0);
@@ -1094,5 +1105,5 @@ int main(int ac, char **av)
 			"\n\n"));
 	}
 
-	return 0;
+	return EXIT_SUCCESS;
 }

Re: [PATCH] a few small mconf improvements

[Roman Zippel]

On Sun, 7 May 2006, Jesper Juhl wrote:

>  - rename main() arguments from "ac"/"av" to the more common "argc"/"argv".

conf.c and qconf.cc do the same, it's a personal preference.

>  - when unlinking lxdialog.scrltmp, the return value of unlink() is not 
>    checked. The patch adds a check of the return value and bails out if 
>    unlink() fails for any reason other than ENOENT.

The check is not needed, the worst that can happen is a misbehaving 
lxdialog and you certainly have bigger problems than this, if the unlink
should fail. In the long term this should go away anyway.

>  - if the sscanf() call in conf() fails and stat==0 && type=='t', then 
>    we'll end up dereferencing a NULL 'sym' in sym_is_choice(). The patch 
>    adds a NULL check of 'sym' to that path and bails out with a big fat 
>    error message if that should ever happen (better than just crashing 
>    IMHO).

That error message is as useful to the normal user as a segfault - mconf 
doesn't work. Since it shouldn't happen, this check adds no real value, 
the user still has to provide enough information to reproduce the problem 
and at this point it makes no difference, whether I get this message or I 
see where it stops with gdb.

bye, Roman

Re: [PATCH] a few small mconf improvements

[Jesper Juhl]

On 09/05/06, Roman Zippel <zippel@linux-m68k.org> wrote:
>
>
> On Sun, 7 May 2006, Jesper Juhl wrote:
>
> >  - rename main() arguments from "ac"/"av" to the more common "argc"/"argv".
>
> conf.c and qconf.cc do the same, it's a personal preference.
>
Fair enough.


> >  - when unlinking lxdialog.scrltmp, the return value of unlink() is not
> >    checked. The patch adds a check of the return value and bails out if
> >    unlink() fails for any reason other than ENOENT.
>
> The check is not needed, the worst that can happen is a misbehaving
> lxdialog and you certainly have bigger problems than this, if the unlink
> should fail. In the long term this should go away anyway.
>
Ok, your call.


> >  - if the sscanf() call in conf() fails and stat==0 && type=='t', then
> >    we'll end up dereferencing a NULL 'sym' in sym_is_choice(). The patch
> >    adds a NULL check of 'sym' to that path and bails out with a big fat
> >    error message if that should ever happen (better than just crashing
> >    IMHO).
>
> That error message is as useful to the normal user as a segfault - mconf
> doesn't work. Since it shouldn't happen, this check adds no real value,
> the user still has to provide enough information to reproduce the problem
> and at this point it makes no difference, whether I get this message or I
> see where it stops with gdb.
>
I disagree a little here. It may not really matter to you if you get a
report of a crash or if you get a report that mconf spewed an error
message, but to the user who experiences it (should it ever happen)
there's a difference - it's either "the damn thing crashed on me, what
a piece of crap" or "the damn thing crashed on me, but at least it
told me something went wrong, so now I can report it"... Printing an
error and exiting cleanly is IMHO always preferable to a crash - users
respond better to that and it's the "right" thing to do.

What about the other bits of the patch? are those OK?
Want me to send an updated patch - or?


-- 
Jesper Juhl <jesper.juhl@gmail.com>
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please      http://www.expita.com/nomime.html

Re: [PATCH] a few small mconf improvements

[Roman Zippel]

Hi,

On Sat, 13 May 2006, Jesper Juhl wrote:

(Sorry for the delay.)

> > >  - if the sscanf() call in conf() fails and stat==0 && type=='t', then
> > >    we'll end up dereferencing a NULL 'sym' in sym_is_choice(). The patch
> > >    adds a NULL check of 'sym' to that path and bails out with a big fat
> > >    error message if that should ever happen (better than just crashing
> > >    IMHO).
> > 
> > That error message is as useful to the normal user as a segfault - mconf
> > doesn't work. Since it shouldn't happen, this check adds no real value,
> > the user still has to provide enough information to reproduce the problem
> > and at this point it makes no difference, whether I get this message or I
> > see where it stops with gdb.
> > 
> I disagree a little here. It may not really matter to you if you get a
> report of a crash or if you get a report that mconf spewed an error
> message, but to the user who experiences it (should it ever happen)
> there's a difference - it's either "the damn thing crashed on me, what
> a piece of crap" or "the damn thing crashed on me, but at least it
> told me something went wrong, so now I can report it"... Printing an
> error and exiting cleanly is IMHO always preferable to a crash - users
> respond better to that and it's the "right" thing to do.

The point is that this shouldn't happen and so far didn't, you have to do 
something really weird to trigger this, so a big error message is not 
appropriate. Something like assert() would be more acceptable, but IMO 
it's just not important enough.

> What about the other bits of the patch? are those OK?

Yes, it's OK.

bye, Roman