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

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



"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:
>> > This final patch switches over all places which do fork()/exec() to use the
>> > new enhanced virExec() code. A fair amount of code is deleted, but that's
>> > not the whole story - the new impl is more robust that most of the existing
>> > code we're deleting here.
>>
>> Nice clean-up!
>
> I believe I've addressed all the comments you had - its quite a significant
> change from the previous patch. I was in fact able to simplify the bridge.c
> file even more than I did before.

You did, indeed.  Even nicer ;-)

...
> diff -r c3d345142e1b src/qemu_conf.c
> --- a/src/qemu_conf.c	Wed Aug 27 12:45:11 2008 +0100
> +++ b/src/qemu_conf.c	Wed Aug 27 14:09:26 2008 +0100
> @@ -394,124 +394,100 @@
>  }
>
>
> -int qemudExtractVersionInfo(const char *qemu, int *version, int *flags) {
> +int qemudExtractVersionInfo(const char *qemu,
> +                            unsigned int *retversion,
> +                            unsigned int *retflags) {
> +    const char *const qemuarg[] = { qemu, "-help", NULL };
> +    const char *const qemuenv[] = { "LC_ALL=C", NULL };
>      pid_t child;
> -    int newstdout[2];
> +    int newstdout = -1;
> +    char help[8192]; /* Ought to be enough to hold QEMU help screen */
> +    int got = 0, ret = -1, status;
> +    unsigned int major, minor, micro;
> +    unsigned int version;
> +    unsigned int flags = 0;
>
> -    if (flags)
> -        *flags = 0;
> -    if (version)
> -        *version = 0;
> +    if (retflags)
> +        *retflags = 0;
> +    if (retversion)
> +        *retversion = 0;
>
> -    if (pipe(newstdout) < 0) {
> +    if (virExec(NULL, qemuarg, qemuenv, NULL,
> +                &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';


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 620584c1effef8882687c0560dcfaec5a4614c25 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 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


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