- Remove one indentation level in `rpc_out()`, as the conditions can be
combined into just one `if` statement without losing readability.
Skipping the resetting of `initial_buffer`/`len`/`pos` revealed a bug
in `stateless_connect()`, where `rpc.len` is reset to 0 after each
request, but not `rpc.pos`. Relying on `rpc_out()` always doing this
before has never been safe (it might have not finished cleanly, for
example). So better reset it there, just like `rpc.len`.
- Rename `flush_read_but_not_sent` to `read_from_out_done`. The name is
slightly misleading, because the "flush" might never be really "sent"
(depends on `write_line_lengths`), and this is not the most important
part anyway. The primary role of the flag is rather to signal that
`read_from_out()` is "done" and must not be called for this particular
RPC exchange anymore.
- Update/add some related comments.
Signed-off-by: Jiri Hruska <jirka@xxxxxx>
---
remote-curl.c | 46 ++++++++++++++++++++++++++++------------------
1 file changed, 28 insertions(+), 18 deletions(-)
diff --git a/remote-curl.c b/remote-curl.c
index 690df2a43e..d5aa66a44c 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -606,13 +606,14 @@ struct rpc_state {
unsigned write_line_lengths : 1;
/*
- * Used by rpc_out; initialize to 0. This is true if a flush has been
- * read, but the corresponding line length (if write_line_lengths is
- * true) and EOF have not been sent to libcurl. Since each flush marks
- * the end of a request, each flush must be completely sent before any
- * further reading occurs.
+ * Used by rpc_out; initialize to 0. This is true if a flush packet
+ * has been read from the child process, signaling the end of the
+ * current data to send. There might be still some bytes pending in
+ * 'buf' (e.g. the corresponding line length, if write_line_lengths
+ * is true), but no more reads can be performed on the 'out' pipe as
+ * part of the current RPC exchange.
*/
- unsigned flush_read_but_not_sent : 1;
+ unsigned read_from_out_done : 1;
};
#define RPC_STATE_INIT { 0 }
@@ -690,21 +691,29 @@ static size_t rpc_out(void *ptr, size_t eltsize,
size_t avail = rpc->len - rpc->pos;
enum packet_read_status status;
- if (!avail) {
+ /*
+ * If there is no more data available in our buffer and the child
+ * process is not done sending yet, read the next packet.
+ */
+ if (!avail && !rpc->read_from_out_done) {
rpc->initial_buffer = 0;
rpc->len = 0;
rpc->pos = 0;
- if (!rpc->flush_read_but_not_sent) {
- if (!rpc_read_from_out(rpc, 0, &avail, &status))
- BUG("The entire rpc->buf should be larger than LARGE_PACKET_MAX");
- if (status == PACKET_READ_FLUSH)
- rpc->flush_read_but_not_sent = 1;
- }
+ if (!rpc_read_from_out(rpc, 0, &avail, &status))
+ BUG("The entire rpc->buf should be larger than LARGE_PACKET_MAX");
+
/*
- * If flush_read_but_not_sent is true, we have already read one
- * full request but have not fully sent it + EOF, which is why
- * we need to refrain from reading.
+ * If a flush packet was read, it means the child process is
+ * done sending this request. The buffer might be fully empty
+ * at this point or contain a flush packet too, depending on
+ * rpc->write_line_lengths.
+ * In any case, we must refrain from reading any more, because
+ * the child process already expects to receive a response back
+ * instead. If both sides would try to read at once, they would
+ * just hang waiting for each other.
*/
+ if (status == PACKET_READ_FLUSH)
+ rpc->read_from_out_done = 1;
}
/*
@@ -967,7 +976,7 @@ static int post_rpc(struct rpc_state *rpc, int
stateless_connect, int flush_rece
*/
headers = curl_slist_append(headers, "Transfer-Encoding: chunked");
rpc->initial_buffer = 1;
- rpc->flush_read_but_not_sent = 0;
+ rpc->read_from_out_done = 0;
curl_easy_setopt(slot->curl, CURLOPT_READFUNCTION, rpc_out);
curl_easy_setopt(slot->curl, CURLOPT_INFILE, rpc);
curl_easy_setopt(slot->curl, CURLOPT_SEEKFUNCTION, rpc_seek);
@@ -1487,7 +1496,7 @@ static int stateless_connect(const char *service_name)
rpc.gzip_request = 1;
rpc.initial_buffer = 0;
rpc.write_line_lengths = 1;
- rpc.flush_read_but_not_sent = 0;
+ rpc.read_from_out_done = 0;
/*
* Dump the capability listing that we got from the server earlier
@@ -1510,6 +1519,7 @@ static int stateless_connect(const char *service_name)
break;
/* Reset the buffer for next request */
rpc.len = 0;
+ rpc.pos = 0;
}
free(rpc.service_url);
--
2.42.1.5.g2f21867bd5
