From owner-freebsd-bugs Wed Feb 11 08:30:04 1998 Return-Path: Received: (from majordom@localhost) by hub.freebsd.org (8.8.8/8.8.8) id IAA29451 for freebsd-bugs-outgoing; Wed, 11 Feb 1998 08:30:04 -0800 (PST) (envelope-from owner-freebsd-bugs@FreeBSD.ORG) Received: (from gnats@localhost) by hub.freebsd.org (8.8.8/8.8.8) id IAA29424; Wed, 11 Feb 1998 08:30:02 -0800 (PST) (envelope-from gnats) Date: Wed, 11 Feb 1998 08:30:02 -0800 (PST) Message-Id: <199802111630.IAA29424@hub.freebsd.org> To: freebsd-bugs Cc: From: Bruce Evans Subject: Re: bin/5712: /bin/chio code cleaup and option added Reply-To: Bruce Evans Sender: owner-freebsd-bugs@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.org The following reply was made to PR bin/5712; it has been noted by GNATS. From: Bruce Evans To: freebsd-gnats-submit@FreeBSD.ORG, jason_smethers@bigfoot.com Cc: Subject: Re: bin/5712: /bin/chio code cleaup and option added Date: Thu, 12 Feb 1998 03:22:55 +1100 Review (points noted in review of `cat' cleanup not all repeated): >diff -c -r /usr/src/bin/chio/chio.1 /usr/local/src/bin/chio/chio.1 >*** /usr/src/bin/chio/chio.1 Sat Sep 13 11:01:18 1997 >--- /usr/local/src/bin/chio/chio.1 Sun Feb 8 21:16:55 1998 >... >--- 190,203 ---- > Moves the media in slot 3 (fourth slot) to drive 0 (first drive). > .Pp > .Nm chio setpicker 2 >+ .Pp > Configures the changer to use picker 2 (third picker) for operations. > .Pp > .Sh FILES > /dev/ch0 - default changer device > .Sh SEE ALSO > .Xr mt 1 , >! .Xr ch 4 > .Xr mount 8 . >... Lost a comma. >diff -c -r /usr/src/bin/chio/chio.c /usr/local/src/bin/chio/chio.c >*** /usr/src/bin/chio/chio.c Fri Jun 6 01:32:09 1997 >--- /usr/local/src/bin/chio/chio.c Sun Feb 8 19:08:22 1998 >*************** >*** 47,52 **** >--- 47,53 ---- > #include "defs.h" > #include "pathnames.h" > >+ int main __P((int, char *[])); > static void usage __P((void)); > static void cleanup __P((void)); > static int parse_element_type __P((char *)); >*************** >*** 62,67 **** >--- 63,69 ---- > static int do_getpicker __P((char *, int, char **)); > static int do_setpicker __P((char *, int, char **)); > static int do_status __P((char *, int, char **)); >+ static int do_ielem __P((char *, int, char **)); > > /* Valid changer element types. */ > const struct element_type elements[] = { >*************** >*** 81,86 **** >--- 83,89 ---- > { "getpicker", do_getpicker }, > { "setpicker", do_setpicker }, > { "status", do_status }, >+ { "ielem", do_ielem }, > { NULL, 0 }, > }; > The lists are more disordered than before. >*************** >*** 135,146 **** >--- 138,157 ---- > for (i = 0; commands[i].cc_name != NULL; ++i) > if (strcmp(*argv, commands[i].cc_name) == 0) > break; >+ if (commands[i].cc_name == NULL) { >+ /* look for abbreviation */ >+ for (i = 0; commands[i].cc_name != NULL; ++i) >+ if (strncmp(*argv, commands[i].cc_name, >+ strlen(*argv)) == 0) >+ break; >+ } > if (commands[i].cc_name == NULL) > errx(1, "unknown command: %s", *argv); > > /* Skip over the command name and call handler. */ > ++argv; --argc; > exit ((*commands[i].cc_handler)(commands[i].cc_name, argc, argv)); >+ /* NOTREACHED */ > } I don't like comments for a nonexistent/unusable lint. gcc knows that exit() doesn't return. > static int >*************** >*** 166,172 **** > warnx("%s: too many arguments", cname); > goto usage; > } >! bzero(&cmd, sizeof(cmd)); > > /* */ > cmd.cm_fromtype = parse_element_type(*argv); >--- 177,183 ---- > warnx("%s: too many arguments", cname); > goto usage; > } >! (void) memset(&cmd, 0, sizeof(cmd)); > > /* */ > cmd.cm_fromtype = parse_element_type(*argv); style(9) specifies no space after casts. This rule is followed only about half the time for casts of function return values in the kernel, but I think it should be followed in new code. I don't like casting function return values anyway. >--- 534,551 ---- >... >+ >+ default: >+ /* To appease gcc -Wuninitialized. */ >+ count = 0; >+ description = NULL; Not initializing `count' would just be a bug. Only `description' must be set to appease gcc. >--- 578,601 ---- >... >+ /* ARGSUSED */ >+ static int >+ do_ielem(cname, argc, argv) >+ char *cname; >+ int argc; >+ char **argv; >+ { >+ if (ioctl(changer_fd, CHIOIELEM, NULL)) >+ err(1, "%s: CHIOIELEM", changer_name); >+ >+ return (0); >+ } >+ >+ > static int > parse_element_type(cp) > char *cp; style(9) should specify that there are no empty lines between statements except to separate blocks of code headed by a comment, and no pairs of empty lines. >*************** >*** 576,581 **** >--- 607,613 ---- > return (elements[i].et_type); > > errx(1, "invalid element type `%s'", cp); >+ /* NOTREACHED */ > } > > static int gcc also understands that errx() returns. I think the author of lint was going to teach it about gcc attributes, but has stopped working on it. >*************** >*** 636,642 **** > if ((v & (1 << (f - 1))) == 0) > continue; > bp += snprintf(bp, sizeof(buf) - (bp - &buf[0]), >! "%c%.*s", sep, np - cp, cp); > sep = ','; > } > if (sep != '<') >--- 669,675 ---- > if ((v & (1 << (f - 1))) == 0) > continue; > bp += snprintf(bp, sizeof(buf) - (bp - &buf[0]), >! "%c%.*s", sep, (int)(long)(np - cp), cp); > sep = ','; > } > if (sep != '<') style(9) specifies a secondary indent of 4. Casting to (long) is bogus here. >*************** >*** 648,654 **** > static void > cleanup() > { >- > /* Simple enough... */ > (void)close(changer_fd); > } style(9) specifies the empty line that was here. Bruce To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-bugs" in the body of the message