[BusyBox] added command "mesg" to busybox

[Manuel Novoa III]

Hello,

On Tue, Apr 30, 2002 at 12:01:38PM -0700, Da Chen wrote:
> Hi,
>     I added linux command "mesg" to busybox to enable or disable
> your terminal to receive message. Here is the patch and file mesg.c.
> mesg.c is in directory "shellutils". I will add command "write" next
> time.
> 
>     Please review it. Thanks.

Since you mentioned you would be submitting more code, here are some
comments.  Just remember that you asked...  And please don't take
this too personally.  This is kind of a general rant against people
submitting additions to busybox without any apparent thought to generated
code size.

>  * Mini mesg -- control write access to terminal.
>  *
>  * Ported to busybox by Da Chen <dchen at ayrnetworks.com>
>  * Copyright (c) 2002 AYR Networks, Inc.

I disagree with "Mini" for the reasons given below.

> ... (stuff omitted)

> extern int mesg_main(int argc, char *argv[])
> {
>         struct stat sb;
>         char *tty;
>         int ch;
> 

Don't you think that calling getopt is overkill here?

>         while ((ch = getopt(argc, argv, "")) != EOF)
>                 switch (ch) {
>                 case '?':
>                 default: show_usage();
>                 }
>         argc -= optind;
>         argv += optind;
> 
>         if ((tty = ttyname(STDERR_FILENO)) == NULL)
>              perror_msg("ttyname");
>         if (stat(tty, &sb) < 0)
>              perror_msg("%s", tty);

Repetitive calles to perror_msg() that could be combined.
Of course, you should really be calling perror_msg_and die(), as
there's been an error and you don't want to continue.

>         if (*argv == NULL) {
>                 if (sb.st_mode & (S_IWGRP | S_IWOTH)) {
>                     fprintf(stdout, "is y\n");
>                     exit (1); 
>                 } else {
>                     fprintf(stdout,"is n\n");
>                     exit (2);
>                 }
>                 
>         }

More repetitive function calls... and using fprintf here is bloat.
Especially since fprint(stdout,...) is equivalent to printf().
But even printf() is more than you need.  In fact, using puts()
allows you to save adding '\n' to the output strings.

Also, why call exit() when a return would suffice.
Furthermore, why the strange return codes.  I suppose it might be
nice to test based against the return code, but in this case it
is questionable since you can't tell whether or not they're indicating
the mode or an error.

>         switch (*argv[0]) {
>         case 'y':
> #ifdef USE_TTY_GROUP
>                 if (chmod(tty, sb.st_mode | S_IWGRP) < 0)
>                     perror_msg("%s", tty);
> #else
>                 if (chmod(tty, sb.st_mode | S_IWGRP | S_IWOTH) < 0)
>                     perror_msg("%s", tty);
> #endif
>                 exit(0);
>         case 'n':
>                 if (chmod(tty, sb.st_mode & ~(S_IWGRP|S_IWOTH)) < 0)
>                     perror_msg("%s", tty);
>                 exit(1);

Similar comments as above... repetitive function calls, redundant code, 
etc...

>         default: show_usage();
>         }
> 
>         return (0);
> }


********************************************************************

To illustrate the points I made above, take a look at the following
version.  I got rid of the odd return codes for the reasons stated
above.  Also, the mesg app that comes with sysvinit doesn't seem to
use the odd return codes either.  For me, size returned 296 bytes
for your mesg.o and 197 for the one below (i386).

Manuel


(same comments and headers)

#ifdef USE_TTY_GROUP
#define S_IWGRP_OR_S_IWOTH	S_IWGRP
#else
#define S_IWGRP_OR_S_IWOTH	(S_IWGRP | S_IWOTH)
#endif

extern int mesg_main(int argc, char *argv[])
{
	struct stat sb;
	char *tty;
	char c;

	if ((--argc == 0)
		|| ((argc == 1) && (((c = **++argv) == 'y') || (c == 'n')))
		) {
		if ((tty = ttyname(STDERR_FILENO)) == NULL) {
			tty = "ttyname";
		} else if (stat(tty, &sb) == 0) {
			if (argc == 0) {
				puts(((sb.st_mode & (S_IWGRP | S_IWOTH)) == 0)
					 ? "is n" : "is y");
				return EXIT_SUCCESS;
			}
			if (chmod(tty,
					  (c == 'y')
					  ? sb.st_mode | (S_IWGRP_OR_S_IWOTH)
					  : sb.st_mode & ~(S_IWGRP|S_IWOTH))
				== 0) {
				return EXIT_SUCCESS;
			}
		}
		perror_msg_and_die("%s", tty);
	}
	show_usage();
}