[libvirt] [PATCHv2 1/2] build: avoid close, system

Eric Blake eblake at redhat.com
Sat Jan 29 00:02:08 UTC 2011


* src/fdstream.c (virFDStreamOpenFile, virFDStreamCreateFile):
Use VIR_FORCE_CLOSE instead of close.
* tests/commandtest.c (mymain): Likewise.
* tools/virsh.c (editFile): Use virCommand instead of system.
* src/util/util.c (__virExec): Special case preservation of std
file descriptors to child.
---

v2: make virCommand behave more like system.  So I didn't do
any signal handling like system, but at least now you can
actually edit interactively.  virCommandRun doesn't like
interactive sessions, so I have to use virCommandRunAsync
followed by virCommandWait.  I also had to fix virExec.

 src/fdstream.c      |    6 ++--
 src/util/util.c     |   12 +++++----
 tests/commandtest.c |   12 ++++++---
 tools/virsh.c       |   65 ++++++++++++++++++++++++++------------------------
 4 files changed, 52 insertions(+), 43 deletions(-)

diff --git a/src/fdstream.c b/src/fdstream.c
index 952bd69..701fafc 100644
--- a/src/fdstream.c
+++ b/src/fdstream.c
@@ -1,7 +1,7 @@
 /*
  * fdstream.h: generic streams impl for file descriptors
  *
- * Copyright (C) 2009-2010 Red Hat, Inc.
+ * Copyright (C) 2009-2011 Red Hat, Inc.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Lesser General Public
@@ -452,7 +452,7 @@ int virFDStreamOpenFile(virStreamPtr st,
     return 0;

 error:
-    close(fd);
+    VIR_FORCE_CLOSE(fd);
     return -1;
 }

@@ -498,6 +498,6 @@ int virFDStreamCreateFile(virStreamPtr st,
     return 0;

 error:
-    close(fd);
+    VIR_FORCE_CLOSE(fd);
     return -1;
 }
diff --git a/src/util/util.c b/src/util/util.c
index f412a83..af14b2e 100644
--- a/src/util/util.c
+++ b/src/util/util.c
@@ -593,14 +593,16 @@ __virExec(const char *const*argv,
         goto fork_error;
     }

-    VIR_FORCE_CLOSE(infd);
+    if (infd != STDIN_FILENO)
+        VIR_FORCE_CLOSE(infd);
     VIR_FORCE_CLOSE(null);
-    tmpfd = childout;   /* preserve childout value */
-    VIR_FORCE_CLOSE(tmpfd);
-    if (childerr > 0 &&
+    if (childout != STDOUT_FILENO) {
+        tmpfd = childout;   /* preserve childout value */
+        VIR_FORCE_CLOSE(tmpfd);
+    }
+    if (childerr > STDERR_FILENO &&
         childerr != childout) {
         VIR_FORCE_CLOSE(childerr);
-        childout = -1;
     }

     /* Initialize full logging for a while */
diff --git a/tests/commandtest.c b/tests/commandtest.c
index 38a7816..7157c51 100644
--- a/tests/commandtest.c
+++ b/tests/commandtest.c
@@ -1,7 +1,7 @@
 /*
  * commandtest.c: Test the libCommand API
  *
- * Copyright (C) 2010 Red Hat, Inc.
+ * Copyright (C) 2010-2011 Red Hat, Inc.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Lesser General Public
@@ -714,6 +714,7 @@ mymain(int argc, char **argv)
 {
     int ret = 0;
     char cwd[PATH_MAX];
+    int fd;

     abs_srcdir = getenv("abs_srcdir");
     if (!abs_srcdir)
@@ -731,9 +732,12 @@ mymain(int argc, char **argv)

     /* Kill off any inherited fds that might interfere with our
      * testing.  */
-    close(3);
-    close(4);
-    close(5);
+    fd = 3;
+    VIR_FORCE_CLOSE(fd);
+    fd = 4;
+    VIR_FORCE_CLOSE(fd);
+    fd = 5;
+    VIR_FORCE_CLOSE(fd);

     virInitialize();

diff --git a/tools/virsh.c b/tools/virsh.c
index d411381..59d099e 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -56,6 +56,7 @@
 #include "../daemon/event.h"
 #include "configmake.h"
 #include "threads.h"
+#include "command.h"

 static char *progname;

@@ -9354,50 +9355,52 @@ static int
 editFile (vshControl *ctl, const char *filename)
 {
     const char *editor;
-    char *command;
-    int command_ret;
+    virCommandPtr cmd;
+    int ret = -1;
+    int outfd = STDOUT_FILENO;
+    int errfd = STDERR_FILENO;

     editor = getenv ("VISUAL");
-    if (!editor) editor = getenv ("EDITOR");
-    if (!editor) editor = "vi"; /* could be cruel & default to ed(1) here */
+    if (!editor)
+        editor = getenv ("EDITOR");
+    if (!editor)
+        editor = "vi"; /* could be cruel & default to ed(1) here */

     /* Check that filename doesn't contain shell meta-characters, and
      * if it does, refuse to run.  Follow the Unix conventions for
      * EDITOR: the user can intentionally specify command options, so
      * we don't protect any shell metacharacters there.  Lots more
      * than virsh will misbehave if EDITOR has bogus contents (which
-     * is why sudo scrubs it by default).
+     * is why sudo scrubs it by default).  Conversely, if the editor
+     * is safe, we can run it directly rather than wasting a shell.
      */
-    if (strspn (filename, ACCEPTED_CHARS) != strlen (filename)) {
-        vshError(ctl,
-                 _("%s: temporary filename contains shell meta or other "
-                   "unacceptable characters (is $TMPDIR wrong?)"),
-                 filename);
-        return -1;
+    if (strspn (editor, ACCEPTED_CHARS) != strlen (editor)) {
+        if (strspn (filename, ACCEPTED_CHARS) != strlen (filename)) {
+            vshError(ctl,
+                     _("%s: temporary filename contains shell meta or other "
+                       "unacceptable characters (is $TMPDIR wrong?)"),
+                     filename);
+            return -1;
+        }
+        cmd = virCommandNewArgList("sh", "-c", NULL);
+        virCommandAddArgFormat(cmd, "%s %s", editor, filename);
+    } else {
+        cmd = virCommandNewArgList(editor, filename, NULL);
     }

-    if (virAsprintf(&command, "%s %s", editor, filename) == -1) {
-        vshError(ctl,
-                 _("virAsprintf: could not create editing command: %s"),
-                 strerror(errno));
-        return -1;
+    virCommandSetInputFD(cmd, STDIN_FILENO);
+    virCommandSetOutputFD(cmd, &outfd);
+    virCommandSetErrorFD(cmd, &errfd);
+    if (virCommandRunAsync(cmd, NULL) < 0 ||
+        virCommandWait(cmd, NULL) < 0) {
+        virshReportError(ctl);
+        goto cleanup;
     }
+    ret = 0;

-    command_ret = system (command);
-    if (command_ret == -1) {
-        vshError(ctl,
-                 _("%s: edit command failed: %s"), command, strerror(errno));
-        VIR_FREE(command);
-        return -1;
-    }
-    if (WEXITSTATUS(command_ret) != 0) {
-        vshError(ctl,
-                 _("%s: command exited with non-zero status"), command);
-        VIR_FREE(command);
-        return -1;
-    }
-    VIR_FREE(command);
-    return 0;
+cleanup:
+    virCommandFree(cmd);
+    return ret;
 }

 static char *
-- 
1.7.3.5




More information about the libvir-list mailing list