"Daniel P. Berrange" <berrange@xxxxxxxxxx> wrote:
> On Thu, Aug 28, 2008 at 04:18:08PM +0200, Jim Meyering wrote:
>> "Daniel P. Berrange" <berrange@xxxxxxxxxx> wrote:
>> > On Wed, Aug 20, 2008 at 06:40:37PM +0200, Jim Meyering wrote:
>> >> "Daniel P. Berrange" <berrange@xxxxxxxxxx> wrote:
>> > +
>> > +
>> > + while (got < (sizeof(help)-1)) {
>> > + int len;
>> > + if ((len = saferead(newstdout, help+got, sizeof(help)-got-1)) < 0)
>> > + goto cleanup2;
>> > + if (!len)
>> > + break;
>> > + got += len;
>> > + }
>> > + help[got] = '\0';
>>
>>
>> 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);
>
> This sounds like a nice idea - the loop is rather unpleasant to read as
> it is. I'll commit my patch shortly
Here are two change sets:
The first adds two new file-reading functions in util.c
and makes the existing virFileReadAll use one of them.
The second implements the change suggested above.
>From f5fa2a2963ecc70022b30317a29b0f769a34fc8a Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering@xxxxxxxxxx>
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 cb03e7b..c724268 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 (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;
+}
- 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
>From e5771e49d5887e546db8fc47a7ad7785ea009eec Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering@xxxxxxxxxx>
Date: Fri, 29 Aug 2008 14:43:34 +0200
Subject: [PATCH] virFileReadLimFD, virFileReadLimFP: new functions; use them
* src/qemu_conf.c (qemudExtractVersionInfo): Use virFileReadLimFD
and VIR_FREE in place of an open-coded loop and a static buffer.
---
src/qemu_conf.c | 21 +++++++--------------
1 files changed, 7 insertions(+), 14 deletions(-)
diff --git a/src/qemu_conf.c b/src/qemu_conf.c
index bab2092..0686978 100644
--- a/src/qemu_conf.c
+++ b/src/qemu_conf.c
@@ -401,8 +401,7 @@ int qemudExtractVersionInfo(const char *qemu,
const char *const qemuenv[] = { "LC_ALL=C", NULL };
pid_t child;
int newstdout = -1;
- char help[8192]; /* Ought to be enough to hold QEMU help screen */
- int got = 0, ret = -1, status;
+ int ret = -1, status;
unsigned int major, minor, micro;
unsigned int version;
unsigned int flags = 0;
@@ -416,16 +415,11 @@ int qemudExtractVersionInfo(const char *qemu,
&child, -1, &newstdout, NULL, VIR_EXEC_NONE) < 0)
return -1;
-
- while (got < (sizeof(help)-1)) {
- int len;
- if ((len = saferead(newstdout, help+got, sizeof(help)-got-1)) < 0)
- goto cleanup2;
- if (!len)
- break;
- got += len;
- }
- help[got] = '\0';
+ char *help = NULL;
+ enum { MAX_HELP_OUTPUT_SIZE = 8192 };
+ int len = virFileReadLimFD(newstdout, MAX_HELP_OUTPUT_SIZE, &help);
+ if (len < 0)
+ goto cleanup2;
if (sscanf(help, "QEMU PC emulator version %u.%u.%u",
&major, &minor, µ) != 3) {
@@ -447,7 +441,6 @@ int qemudExtractVersionInfo(const char *qemu,
if (version >= 9000)
flags |= QEMUD_CMD_FLAG_VNC_COLON;
-
if (retversion)
*retversion = version;
if (retflags)
@@ -459,6 +452,7 @@ int qemudExtractVersionInfo(const char *qemu,
major, minor, micro, version, flags);
cleanup2:
+ VIR_FREE(help);
if (close(newstdout) < 0)
ret = -1;
@@ -1229,4 +1223,3 @@ int qemudBuildCommandLine(virConnectPtr conn,
#undef ADD_ARG_LIT
#undef ADD_ARG_SPACE
}
-
--
1.6.0.1.126.ga118
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list