On Mon, 2009-10-12 at 20:37 +0100, Daniel P. Berrange wrote: > The fread_file_lim() function uses fread() but never handles > EINTR results, causing unexpected failures when reading QEMU > help arg info. It was unneccessarily using FILE * instead > of plain UNIX file handles, which prevented use of saferead() Looks like the same thing Charles Duffy reported here: http://www.redhat.com/archives/libvir-list/2009-September/msg00662.html > * src/util/util.c: Switch fread_file_lim over to use saferead > instead of fread, remove FILE * use, and rename > --- > src/util/util.c | 45 ++++++++++++--------------------------------- > 1 files changed, 12 insertions(+), 33 deletions(-) > > diff --git a/src/util/util.c b/src/util/util.c > index e5135fc..98f8a14 100644 > --- a/src/util/util.c > +++ b/src/util/util.c > @@ -898,7 +898,7 @@ virExec(virConnectPtr conn, > number of bytes. If the length of the input is <= max_len, and > upon error while reading that data, it works just like fread_file. */ > static char * > -fread_file_lim (FILE *stream, size_t max_len, size_t *length) > +saferead_lim (int fd, size_t max_len, size_t *length) > { > char *buf = NULL; > size_t alloc = 0; > @@ -906,8 +906,8 @@ fread_file_lim (FILE *stream, size_t max_len, size_t *length) > int save_errno; > > for (;;) { > - size_t count; > - size_t requested; > + int count; > + int requested; > > if (size + BUFSIZ + 1 > alloc) { > alloc += alloc / 2; > @@ -923,12 +923,12 @@ fread_file_lim (FILE *stream, size_t max_len, size_t *length) > /* Ensure that (size + requested <= max_len); */ > requested = MIN (size < max_len ? max_len - size : 0, > alloc - size - 1); > - count = fread (buf + size, 1, requested, stream); > + count = saferead (fd, buf + size, requested); > size += count; > > if (count != requested || requested == 0) { > save_errno = errno; > - if (ferror (stream)) > + if (count < 0) > break; Perhaps you should move the save_errno assignment under this condition too, but not a big deal > buf[size] = '\0'; > *length = size; > @@ -941,12 +941,12 @@ fread_file_lim (FILE *stream, size_t max_len, size_t *length) > return NULL; > } > > -/* A wrapper around fread_file_lim that maps a failure due to > +/* A wrapper around saferead_lim that maps a failure due to > exceeding the maximum size limitation to EOVERFLOW. */ > -static int virFileReadLimFP(FILE *fp, int maxlen, char **buf) > +int virFileReadLimFD(int fd, int maxlen, char **buf) > { > size_t len; > - char *s = fread_file_lim (fp, maxlen+1, &len); > + char *s = saferead_lim (fd, maxlen+1, &len); > if (s == NULL) > return -1; > if (len > maxlen || (int)len != len) { > @@ -960,37 +960,16 @@ static int virFileReadLimFP(FILE *fp, int maxlen, char **buf) > return len; > } > > -/* Like virFileReadLimFP, but use a file descriptor rather than a FILE*. */ > -int virFileReadLimFD(int fd_arg, int maxlen, char **buf) > -{ > - int fd = dup (fd_arg); > - if (fd >= 0) { > - FILE *fp = fdopen (fd, "r"); > - if (fp) { > - int len = virFileReadLimFP (fp, maxlen, buf); > - int saved_errno = errno; > - fclose (fp); > - errno = saved_errno; > - return len; > - } else { > - int saved_errno = errno; > - close (fd); > - errno = saved_errno; > - } > - } > - return -1; > -} > - > int virFileReadAll(const char *path, int maxlen, char **buf) > { > - FILE *fh = fopen(path, "r"); > - if (fh == NULL) { > + int fd = open(path, O_RDONLY); > + if (fd < 0) { > virReportSystemError(NULL, errno, _("Failed to open file '%s'"), path); > return -1; > } > > - int len = virFileReadLimFP (fh, maxlen, buf); > - fclose(fh); > + int len = virFileReadLimFD(fd, maxlen, buf); > + close(fd); > if (len < 0) { > virReportSystemError(NULL, errno, _("Failed to read file '%s'"), path); > return -1; Looks good to me, ACK Cheers, Mark. -- Libvir-list mailing list Libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list