[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

[libvirt] [PATCH] util.c: add a file-descriptor-based wrapper for fread_file_lim



"Daniel P. Berrange" <berrange redhat com> wrote:
> On Thu, Aug 28, 2008 at 04:18:08PM +0200, Jim Meyering wrote:
>> "Daniel P. Berrange" <berrange redhat com> wrote:
>> > On Wed, Aug 20, 2008 at 06:40:37PM +0200, Jim Meyering wrote:
>> >> "Daniel P. Berrange" <berrange redhat com> 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 redhat com>
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 redhat com>
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, &micro) != 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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]