On Tue, Jun 14, 2016 at 05:50:19PM -0400, Jeff King wrote:
> On Tue, Jun 14, 2016 at 11:13:54AM -0700, Junio C Hamano wrote:
>
> > Michael J Gruber <git@xxxxxxxxxxxxxxxxxxxx> writes:
> >
> > > bottom = signature->len;
> > > - len = strbuf_read(signature, gpg.out, 1024);
> > > + strbuf_read(signature, gpg.out, 1024);
> > > + strbuf_read(&err, gpg.err, 0);
> >
> > Hmmmm, isn't this asking for a deadlock? When GPG spews more than
> > what would fit in a pipe buffer to its standard error (hence gets
> > blocked), its standard output may not complete, and the we would get
> > stuck by attempting to read from gpg.out, failing to reach the other
> > strbuf_read() that would unblock GPG by reading from gpg.err?
>
> Yeah, it definitely is a deadlock. I think we'd need a select loop to
> read into multiple strbufs at once (we can't use "struct async" because
> that might happen in another process).
Something like this on top of Michael's patch (only lightly tested).
I'm still undecided on whether it is a better approach than making sure
the stdout we got looks sane. In particular I'd worry that it would make
things harder for somebody trying to plug in something gpg-like (e.g.,
if you wanted to do something exotic like call a program which fetched
the signature from a remote device or something). But it's probably not
_that_ hard for such a script to emulate --status-fd.
---
diff --git a/gpg-interface.c b/gpg-interface.c
index 850dc81..576e462 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -153,6 +153,7 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *sig
const char *args[5];
size_t i, j, bottom;
struct strbuf err = STRBUF_INIT;
+ struct strbuf_reader readers[2];
gpg.argv = args;
gpg.in = -1;
@@ -183,8 +184,15 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *sig
close(gpg.in);
bottom = signature->len;
- strbuf_read(signature, gpg.out, 1024);
- strbuf_read(&err, gpg.err, 0);
+
+ readers[0].buf = signature;
+ readers[0].fd = gpg.out;
+ readers[0].hint = 1024;
+ readers[1].buf = &err;
+ readers[1].fd = gpg.err;
+ readers[1].hint = 1024;
+ strbuf_read_parallel(readers, 2);
+
close(gpg.out);
close(gpg.err);
diff --git a/strbuf.c b/strbuf.c
index 1ba600b..f674b23 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -395,6 +395,58 @@ ssize_t strbuf_read_once(struct strbuf *sb, int fd, size_t hint)
return cnt;
}
+void strbuf_read_parallel(struct strbuf_reader *readers, int nr)
+{
+ int i;
+ struct pollfd *pfd;
+
+ ALLOC_ARRAY(pfd, nr);
+ for (i = 0; i < nr; i++)
+ readers[i].error = 0;
+
+ while (1) {
+ int pollsize = 0;
+ int ret;
+
+ for (i = 0; i < nr; i++) {
+ if (readers[i].fd < 0)
+ continue;
+ pfd[pollsize].fd = readers[i].fd;
+ pfd[pollsize].events = POLLIN;
+ readers[i].pfd = &pfd[pollsize];
+ pollsize++;
+ }
+
+ if (!pollsize)
+ break;
+
+ ret = poll(pfd, pollsize, -1);
+ if (ret < 0) {
+ if (errno == EINTR)
+ continue;
+ /* should never happen? */
+ die_errno("poll failed");
+ }
+
+ for (i = 0; i < nr; i++) {
+ if (readers[i].fd < 0)
+ continue;
+ if (readers[i].pfd->revents &
+ (POLLIN|POLLHUP|POLLERR|POLLNVAL)) {
+ ret = strbuf_read_once(readers[i].buf,
+ readers[i].fd,
+ readers[i].hint);
+ if (ret < 0)
+ readers[i].error = errno;
+ if (ret <= 0)
+ readers[i].fd = -1;
+ }
+ }
+ }
+
+ free(pfd);
+}
+
ssize_t strbuf_write(struct strbuf *sb, FILE *f)
{
return sb->len ? fwrite(sb->buf, 1, sb->len, f) : 0;
diff --git a/strbuf.h b/strbuf.h
index 7987405..b93822e 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -374,6 +374,25 @@ extern ssize_t strbuf_read(struct strbuf *, int fd, size_t hint);
*/
extern ssize_t strbuf_read_once(struct strbuf *, int fd, size_t hint);
+/**
+ * Read from several descriptors in parallel, each into its own strbuf.
+ * This can be used, for example, to capture stdout and stderr from a
+ * sub-process without worrying about deadlocks.
+ */
+struct strbuf_reader {
+ /* Initialized by caller */
+ struct strbuf *buf;
+ int fd;
+ size_t hint;
+
+ /* Returned by strbuf_read_parallel */
+ int error; /* 0 for success, otherwise errno */
+
+ /* Internal use */
+ struct pollfd *pfd;
+};
+void strbuf_read_parallel(struct strbuf_reader *readers, int nr);
+
/**
* Read the contents of a file, specified by its path. The third argument
* can be used to give a hint about the file size, to avoid reallocs.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html