Re: config(8): use {err,warn}{,x} instead of fprintf(stderr, ...)

2016-10-16 Thread Todd C. Miller 
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, ) != 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, ) != 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, ) != 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, ...)

2016-10-16 Thread Martin Natano 
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, ) != 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, ) != 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, ) != 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, ...)

2016-10-16 Thread Theo Buehler 
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, ) != 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, ) != 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, ) != 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

config(8): use {err,warn}{,x} instead of fprintf(stderr, ...)

2016-10-16 Thread Theo Buehler 
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, ) != 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, ) != 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, ) != 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