[PATCH 3/7] strbuf: introduce strbuf_read_cmd helper
Something as simple as reading the stdout from a command
turns out to be rather hard to do right. Doing:
if (!run_command(cmd))
strbuf_read(buf, cmd.out, 0);
can result in deadlock if the child process produces a large
amount of output. What happens is:
1. The parent spawns the child with its stdout connected
to a pipe, of which the parent is the sole reader.
2. The parent calls wait(), blocking until the child exits.
3. The child writes to stdout. If it writes more data than
the OS pipe buffer can hold, the write() call will
block.
This is a deadlock; the parent is waiting for the child to
exit, and the child is waiting for the parent to call
read().
So we should do instead:
if (!start_command(cmd)) {
strbuf_read(buf, cmd.out, 0);
finish_command(cmd);
}
But note that this leaks cmd.out (which must be closed). And
there's no error handling for strbuf_read. We probably want
to know whether the operation succeeded, but we must make
sure to always run finish_command even if the read failed
(or else we leave a zombie child process).
Let's introduce a strbuf helper that can make this a bit
simpler for callers to do right.
Signed-off-by: Jeff King p...@peff.net
---
This is really at the intersection of the strbuf and
run-command APIs, so you could argue for it being part of
either It is logically quite like the strbuf_read_file()
function, so I put it there.
strbuf.c | 17 +
strbuf.h | 10 ++
2 files changed, 27 insertions(+)
diff --git a/strbuf.c b/strbuf.c
index 88cafd4..9d1d48f 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -1,6 +1,7 @@
#include cache.h
#include refs.h
#include utf8.h
+#include run-command.h
int starts_with(const char *str, const char *prefix)
{
@@ -414,6 +415,22 @@ int strbuf_readlink(struct strbuf *sb, const char *path,
size_t hint)
return -1;
}
+int strbuf_read_cmd(struct strbuf *sb, struct child_process *cmd, size_t hint)
+{
+ cmd-out = -1;
+ if (start_command(cmd) 0)
+ return -1;
+
+ if (strbuf_read(sb, cmd-out, hint) 0) {
+ close(cmd-out);
+ finish_command(cmd); /* throw away exit code */
+ return -1;
+ }
+
+ close(cmd-out);
+ return finish_command(cmd);
+}
+
int strbuf_getcwd(struct strbuf *sb)
{
size_t oldalloc = sb-alloc;
diff --git a/strbuf.h b/strbuf.h
index 1883494..93a50da 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -1,6 +1,8 @@
#ifndef STRBUF_H
#define STRBUF_H
+struct child_process;
+
/**
* strbuf's are meant to be used with all the usual C string and memory
* APIs. Given that the length of the buffer is known, it's often better to
@@ -373,6 +375,14 @@ extern int strbuf_read_file(struct strbuf *sb, const char
*path, size_t hint);
extern int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint);
/**
+ * Execute the given command, capturing its stdout in the given strbuf.
+ * Returns -1 if starting the command fails or reading fails, and otherwise
+ * returns the exit code of the command. The output collected in the
+ * buffer is kept even if the command returns a non-zero exit.
+ */
+int strbuf_read_cmd(struct strbuf *sb, struct child_process *cmd, size_t hint);
+
+/**
* Read a line from a FILE *, overwriting the existing contents
* of the strbuf. The second argument specifies the line
* terminator character, typically `'\n'`.
--
2.3.3.618.ga041503
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/7] strbuf: introduce strbuf_read_cmd helper
On Sun, Mar 22, 2015 at 6:07 AM, Jeff King p...@peff.net wrote: Something as simple as reading the stdout from a command turns out to be rather hard to do right. Doing: if (!run_command(cmd)) strbuf_read(buf, cmd.out, 0); can result in deadlock if the child process produces a large amount of output. [...] Let's introduce a strbuf helper that can make this a bit simpler for callers to do right. Signed-off-by: Jeff King p...@peff.net --- This is really at the intersection of the strbuf and run-command APIs, so you could argue for it being part of either It is logically quite like the strbuf_read_file() function, so I put it there. It does feel like a layering violation. If moved to the run-command API, it could given one of the following names or something better: run_command_capture() capture_command() command_capture() run_command_with_output() capture_output() diff --git a/strbuf.h b/strbuf.h index 1883494..93a50da 100644 --- a/strbuf.h +++ b/strbuf.h @@ -1,6 +1,8 @@ #ifndef STRBUF_H #define STRBUF_H +struct child_process; + /** * strbuf's are meant to be used with all the usual C string and memory * APIs. Given that the length of the buffer is known, it's often better to @@ -373,6 +375,14 @@ extern int strbuf_read_file(struct strbuf *sb, const char *path, size_t hint); extern int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint); /** + * Execute the given command, capturing its stdout in the given strbuf. + * Returns -1 if starting the command fails or reading fails, and otherwise + * returns the exit code of the command. The output collected in the + * buffer is kept even if the command returns a non-zero exit. + */ +int strbuf_read_cmd(struct strbuf *sb, struct child_process *cmd, size_t hint); + +/** * Read a line from a FILE *, overwriting the existing contents * of the strbuf. The second argument specifies the line * terminator character, typically `'\n'`. -- 2.3.3.618.ga041503 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/7] strbuf: introduce strbuf_read_cmd helper
Eric Sunshine sunsh...@sunshineco.com writes: On Sun, Mar 22, 2015 at 6:07 AM, Jeff King p...@peff.net wrote: Something as simple as reading the stdout from a command turns out to be rather hard to do right. Doing: if (!run_command(cmd)) strbuf_read(buf, cmd.out, 0); can result in deadlock if the child process produces a large amount of output. [...] Let's introduce a strbuf helper that can make this a bit simpler for callers to do right. Signed-off-by: Jeff King p...@peff.net --- This is really at the intersection of the strbuf and run-command APIs, so you could argue for it being part of either It is logically quite like the strbuf_read_file() function, so I put it there. It does feel like a layering violation. If moved to the run-command API, it could given one of the following names or something better: run_command_capture() capture_command() command_capture() run_command_with_output() capture_output() Sound like a good suggestion (but I haven't read the users of the proposed function, after doing which I might change my mind---I'll see). Thanks. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/7] strbuf: introduce strbuf_read_cmd helper
Junio C Hamano gits...@pobox.com writes: Eric Sunshine sunsh...@sunshineco.com writes: It does feel like a layering violation. If moved to the run-command API, it could given one of the following names or something better: run_command_capture() capture_command() command_capture() run_command_with_output() capture_output() Sound like a good suggestion (but I haven't read the users of the proposed function, after doing which I might change my mind---I'll see). Now I read the callers, it does look like this new function better fits in the run-command suite, essentially allowing us to do what we would do with $(cmd) or `cmd` in shell and Perl scripts, even though I do not particularly agree with the phrase layering violation to call its current placement. Thanks. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/7] strbuf: introduce strbuf_read_cmd helper
Jeff King p...@peff.net writes: diff --git a/strbuf.h b/strbuf.h index 1883494..93a50da 100644 --- a/strbuf.h +++ b/strbuf.h @@ -1,6 +1,8 @@ #ifndef STRBUF_H #define STRBUF_H +struct child_process; + /** * strbuf's are meant to be used with all the usual C string and memory * APIs. Given that the length of the buffer is known, it's often better to @@ -373,6 +375,14 @@ extern int strbuf_read_file(struct strbuf *sb, const char *path, size_t hint); extern int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint); /** + * Execute the given command, capturing its stdout in the given strbuf. + * Returns -1 if starting the command fails or reading fails, and otherwise + * returns the exit code of the command. The output collected in the + * buffer is kept even if the command returns a non-zero exit. + */ +int strbuf_read_cmd(struct strbuf *sb, struct child_process *cmd, size_t hint); + +/** * Read a line from a FILE *, overwriting the existing contents * of the strbuf. The second argument specifies the line * terminator character, typically `'\n'`. It is an unfortunate tangent that this is a bugfix that may want to go to 'maint' and older, but our earlier jk/strbuf-doc-to-header topic introduces an unnecessary merge conflicts. I've wiggled this part and moved the doc elsewhere, only to remove that in the merge, which may not be optimal from the point of view of what I have to do when merging this topic down from pu to next to master to maint, but I do not see a good way around it. Thanks. The whole series looks very sensible. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/7] strbuf: introduce strbuf_read_cmd helper
On Sun, Mar 22, 2015 at 04:22:50PM -0700, Junio C Hamano wrote: +/** * Read a line from a FILE *, overwriting the existing contents * of the strbuf. The second argument specifies the line * terminator character, typically `'\n'`. It is an unfortunate tangent that this is a bugfix that may want to go to 'maint' and older, but our earlier jk/strbuf-doc-to-header topic introduces an unnecessary merge conflicts. Yeah, that is the worst part of refactoring and cleanup. Even when you make sure you are not hurting any topics in flight, you cannot know when a new topic will take off in your general area. I've wiggled this part and moved the doc elsewhere, only to remove that in the merge, which may not be optimal from the point of view of what I have to do when merging this topic down from pu to next to master to maint, but I do not see a good way around it. I'd suggest just dropping the documentation in the maint version (i.e., make it a moral cherry-pick of the function declaration only). -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/7] strbuf: introduce strbuf_read_cmd helper
On Sun, Mar 22, 2015 at 03:36:01PM -0400, Eric Sunshine wrote: This is really at the intersection of the strbuf and run-command APIs, so you could argue for it being part of either It is logically quite like the strbuf_read_file() function, so I put it there. It does feel like a layering violation. If moved to the run-command API, it could given one of the following names or something better: A layering violation implies there is an ordering to the APIs. Certainly we call APIs from other APIs all the time. I guess you could argue that these are the same layer, and should be next to each, and not building on each other (i.e., that strbuf has dependencies only on system APIs like stdio.h, and run-command only on system APIs like unistd.h, etc). But then reversing the order of the dependency does not really solve that. You would have to introduce a new higher-level API that combines them. But that seems silly for a single function (and I do not foresee any other similar functions). That being said, I'm not opposed to one of the reverse names if people feel strongly (I also considered making it an option flag to run_command_v_opt, but it ended up tangling things quite a bit more). -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
