4:06 p.m.
As reported here, 'make syntax-check' missed a case:
https://www.redhat.com/archives/libvir-list/2011-January/msg01165.html
This cleans up the remaining offenders, then improves the rules.
Eric Blake (2):
build: avoid close, system
maint: reject raw close, popen in 'make syntax-check'
.x-sc_prohibit_close | 6 ++++
.x-sc_prohibit_fork_wrappers | 8 +++++
Makefile.am | 1 +
cfg.mk | 9 +++++-
src/datatypes.c | 2 +-
src/fdstream.c | 6 ++--
src/util/files.h | 4 +-
tests/commandtest.c | 12 +++++---
tools/virsh.c | 59 +++++++++++++++++++----------------------
9 files changed, 64 insertions(+), 43 deletions(-)
create mode 100644 .x-sc_prohibit_fork_wrappers
--
1.7.3.5
4:06 p.m.
New subject: [libvirt] [PATCH 1/2] build: avoid close, system
* 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/fdstream.c | 6 ++--
tests/commandtest.c | 12 +++++++---
tools/virsh.c | 59 +++++++++++++++++++++++---------------------------
3 files changed, 38 insertions(+), 39 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/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 cd54174..cf482e3 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;
@@ -9303,50 +9304,44 @@ static int
editFile (vshControl *ctl, const char *filename)
{
const char *editor;
- char *command;
- int command_ret;
+ virCommandPtr cmd;
+ int ret = -1;
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;
- }
+ if (virCommandRun(cmd, NULL) < 0)
+ 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
4:39 p.m.
New subject: [libvirt] [PATCH 1/2] build: avoid close, system
2011/1/28 Eric Blake <eblake(a)redhat.com>:
* 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/fdstream.c | 6 ++--
tests/commandtest.c | 12 +++++++---
tools/virsh.c | 59 +++++++++++++++++++++++---------------------------
3 files changed, 38 insertions(+), 39 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c
index cd54174..cf482e3 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
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 */
When VISUAL and EDITOR isn't set we fallback to vi here, but ...
/* 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);
}
... here you made it fallback to editor instead. Shouldn't this be
consistent and fallback to the same in both cases?
Anyway, that's minor and doesn't affect my ACK.
Matthias
5:05 p.m.
New subject: [libvirt] [PATCH 1/2] build: avoid close, system
On 01/28/2011 03:39 PM, Matthias Bolte wrote:
> + if (!editor)
> + editor = "vi"; /* could be cruel & default to ed(1) here */
When VISUAL and EDITOR isn't set we fallback to vi here, but ...
> + cmd = virCommandNewArgList("sh", "-c", NULL);
> + virCommandAddArgFormat(cmd, "%s %s", editor, filename);
> + } else {
> + cmd = virCommandNewArgList("editor", filename, NULL);
AARGH - stupid typo. That should be 'editor', not '"editor"'.
(Can you
tell that I tested the patch with EDITOR='emacs -nw' in my environment,
and not with EDITOR unset or a simple shell word?)
Anyway, that's minor and doesn't affect my ACK.
It does affect me applying the patch, though.
Ooh, one other bug. system() shares stdin with the child process
(important if you invoke 'virsh' interactively, and your editor wants to
reuse the same stdin as virsh for the duration of virsh being blocked).
But virCommand defaults to redirecting stdin from /dev/null (which
would make such an editor a nop, because stdin would be at EOF). v2
coming up.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
5:41 p.m.
New subject: [libvirt] [PATCH 1/2] build: avoid close, system
2011/1/29 Eric Blake <eblake(a)redhat.com>:
On 01/28/2011 03:39 PM, Matthias Bolte wrote:
>> + if (!editor)
>> + editor = "vi"; /* could be cruel & default to ed(1) here
*/
>
> When VISUAL and EDITOR isn't set we fallback to vi here, but ...
>
>> + cmd = virCommandNewArgList("sh", "-c", NULL);
>> + virCommandAddArgFormat(cmd, "%s %s", editor, filename);
>> + } else {
>> + cmd = virCommandNewArgList("editor", filename, NULL);
AARGH - stupid typo. That should be 'editor', not '"editor"'.
(Can you
tell that I tested the patch with EDITOR='emacs -nw' in my environment,
and not with EDITOR unset or a simple shell word?)
>
> Anyway, that's minor and doesn't affect my ACK.
It does affect me applying the patch, though.
Well, I misunderstood the logic here and thought that you want to
fallback to the editor command here, as editor exists on my system as
/usr/bin/editor -> /etc/alternatives/editor.
Now I understand the problem here and revoke my previous ACK :)
Matthias
6:02 p.m.
New subject: [libvirt] [PATCHv2 1/2] build: avoid close, system
* 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
5:40 a.m.
New subject: [libvirt] [PATCHv2 1/2] build: avoid close, system
2011/1/29 Eric Blake <eblake(a)redhat.com>:
* 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/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);
Took me a moment to understand this. I think in case you pass parent's
std{in,out,err} to child's std{in,out,err} this works. But when you
exchange stdout and stderr like this: parent std{in,err,out} -> child
std{in,out,err}, then childout != STDOUT_FILENO is wrong and should be
childout > STDERR_FILENO, otherwise you could close the parent's
stderr here.
+ }
+ if (childerr > STDERR_FILENO &&
childerr != childout) {
VIR_FORCE_CLOSE(childerr);
- childout = -1;
Technically it's okay to remove this like as childout isn't accessed
afterwards anymore. But by using the tmpfd variable we tricked
VIR_FORCE_CLOSE and should finish it's job here.
ACK, with that comments addressed.
Mathias
11:41 a.m.
New subject: [libvirt] [PATCHv2 1/2] build: avoid close, system
On 01/29/2011 04:40 AM, Matthias Bolte wrote:
>
> - 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);
Took me a moment to understand this. I think in case you pass parent's
std{in,out,err} to child's std{in,out,err} this works. But when you
exchange stdout and stderr like this: parent std{in,err,out} -> child
std{in,out,err}, then childout != STDOUT_FILENO is wrong and should be
childout > STDERR_FILENO, otherwise you could close the parent's
stderr here.
Okay, I made that change (I doubt that anyone will use virExec to swap
out/err from the parent to err/out in the child, but we might as well
make the change for robustness sake).
> + }
> + if (childerr > STDERR_FILENO &&
> childerr != childout) {
> VIR_FORCE_CLOSE(childerr);
> - childout = -1;
Technically it's okay to remove this like as childout isn't accessed
afterwards anymore. But by using the tmpfd variable we tricked
VIR_FORCE_CLOSE and should finish it's job here.
But if childout > STDERR_FILENO, while childerr = 2, then this branch of
code would not be reached anyway. I don't see any reason to set
childout to -1; the only reason that line was added in the first place
was due to the search-and-replace nature of converting all close() to
VIR_FORCE_CLOSE(), when tmpfd was introduced in the first place so as
not to invalidate the later comparison of childerr != childout. You are
right that childout isn't accessed later, so there's no risk of a
double-close() bug.
ACK, with that comments addressed.
I've pushed this series now.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
4:06 p.m.
New subject: [libvirt] [PATCH 2/2] maint: reject raw close, popen in 'make syntax-check'
commit f1fe9671e was supposed to make sure we use files.h
macros to avoid double close, but it didn't work.
Meanwhile, virCommand is vastly superior to system(), fork(),
and popen() (also to virExec, but we haven't completed that
conversion), so enforce that, too.
* cfg.mk (sc_prohibit_close): Fix typo that excluded close, and
add pclose.
(sc_prohibit_fork_wrappers): New rule, for fork, system, and popen.
* .x-sc_prohibit_close: More exemptions.
* .x-sc_prohibit_fork_wrappers: New file.
* Makefile.am (syntax_check_exceptions): Ship new file.
* src/datatypes.c (virReleaseConnect): Tweak comment to avoid
false positive.
* src/util/files.h (VIR_CLOSE): Likewise.
---
.x-sc_prohibit_close | 6 ++++++
.x-sc_prohibit_fork_wrappers | 8 ++++++++
Makefile.am | 1 +
cfg.mk | 9 ++++++++-
src/datatypes.c | 2 +-
src/util/files.h | 4 ++--
6 files changed, 26 insertions(+), 4 deletions(-)
create mode 100644 .x-sc_prohibit_fork_wrappers
diff --git a/.x-sc_prohibit_close b/.x-sc_prohibit_close
index 348200c..0b47b29 100644
--- a/.x-sc_prohibit_close
+++ b/.x-sc_prohibit_close
@@ -1,3 +1,9 @@
+# Non-C files:
^docs/.*
+^ChangeLog*
^HACKING$
+*.py$
+# Wrapper implementation:
^src/util/files.c$
+# Only uses close in documentation comments:
+^src/libvirt\.c$
diff --git a/.x-sc_prohibit_fork_wrappers b/.x-sc_prohibit_fork_wrappers
new file mode 100644
index 0000000..7f8fc6c
--- /dev/null
+++ b/.x-sc_prohibit_fork_wrappers
@@ -0,0 +1,8 @@
+^docs/.*
+^HACKING$
+^src/util/util\.c$
+^tests/testutils\.c$
+# Files that we may want to convert over to virCommand someday...
+^daemon/libvirtd\.c$
+^src/libvirt\.c$
+^src/lxc/lxc_controller\.c$
diff --git a/Makefile.am b/Makefile.am
index 36463f5..597ec61 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -25,6 +25,7 @@ syntax_check_exceptions = \
.x-sc_prohibit_asprintf \
.x-sc_prohibit_close \
.x-sc_prohibit_empty_lines_at_EOF \
+ .x-sc_prohibit_fork_wrappers \
.x-sc_prohibit_gethostby \
.x-sc_prohibit_gethostname \
.x-sc_prohibit_gettext_noop \
diff --git a/cfg.mk b/cfg.mk
index db6863d..2cc6f90 100644
--- a/cfg.mk
+++ b/cfg.mk
@@ -241,13 +241,20 @@ sc_avoid_write:
# Avoid functions that can lead to double-close bugs.
sc_prohibit_close:
- @prohibit='\<[f]close *\(' \
+ @prohibit='([^>.]|^)\<[fp]?close *\(' \
halt='use VIR_{FORCE_}[F]CLOSE instead of [f]close' \
$(_sc_search_regexp)
@prohibit='\<fdopen *\(' \
halt='use VIR_FDOPEN instead of fdopen' \
$(_sc_search_regexp)
+# Prefer virCommand for all child processes.
+# XXX - eventually, we want to enhance this to also prohibit virExec.
+sc_prohibit_fork_wrappers:
+ @prohibit='= *\<(fork|popen|system) *\(' \
+ halt='use virCommand for child processes' \
+ $(_sc_search_regexp)
+
# Similar to the gnulib maint.mk rule for sc_prohibit_strcmp
# Use STREQLEN or STRPREFIX rather than comparing strncmp == 0, or != 0.
sc_prohibit_strncmp:
diff --git a/src/datatypes.c b/src/datatypes.c
index 7f2d0c5..e26e332 100644
--- a/src/datatypes.c
+++ b/src/datatypes.c
@@ -246,7 +246,7 @@ virReleaseConnect(virConnectPtr conn) {
DEBUG("release connection %p", conn);
/* make sure to release the connection lock before we call the
- * close() callbacks, otherwise we will deadlock if an error
+ * close callbacks, otherwise we will deadlock if an error
* is raised by any of the callbacks */
virMutexUnlock(&conn->lock);
diff --git a/src/util/files.h b/src/util/files.h
index fe8c00c..744ba93 100644
--- a/src/util/files.h
+++ b/src/util/files.h
@@ -1,9 +1,9 @@
/*
* files.h: safer file handling
*
+ * Copyright (C) 2010-2011 RedHat, Inc.
* Copyright (C) 2010 IBM Corporation
* Copyright (C) 2010 Stefan Berger
- * Copyright (C) 2010 RedHat, Inc.
* Copyright (C) 2010 Eric Blake
*
* This library is free software; you can redistribute it and/or
@@ -39,7 +39,7 @@ int virFclose(FILE **file, bool preserve_errno) ATTRIBUTE_RETURN_CHECK;
FILE *virFdopen(int *fdptr, const char *mode) ATTRIBUTE_RETURN_CHECK;
/* For use on normal paths; caller must check return value,
- and failure sets errno per close(). */
+ and failure sets errno per close. */
# define VIR_CLOSE(FD) virClose(&(FD), false)
# define VIR_FCLOSE(FILE) virFclose(&(FILE), false)
--
1.7.3.5
5:04 p.m.
New subject: [libvirt] [PATCH 2/2] maint: reject raw close, popen in 'make syntax-check'
2011/1/28 Eric Blake <eblake(a)redhat.com>:
commit f1fe9671e was supposed to make sure we use files.h
macros to avoid double close, but it didn't work.
Meanwhile, virCommand is vastly superior to system(), fork(),
and popen() (also to virExec, but we haven't completed that
conversion), so enforce that, too.
* cfg.mk (sc_prohibit_close): Fix typo that excluded close, and
add pclose.
(sc_prohibit_fork_wrappers): New rule, for fork, system, and popen.
* .x-sc_prohibit_close: More exemptions.
* .x-sc_prohibit_fork_wrappers: New file.
* Makefile.am (syntax_check_exceptions): Ship new file.
* src/datatypes.c (virReleaseConnect): Tweak comment to avoid
false positive.
* src/util/files.h (VIR_CLOSE): Likewise.
---
.x-sc_prohibit_close | 6 ++++++
.x-sc_prohibit_fork_wrappers | 8 ++++++++
Makefile.am | 1 +
cfg.mk | 9 ++++++++-
src/datatypes.c | 2 +-
src/util/files.h | 4 ++--
6 files changed, 26 insertions(+), 4 deletions(-)
create mode 100644 .x-sc_prohibit_fork_wrappers
diff --git a/.x-sc_prohibit_close b/.x-sc_prohibit_close
index 348200c..0b47b29 100644
--- a/.x-sc_prohibit_close
+++ b/.x-sc_prohibit_close
@@ -1,3 +1,9 @@
+# Non-C files:
^docs/.*
+^ChangeLog*
^HACKING$
+*.py$
Shouldn't the dot be escaped here like in the other lines you added?
+# Wrapper implementation:
^src/util/files.c$
And here too, although this one is pre-existing.
+# Only uses close in documentation comments:
+^src/libvirt\.c$
ACK, with that nit fixed.
Matthias
6:03 p.m.
New subject: [libvirt] [PATCH 2/2] maint: reject raw close, popen in 'make syntax-check'
On 01/28/2011 04:04 PM, Matthias Bolte wrote:
> +++ b/.x-sc_prohibit_close
> @@ -1,3 +1,9 @@
> +# Non-C files:
> ^docs/.*
> +^ChangeLog*
> ^HACKING$
> +*.py$
Shouldn't the dot be escaped here like in the other lines you added?
> +# Wrapper implementation:
> ^src/util/files.c$
And here too, although this one is pre-existing.
Sure. I'll fix that.
> +# Only uses close in documentation comments:
> +^src/libvirt\.c$
ACK, with that nit fixed.
It's minor, so I'm not posting my v2 of this patch. But obviously I
can't apply this until v2 of 1/2 is approved, since otherwise I would be
breaking 'make syntax-check'.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
5081
days inactive
5082
days old
10 comments
2 participants
participants (2)
-
Eric Blake -
Matthias Bolte