Re: config(8): use {err,warn}{,x} instead of fprintf(stderr, ...)
On Sun, 16 Oct 2016 11:47:32 +0200, Theo Buehler wrote: > Many files already include and there's a mix of hand-rolled > warning messages and there's incorrect usage warn("config: ..."). This > is a first sweep at unifying them. > > In mkheaders.c, there is an err() function, rename it to emitwarn() > since there are emit* functions already and it is non-fatal. Some minor issues noted inline but otherwise OK. - todd > Index: main.c > === > RCS file: /cvs/src/usr.sbin/config/main.c,v > retrieving revision 1.55 > diff -u -p -r1.55 main.c > --- main.c16 Oct 2016 09:36:46 - 1.55 > +++ main.c16 Oct 2016 09:42:01 - > @@ -75,7 +75,7 @@ static void optiondelta(void); > > int verbose; > > -void > +__dead void > usage(void) > { > extern char *__progname; > @@ -169,19 +169,15 @@ main(int argc, char *argv[]) > > if (eflag) { > #ifdef MAKE_BOOTSTRAP > - fprintf(stderr, "config: UKC not available in this binary\n"); > - exit(1); > + errx(1, "UKC not available in this binary"); > #else > return (ukc(argv[0], outfile, uflag, fflag)); > #endif > } > > conffile = (argc == 1) ? argv[0] : "CONFIG"; > - if (firstfile(conffile)) { > - (void)fprintf(stderr, "config: cannot read %s: %s\n", > - conffile, strerror(errno)); > - exit(2); > - } > + if (firstfile(conffile)) > + err(2, "cannot read %s\n", conffile); You don't want that trailing newline. > > /* >* Init variables. > @@ -246,8 +242,7 @@ main(int argc, char *argv[]) > defmaxusers); > maxusers = defmaxusers; > } else { > - (void)fprintf(stderr, > - "config: need \"maxusers\" line\n"); > + warnx("need \"maxusers\" line"); > errors++; > } > } > @@ -269,7 +264,7 @@ main(int argc, char *argv[]) > mkioconf()) > stop(); > optiondelta(); > - exit(0); > + return (0); > } > > static int > @@ -278,11 +273,11 @@ mksymlink(const char *value, const char > int ret = 0; > > if (remove(path) && errno != ENOENT) { > - warn("config: remove(%s)", path); > + warn("remove(%s)", path); > ret = 1; > } > if (symlink(value, path)) { > - warn("config: symlink(%s -> %s)", path, value); > + warn("symlink(%s -> %s)", path, value); > ret = 1; > } > return (ret); > @@ -624,8 +619,7 @@ badstar(void) > continue; > foundstar: > if (ht_lookup(needcnttab, d->d_name)) { > - (void)fprintf(stderr, > - "config: %s's cannot be *'d until its driver is fixed\n", > + warnx("%s's cannot be *'d until its driver is fixed", > d->d_name); > errs++; > continue; > @@ -662,26 +656,14 @@ setupdirs(void) > builddir = defbuilddir; > > if (stat(builddir, &st) != 0) { > - if (mkdir(builddir, 0777)) { > - (void)fprintf(stderr, "config: cannot create %s: %s\n", > - builddir, strerror(errno)); > - exit(2); > - } > - } else if (!S_ISDIR(st.st_mode)) { > - (void)fprintf(stderr, "config: %s is not a directory\n", > - builddir); > - exit(2); > - } > - if (chdir(builddir) != 0) { > - (void)fprintf(stderr, "config: cannot change to %s\n", > - builddir); > - exit(2); > - } > - if (stat(srcdir, &st) != 0 || !S_ISDIR(st.st_mode)) { > - (void)fprintf(stderr, "config: %s is not a directory\n", > - srcdir); > - exit(2); > - } > + if (mkdir(builddir, 0777)) > + err(2, "cannot create %s:\n", builddir); Another trailing newline, also the ':' be removed. > + } else if (!S_ISDIR(st.st_mode)) > + errx(2, "%s is not a directory\n", builddir); Another trailing newline. You might also consider: errc(2, ENOTDIR, "%s", builddir); > + if (chdir(builddir) != 0) > + errx(2, "cannot change to %s\n", builddir); Another trailing newline. > + if (stat(srcdir, &st) != 0 || !S_ISDIR(st.st_mode)) > + errx(2, "%s is not a directory\n", srcdir); Another trailing newline, maybe use errc(). > > if (bflag) { > if (pledge("stdio rpath wpath cpath flock", NULL) == -1) > @@ -693,14 +675,10 @@ setupdirs(void) > goto reconfig; > > fp = fopen("Makefile", "w"); > - if (!fp) { > - (void)fprintf(stderr, "config: cannot create Makefile\n"); > -
Re: config(8): use {err,warn}{,x} instead of fprintf(stderr, ...)
On Sun, Oct 16, 2016 at 01:35:34PM +0200, Theo Buehler wrote: > On Sun, Oct 16, 2016 at 11:47:32AM +0200, Theo Buehler wrote: > > Many files already include and there's a mix of hand-rolled > > warning messages and there's incorrect usage warn("config: ..."). This > > is a first sweep at unifying them. > > > > In mkheaders.c, there is an err() function, rename it to emitwarn() > > since there are emit* functions already and it is non-fatal. > > Here's a slightly improved version: I failed to remove a few newlines in > warning/error strings in main.c. Looks good to me, but see some nits below. > > Index: main.c > === > RCS file: /cvs/src/usr.sbin/config/main.c,v > retrieving revision 1.55 > diff -u -p -r1.55 main.c > --- main.c16 Oct 2016 09:36:46 - 1.55 > +++ main.c16 Oct 2016 11:25:20 - > @@ -75,7 +75,7 @@ static void optiondelta(void); > > int verbose; > > -void > +__dead void > usage(void) The usage declaration in ukc.c needs updating too. __dead void usage(void); > { > extern char *__progname; > @@ -662,26 +656,14 @@ setupdirs(void) > builddir = defbuilddir; > > if (stat(builddir, &st) != 0) { > - if (mkdir(builddir, 0777)) { > - (void)fprintf(stderr, "config: cannot create %s: %s\n", > - builddir, strerror(errno)); > - exit(2); > - } > - } else if (!S_ISDIR(st.st_mode)) { > - (void)fprintf(stderr, "config: %s is not a directory\n", > - builddir); > - exit(2); > - } > - if (chdir(builddir) != 0) { > - (void)fprintf(stderr, "config: cannot change to %s\n", > - builddir); > - exit(2); > - } > - if (stat(srcdir, &st) != 0 || !S_ISDIR(st.st_mode)) { > - (void)fprintf(stderr, "config: %s is not a directory\n", > - srcdir); > - exit(2); > - } > + if (mkdir(builddir, 0777)) > + err(2, "cannot create %s:", builddir); No ":" at the end of format. This will be added by err() automatically. > + } else if (!S_ISDIR(st.st_mode)) > + errx(2, "%s is not a directory", builddir); > + if (chdir(builddir) != 0) > + errx(2, "cannot change to %s", builddir); > + if (stat(srcdir, &st) != 0 || !S_ISDIR(st.st_mode)) > + errx(2, "%s is not a directory", srcdir); > > if (bflag) { > if (pledge("stdio rpath wpath cpath flock", NULL) == -1) > Index: mkheaders.c > === > RCS file: /cvs/src/usr.sbin/config/mkheaders.c,v > retrieving revision 1.21 > diff -u -p -r1.21 mkheaders.c > --- mkheaders.c 16 Jan 2015 06:40:16 - 1.21 > +++ mkheaders.c 16 Oct 2016 11:25:20 - > @@ -102,22 +103,21 @@ emitcnt(struct nvlist *head) > nv = nv->nv_next; > } > if (ferror(fp)) > - return (err("read", fname, fp)); > + return (emitwarn("read", fname, fp)); > (void)fclose(fp); > if (nv == NULL) > return (0); > writeit: > if ((fp = fopen(fname, "w")) == NULL) { > - (void)fprintf(stderr, "config: cannot write %s: %s\n", > - fname, strerror(errno)); > + warn("cannot write %s", fname); > return (1); return(emitwarn("writ", fname, NULL)); ? > } > for (nv = head; nv != NULL; nv = nv->nv_next) > if (fprintf(fp, "#define\t%s\t%d\n", > cntname(nv->nv_name), nv->nv_int) < 0) > - return (err("writ", fname, fp)); > + return (emitwarn("writ", fname, fp)); > if (fclose(fp)) > - return (err("writ", fname, NULL)); > + return (emitwarn("writ", fname, NULL)); > return (0); > } > > @@ -175,23 +175,21 @@ writeit: >* They're different, or the file doesn't exist. >*/ > if ((fp = fopen(fname, "w")) == NULL) { > - (void)fprintf(stderr, "config: cannot write %s: %s\n", > - fname, strerror(errno)); > + warn("cannot write %s", fname); > return (1); return(emitwarn("writ", fname, NULL)); ? > } > if (fprintf(fp, "%s", new_contents) < 0) > - return (err("writ", fname, fp)); > + return (emitwarn("writ", fname, fp)); > if (fclose(fp)) > - return (err("writ", fname, fp)); > + return (emitwarn("writ", fname, fp)); > return (0); > } > > static int > -err(const char *what, char *fname, FILE *fp) > +emitwarn(const char *what, char *fname, FILE *fp) > { > > - (void)fprintf(stderr, "config: error %sing %s: %s\n", > - what, fname, strerror(errno)); > + warn("error %sing %s", what, fname); > if (fp) >
Re: config(8): use {err,warn}{,x} instead of fprintf(stderr, ...)
On Sun, Oct 16, 2016 at 11:47:32AM +0200, Theo Buehler wrote: > Many files already include and there's a mix of hand-rolled > warning messages and there's incorrect usage warn("config: ..."). This > is a first sweep at unifying them. > > In mkheaders.c, there is an err() function, rename it to emitwarn() > since there are emit* functions already and it is non-fatal. Here's a slightly improved version: I failed to remove a few newlines in warning/error strings in main.c. Index: main.c === RCS file: /cvs/src/usr.sbin/config/main.c,v retrieving revision 1.55 diff -u -p -r1.55 main.c --- main.c 16 Oct 2016 09:36:46 - 1.55 +++ main.c 16 Oct 2016 11:25:20 - @@ -75,7 +75,7 @@ static void optiondelta(void); intverbose; -void +__dead void usage(void) { extern char *__progname; @@ -169,19 +169,15 @@ main(int argc, char *argv[]) if (eflag) { #ifdef MAKE_BOOTSTRAP - fprintf(stderr, "config: UKC not available in this binary\n"); - exit(1); + errx(1, "UKC not available in this binary"); #else return (ukc(argv[0], outfile, uflag, fflag)); #endif } conffile = (argc == 1) ? argv[0] : "CONFIG"; - if (firstfile(conffile)) { - (void)fprintf(stderr, "config: cannot read %s: %s\n", - conffile, strerror(errno)); - exit(2); - } + if (firstfile(conffile)) + err(2, "cannot read %s", conffile); /* * Init variables. @@ -246,8 +242,7 @@ main(int argc, char *argv[]) defmaxusers); maxusers = defmaxusers; } else { - (void)fprintf(stderr, - "config: need \"maxusers\" line\n"); + warnx("need \"maxusers\" line"); errors++; } } @@ -269,7 +264,7 @@ main(int argc, char *argv[]) mkioconf()) stop(); optiondelta(); - exit(0); + return (0); } static int @@ -278,11 +273,11 @@ mksymlink(const char *value, const char int ret = 0; if (remove(path) && errno != ENOENT) { - warn("config: remove(%s)", path); + warn("remove(%s)", path); ret = 1; } if (symlink(value, path)) { - warn("config: symlink(%s -> %s)", path, value); + warn("symlink(%s -> %s)", path, value); ret = 1; } return (ret); @@ -624,8 +619,7 @@ badstar(void) continue; foundstar: if (ht_lookup(needcnttab, d->d_name)) { - (void)fprintf(stderr, - "config: %s's cannot be *'d until its driver is fixed\n", + warnx("%s's cannot be *'d until its driver is fixed", d->d_name); errs++; continue; @@ -662,26 +656,14 @@ setupdirs(void) builddir = defbuilddir; if (stat(builddir, &st) != 0) { - if (mkdir(builddir, 0777)) { - (void)fprintf(stderr, "config: cannot create %s: %s\n", - builddir, strerror(errno)); - exit(2); - } - } else if (!S_ISDIR(st.st_mode)) { - (void)fprintf(stderr, "config: %s is not a directory\n", - builddir); - exit(2); - } - if (chdir(builddir) != 0) { - (void)fprintf(stderr, "config: cannot change to %s\n", - builddir); - exit(2); - } - if (stat(srcdir, &st) != 0 || !S_ISDIR(st.st_mode)) { - (void)fprintf(stderr, "config: %s is not a directory\n", - srcdir); - exit(2); - } + if (mkdir(builddir, 0777)) + err(2, "cannot create %s:", builddir); + } else if (!S_ISDIR(st.st_mode)) + errx(2, "%s is not a directory", builddir); + if (chdir(builddir) != 0) + errx(2, "cannot change to %s", builddir); + if (stat(srcdir, &st) != 0 || !S_ISDIR(st.st_mode)) + errx(2, "%s is not a directory", srcdir); if (bflag) { if (pledge("stdio rpath wpath cpath flock", NULL) == -1) @@ -693,14 +675,10 @@ setupdirs(void) goto reconfig; fp = fopen("Makefile", "w"); - if (!fp) { - (void)fprintf(stderr, "config: cannot create Makefile\n"); - exit(2); - } - if (fprintf(fp, ".include \"../Makefile.inc\"\n") < 0) { - (void)fprintf(stderr, "config: cannot create Makefile\n"); - exit(2); - } + if (!fp) + errx(2, "cannot create Makefile"); + if
Re: config(8): use {err,warn}{,x} instead of fprintf(stderr, ...)
Theo Buehler writes: > Many files already include and there's a mix of hand-rolled > warning messages and there's incorrect usage warn("config: ..."). This > is a first sweep at unifying them. > > In mkheaders.c, there is an err() function, rename it to emitwarn() > since there are emit* functions already and it is non-fatal. ok -- jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
config(8): use {err,warn}{,x} instead of fprintf(stderr, ...)
Many files already include and there's a mix of hand-rolled warning messages and there's incorrect usage warn("config: ..."). This is a first sweep at unifying them. In mkheaders.c, there is an err() function, rename it to emitwarn() since there are emit* functions already and it is non-fatal. Index: main.c === RCS file: /cvs/src/usr.sbin/config/main.c,v retrieving revision 1.55 diff -u -p -r1.55 main.c --- main.c 16 Oct 2016 09:36:46 - 1.55 +++ main.c 16 Oct 2016 09:42:01 - @@ -75,7 +75,7 @@ static void optiondelta(void); intverbose; -void +__dead void usage(void) { extern char *__progname; @@ -169,19 +169,15 @@ main(int argc, char *argv[]) if (eflag) { #ifdef MAKE_BOOTSTRAP - fprintf(stderr, "config: UKC not available in this binary\n"); - exit(1); + errx(1, "UKC not available in this binary"); #else return (ukc(argv[0], outfile, uflag, fflag)); #endif } conffile = (argc == 1) ? argv[0] : "CONFIG"; - if (firstfile(conffile)) { - (void)fprintf(stderr, "config: cannot read %s: %s\n", - conffile, strerror(errno)); - exit(2); - } + if (firstfile(conffile)) + err(2, "cannot read %s\n", conffile); /* * Init variables. @@ -246,8 +242,7 @@ main(int argc, char *argv[]) defmaxusers); maxusers = defmaxusers; } else { - (void)fprintf(stderr, - "config: need \"maxusers\" line\n"); + warnx("need \"maxusers\" line"); errors++; } } @@ -269,7 +264,7 @@ main(int argc, char *argv[]) mkioconf()) stop(); optiondelta(); - exit(0); + return (0); } static int @@ -278,11 +273,11 @@ mksymlink(const char *value, const char int ret = 0; if (remove(path) && errno != ENOENT) { - warn("config: remove(%s)", path); + warn("remove(%s)", path); ret = 1; } if (symlink(value, path)) { - warn("config: symlink(%s -> %s)", path, value); + warn("symlink(%s -> %s)", path, value); ret = 1; } return (ret); @@ -624,8 +619,7 @@ badstar(void) continue; foundstar: if (ht_lookup(needcnttab, d->d_name)) { - (void)fprintf(stderr, - "config: %s's cannot be *'d until its driver is fixed\n", + warnx("%s's cannot be *'d until its driver is fixed", d->d_name); errs++; continue; @@ -662,26 +656,14 @@ setupdirs(void) builddir = defbuilddir; if (stat(builddir, &st) != 0) { - if (mkdir(builddir, 0777)) { - (void)fprintf(stderr, "config: cannot create %s: %s\n", - builddir, strerror(errno)); - exit(2); - } - } else if (!S_ISDIR(st.st_mode)) { - (void)fprintf(stderr, "config: %s is not a directory\n", - builddir); - exit(2); - } - if (chdir(builddir) != 0) { - (void)fprintf(stderr, "config: cannot change to %s\n", - builddir); - exit(2); - } - if (stat(srcdir, &st) != 0 || !S_ISDIR(st.st_mode)) { - (void)fprintf(stderr, "config: %s is not a directory\n", - srcdir); - exit(2); - } + if (mkdir(builddir, 0777)) + err(2, "cannot create %s:\n", builddir); + } else if (!S_ISDIR(st.st_mode)) + errx(2, "%s is not a directory\n", builddir); + if (chdir(builddir) != 0) + errx(2, "cannot change to %s\n", builddir); + if (stat(srcdir, &st) != 0 || !S_ISDIR(st.st_mode)) + errx(2, "%s is not a directory\n", srcdir); if (bflag) { if (pledge("stdio rpath wpath cpath flock", NULL) == -1) @@ -693,14 +675,10 @@ setupdirs(void) goto reconfig; fp = fopen("Makefile", "w"); - if (!fp) { - (void)fprintf(stderr, "config: cannot create Makefile\n"); - exit(2); - } - if (fprintf(fp, ".include \"../Makefile.inc\"\n") < 0) { - (void)fprintf(stderr, "config: cannot create Makefile\n"); - exit(2); - } + if (!fp) + errx(2, "cannot create Makefile"); + if (fprintf(fp, ".include \"../Makefile.inc\"\n") < 0) + errx(2, "cannot create Makefile"); fclose(fp); reconfig: Index: mkheaders.c ===