On Sun, Dec 04, 2016 at 09:55:09PM -0800, Michael Forney wrote:
> If we are just copying data from one file to another, we don't need to
> fill a complete buffer, just read a chunk at a time, and write it to the
> output.
> ---
>  cat.c            | 34 ++++++++++------------------------
>  libutil/concat.c | 24 ++++++++++++++----------
>  libutil/cp.c     | 42 ++++++++++++++++--------------------------
>  sponge.c         | 31 +++++++++++++++++--------------
>  text.h           |  1 -
>  util.h           |  1 +
>  xinstall.c       | 25 ++++++++++++-------------
>  7 files changed, 70 insertions(+), 88 deletions(-)
> 
> diff --git a/cat.c b/cat.c
> index e3741aa..55585dd 100644
> --- a/cat.c
> +++ b/cat.c
> @@ -1,22 +1,11 @@
>  /* See LICENSE file for copyright and license details. */
> -#include <stdio.h>
> +#include <fcntl.h>
>  #include <string.h>
>  #include <unistd.h>
>  
> -#include "text.h"
>  #include "util.h"
>  
>  static void
> -uconcat(FILE *fp1, const char *s1, FILE *fp2, const char *s2)
> -{
> -     int c;
> -
> -     setbuf(fp2, NULL);
> -     while ((c = getc(fp1)) != EOF)
> -             putc(c, fp2);
> -}
> -
> -static void
>  usage(void)
>  {
>       eprintf("usage: %s [-u] [file ...]\n", argv0);
> @@ -25,37 +14,34 @@ usage(void)
>  int
>  main(int argc, char *argv[])
>  {
> -     FILE *fp;
> -     int ret = 0;
> -     void (*cat)(FILE *, const char *, FILE *, const char *) = &concat;
> +     int fd, ret = 0;
>  
>       ARGBEGIN {
>       case 'u':
> -             cat = &uconcat;
>               break;
>       default:
>               usage();
>       } ARGEND
>  
>       if (!argc) {
> -             cat(stdin, "<stdin>", stdout, "<stdout>");
> +             if (concat(0, "<stdin>", 1, "<stdout>") < 0)
> +                     ret = 1;
>       } else {
>               for (; *argv; argc--, argv++) {
>                       if (!strcmp(*argv, "-")) {
>                               *argv = "<stdin>";
> -                             fp = stdin;
> -                     } else if (!(fp = fopen(*argv, "r"))) {
> -                             weprintf("fopen %s:", *argv);
> +                             fd = 0;
> +                     } else if ((fd = open(*argv, O_RDONLY)) < 0) {
> +                             weprintf("open %s:", *argv);
>                               ret = 1;
>                               continue;
>                       }
> -                     cat(fp, *argv, stdout, "<stdout>");
> -                     if (fp != stdin && fshut(fp, *argv))
> +                     if (concat(fd, *argv, 1, "<stdout>") < 0)
>                               ret = 1;
> +                     if (fd != 0)
> +                             close(fd);
>               }
>       }
>  
> -     ret |= fshut(stdin, "<stdin>") | fshut(stdout, "<stdout>");
> -
>       return ret;
>  }
> diff --git a/libutil/concat.c b/libutil/concat.c
> index fad9471..3f617e2 100644
> --- a/libutil/concat.c
> +++ b/libutil/concat.c
> @@ -1,19 +1,23 @@
>  /* See LICENSE file for copyright and license details. */
> -#include <stdio.h>
> +#include <unistd.h>
>  
> -#include "../text.h"
>  #include "../util.h"
>  
> -void
> -concat(FILE *fp1, const char *s1, FILE *fp2, const char *s2)
> +int
> +concat(int f1, const char *s1, int f2, const char *s2)
>  {
>       char buf[BUFSIZ];
> -     size_t n;
> +     ssize_t n;
>  
> -     while ((n = fread(buf, 1, sizeof(buf), fp1))) {
> -             fwrite(buf, 1, n, fp2);
> -
> -             if (feof(fp1) || ferror(fp1) || ferror(fp2))
> -                     break;
> +     while ((n = read(f1, buf, sizeof(buf))) > 0) {
> +             if (writeall(f2, buf, n) < 0) {
> +                     weprintf("write %s:", s2);
> +                     return -1;
> +             }
> +     }
> +     if (n < 0) {
> +             weprintf("read %s:", s1);
> +             return -1;
>       }
> +     return 0;
>  }
> diff --git a/libutil/cp.c b/libutil/cp.c
> index c398962..8cd0a7d 100644
> --- a/libutil/cp.c
> +++ b/libutil/cp.c

The stdio.h #include can now also be removed from libutil/cp.c.


> @@ -12,7 +12,6 @@
>  #include <utime.h>
>  
>  #include "../fs.h"
> -#include "../text.h"
>  #include "../util.h"
>  
>  int cp_aflag  = 0;
> @@ -27,7 +26,7 @@ int
>  cp(const char *s1, const char *s2, int depth)
>  {
>       DIR *dp;
> -     FILE *f1, *f2;
> +     int f1, f2;
>       struct dirent *d;
>       struct stat st;
>       struct timespec times[2];
> @@ -112,43 +111,34 @@ cp(const char *s1, const char *s2, int depth)
>                       return 0;
>               }
>       } else {
> -             if (!(f1 = fopen(s1, "r"))) {
> -                     weprintf("fopen %s:", s1);
> +             if ((f1 = open(s1, O_RDONLY)) < 0) {
> +                     weprintf("open %s:", s1);
>                       cp_status = 1;
>                       return 0;
>               }
> -             if (!(f2 = fopen(s2, "w"))) {
> -                     if (cp_fflag) {
> -                             if (unlink(s2) < 0 && errno != ENOENT) {
> -                                     weprintf("unlink %s:", s2);
> -                                     cp_status = 1;
> -                                     return 0;
> -                             } else if (!(f2 = fopen(s2, "w"))) {
> -                                     weprintf("fopen %s:", s2);
> -                                     cp_status = 1;
> -                                     return 0;
> -                             }
> -                     } else {
> -                             weprintf("fopen %s:", s2);
> +             if ((f2 = creat(s2, st.st_mode)) < 0 && cp_fflag) {
> +                     if (unlink(s2) < 0 && errno != ENOENT) {
> +                             weprintf("unlink %s:", s2);
>                               cp_status = 1;
>                               return 0;
>                       }
> +                     f2 = creat(s2, st.st_mode);
>               }
> -             concat(f1, s1, f2, s2);
> -
> -             /* preserve permissions by default */
> -             fchmod(fileno(f2), st.st_mode);
> -
> -             if (fclose(f2) == EOF) {
> -                     weprintf("fclose %s:", s2);
> +             if (f2 < 0) {
> +                     weprintf("creat %s:", s2);
>                       cp_status = 1;
>                       return 0;
>               }
> -             if (fclose(f1) == EOF) {
> -                     weprintf("fclose %s:", s1);
> +             if (concat(f1, s1, f2, s2) < 0) {
>                       cp_status = 1;
>                       return 0;
>               }
> +
> +             /* preserve permissions by default */
> +             fchmod(f2, st.st_mode);
> +
> +             close(f1);
> +             close(f2);
>       }
>  
>       if (cp_aflag || cp_pflag) {
> diff --git a/sponge.c b/sponge.c
> index baeac7f..da8b28c 100644
> --- a/sponge.c
> +++ b/sponge.c
> @@ -1,7 +1,8 @@
>  /* See LICENSE file for copyright and license details. */
> -#include <stdio.h>
> +#include <fcntl.h>
> +#include <stdlib.h>
> +#include <unistd.h>
>  
> -#include "text.h"
>  #include "util.h"
>  
>  static void
> @@ -13,24 +14,26 @@ usage(void)
>  int
>  main(int argc, char *argv[])
>  {
> -     FILE *fp, *tmpfp;
> -     int ret = 0;
> +     char tmp[] = "/tmp/sponge-XXXXXX";
> +     int fd, tmpfd;
>  
>       argv0 = argv[0], argc--, argv++;
>  
>       if (argc != 1)
>               usage();
>  
> -     if (!(tmpfp = tmpfile()))
> -             eprintf("tmpfile:");
> -     concat(stdin, "<stdin>", tmpfp, "<tmpfile>");
> -     rewind(tmpfp);
> +     if ((tmpfd = mkstemp(tmp)) < 0)
> +             eprintf("mkstemp:");
> +     unlink(tmp);
> +     if (concat(0, "<stdin>", tmpfd, "<tmpfile>") < 0)
> +             return 1;
> +     if (lseek(tmpfd, 0, SEEK_SET) < 0)
> +             eprintf("lseek:");
>  
> -     if (!(fp = fopen(argv[0], "w")))
> -             eprintf("fopen %s:", argv[0]);
> -     concat(tmpfp, "<tmpfile>", fp, argv[0]);
> +     if ((fd = creat(argv[0], 0666)) < 0)

Mode 0660 may be the safer option here.

Other than that it LGTM!


Cheers,

Silvan

> +             eprintf("creat %s:", argv[0]);
> +     if (concat(tmpfd, "<tmpfile>", fd, argv[0]) < 0)
> +             return 1;
>  
> -     ret |= fshut(fp, argv[0]) | fshut(tmpfp, "<tmpfile>");
> -
> -     return ret;
> +     return 0;
>  }
> diff --git a/text.h b/text.h
> index bceda52..9858592 100644
> --- a/text.h
> +++ b/text.h
> @@ -13,5 +13,4 @@ struct linebuf {
>  #define EMPTY_LINEBUF {NULL, 0, 0,}
>  void getlines(FILE *, struct linebuf *);
>  
> -void concat(FILE *, const char *, FILE *, const char *);
>  int linecmp(struct line *, struct line *);
> diff --git a/util.h b/util.h
> index eaad3ce..968a3af 100644
> --- a/util.h
> +++ b/util.h
> @@ -64,6 +64,7 @@ int eregcomp(regex_t *, const char *, int);
>  
>  /* io */
>  ssize_t writeall(int, const void *, size_t);
> +int concat(int, const char *, int, const char *);
>  
>  /* misc */
>  void enmasse(int, char **, int (*)(const char *, const char *, int));
> diff --git a/xinstall.c b/xinstall.c
> index 5a0e390..3d24a1a 100644
> --- a/xinstall.c
> +++ b/xinstall.c
> @@ -2,6 +2,7 @@
>  #include <grp.h>
>  #include <pwd.h>
>  #include <errno.h>
> +#include <fcntl.h>
>  #include <unistd.h>
>  #include <stdlib.h>
>  #include <string.h>
> @@ -10,7 +11,6 @@
>  #include <sys/wait.h>
>  
>  #include "util.h"
> -#include "text.h"
>  
>  static int Dflag = 0;
>  static int sflag = 0;
> @@ -61,7 +61,7 @@ static int
>  install(const char *s1, const char *s2, int depth)
>  {
>       DIR *dp;
> -     FILE *f1, *f2;
> +     int f1, f2;
>       struct dirent *d;
>       struct stat st;
>       ssize_t r;
> @@ -109,23 +109,22 @@ install(const char *s1, const char *s2, int depth)
>               else if (mknod(s2, (st.st_mode & ~07777) | mode, st.st_rdev) < 
> 0)
>                       eprintf("mknod %s:", s2);
>       } else {
> -             if (!(f1 = fopen(s1, "r")))
> -                     eprintf("fopen %s:", s1);
> -             if (!(f2 = fopen(s2, "w"))) {
> +             if ((f1 = open(s1, O_RDONLY)) < 0)
> +                     eprintf("open %s:", s1);
> +             if ((f2 = creat(s2, 0600)) < 0) {
>                       if (unlink(s2) < 0 && errno != ENOENT)
>                               eprintf("unlink %s:", s2);
> -                     else if (!(f2 = fopen(s2, "w")))
> -                             eprintf("fopen %s:", s2);
> +                     if ((f2 = creat(s2, 0600)) < 0)
> +                             eprintf("creat %s:", s2);
>               }
> -             concat(f1, s1, f2, s2);
> +             if (concat(f1, s1, f2, s2) < 0)
> +                     exit(1);
>  
> -             if (fchmod(fileno(f2), mode) < 0)
> +             if (fchmod(f2, mode) < 0)
>                       eprintf("fchmod %s:", s2);
>  
> -             if (fclose(f2) == EOF)
> -                     eprintf("fclose %s:", s2);
> -             if (fclose(f1) == EOF)
> -                     eprintf("fclose %s:", s1);
> +             close(f1);
> +             close(f2);
>  
>               if (sflag)
>                       strip(s2);
> -- 
> 2.11.0
> 
> 

Reply via email to