Date: Wed, 11 Feb 1998 08:30:02 -0800 (PST) From: Bruce Evans <bde@zeta.org.au> To: freebsd-bugs Subject: Re: bin/5712: /bin/chio code cleaup and option added Message-ID: <199802111630.IAA29424@hub.freebsd.org>
next in thread | raw e-mail | index | archive | help
The following reply was made to PR bin/5712; it has been noted by GNATS.
From: Bruce Evans <bde@zeta.org.au>
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));
>
> /* <from ET> */
> cmd.cm_fromtype = parse_element_type(*argv);
>--- 177,183 ----
> warnx("%s: too many arguments", cname);
> goto usage;
> }
>! (void) memset(&cmd, 0, sizeof(cmd));
>
> /* <from ET> */
> 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?199802111630.IAA29424>