[libvirt] [PATCH] Fix virFileReadLimFD/virFileReadAll to handle EINTR

[Mark McLoughlin]

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.