Re: [libvirt] PATCH: 5/5: Make all code use virExec

"Daniel P. Berrange" <berrange(a)redhat.com&gt; wrote: You did, indeed. Even nicer ;-) ... I was going to ask why you are using saferead, then realized that *I* suggested the s/read+EINTR/saferead/ change. Now, while re-reviewing this, I wondered if we could get rid of the 8KB stack buffer and encapsulate the above loop -- at the expense of allocating the memory instead -- by using e.g., virFileReadAll. But virFileReadAll operates on a file name, not a file descriptor. So I wrote/factored a couple of wrappers, and now, with the following patch, you can use this: char *help = NULL; enum { MAX_HELP_OUTPUT_SIZE = 8192 }; int len = virFileReadLimFD(newstdout, MAX_HELP_OUTPUT_SIZE, &help); if (len < 0) goto ... ... Then add this somewhere after done with "help": VIR_FREE(help); Go ahead and commit your changes. After you do that, I'll rebase this and propose a complete patch. From: Jim Meyering <meyering(a)redhat.com&gt; Date: Thu, 28 Aug 2008 15:54:03 +0200 Subject: [PATCH] util.c: add a file-descriptor-based wrapper for fread_file_lim * src/util.c (virFileReadLimFP): New function. (__virFileReadLimFD): New function. * src/util.h (__virFileReadLimFD): Declare. (virFileReadLimFD): Define. (virFileReadAll): Rewrite to use virFileReadLimFP. --- src/util.c | 71 +++++++++++++++++++++++++++++++++++++++-------------------- src/util.h | 7 +++-- 2 files changed, 51 insertions(+), 27 deletions(-) diff --git a/src/util.c b/src/util.c index a81af07..fd30778 100644 --- a/src/util.c +++ b/src/util.c @@ -510,40 +510,63 @@ fread_file_lim (FILE *stream, size_t max_len, size_t *length) return NULL; } -int __virFileReadAll(const char *path, int maxlen, char **buf) +/* A wrapper around fread_file_lim that maps a failure due to + exceeding the maximum size limitation to EOVERFLOW. */ +static int virFileReadLimFP(FILE *fp, int maxlen, char **buf) { - FILE *fh; - int ret = -1; size_t len; - char *s; + char *s = fread_file_lim (fp, maxlen+1, &len); + if (s == NULL) + return -1; + if (len > maxlen || (int)len != len) { + VIR_FREE(s); + /* There was at least one byte more than MAXLEN. + Set errno accordingly. */ + errno = EOVERFLOW; + return -1; + } + *buf = s; + 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 (0 <= fd) { + 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; +} - if (!(fh = fopen(path, "r"))) { +int __virFileReadAll(const char *path, int maxlen, char **buf) +{ + FILE *fh = fopen(path, "r"); + if (fh == NULL) { virLog("Failed to open file '%s': %s\n", path, strerror(errno)); - goto error; + return -1; } - s = fread_file_lim(fh, maxlen+1, &len); - if (s == NULL) { + int len = virFileReadLimFP (fh, maxlen, buf); + fclose(fh); + if (len < 0) { virLog("Failed to read '%s': %s\n", path, strerror (errno)); - goto error; - } - - if (len > maxlen || (int)len != len) { - VIR_FREE(s); - virLog("File '%s' is too large %d, max %d\n", - path, (int)len, maxlen); - goto error; + return -1; } - *buf = s; - ret = len; - - error: - if (fh) - fclose(fh); - - return ret; + return len; } int virFileMatchesNameSuffix(const char *file, diff --git a/src/util.h b/src/util.h index 5151582..f2da006 100644 --- a/src/util.h +++ b/src/util.h @@ -45,9 +45,10 @@ int virExec(virConnectPtr conn, int flags); int virRun(virConnectPtr conn, const char *const*argv, int *status); -int __virFileReadAll(const char *path, - int maxlen, - char **buf); +int __virFileReadLimFD(int fd, int maxlen, char **buf); +#define virFileReadLimFD(fd,m,b) __virFileReadLimFD((fd),(m),(b)) + +int __virFileReadAll(const char *path, int maxlen, char **buf); #define virFileReadAll(p,m,b) __virFileReadAll((p),(m),(b)) int virFileMatchesNameSuffix(const char *file, -- 1.6.0.1.126.ga118