| Message ID | e19f294df9ff999d30a47339a7848c7104bfae7d.1543879256.git.jonathantanmy@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Design for offloading part of packfile response to CDN | expand |
On Mon, Dec 3, 2018 at 3:37 PM Jonathan Tan <jonathantanmy@google.com> wrote: > > Subsequent patches will change how the output of pack-objects is > processed, so extract that processing into its own function. > > Currently, at most 1 character can be buffered (in the "buffered" local > variable). One of those patches will require a larger buffer, so replace > that "buffered" local variable with a buffer array. This buffering sounds oddly similar to the pkt reader which can buffer at most one pkt, the difference being that we'd buffer bytes instead of pkts.
Jonathan Tan <jonathantanmy@google.com> writes: > +struct output_state { > + char buffer[8193]; > + int used; > +}; > + > +static int read_pack_objects_stdout(int outfd, struct output_state *os) > +{ The naming feels quite odd. We are downstream of pack-objects and reading its standard output stream, so "read stdout-of-pack-objects" is not wrong per-se, but it may be just me. Stepping back and what the function is really about often helps us to understand what we are doing. I think the function you extracted from the existing logic is responsible for relaying the data read from pack-objects to the fetch-pack client on the other side of the wire. So from that point of view, singling out "read" as if it is a primary thing the function does is already suboptimal. Perhaps static int relay_pack_data(int in, struct output_state *os) because the fd is what we "read" from (hence, 'in', not 'out'), and relaying is what we do and reading is only one half of doing so? > + /* Data ready; we keep the last byte to ourselves > + * in case we detect broken rev-list, so that we > + * can leave the stream corrupted. This is > + * unfortunate -- unpack-objects would happily > + * accept a valid packdata with trailing garbage, > + * so appending garbage after we pass all the > + * pack data is not good enough to signal > + * breakage to downstream. > + */ Yeah, I recall writing this funky and unfortunate logic in 2006. Perhaps we want to update the comment style to make it a bit more modern? The "Data ready;" part of the comment applies more to what the caller of this logic does (i.e. poll returned and revents indicates we can read, and that is why we are calling into this logic). The remainder is the comment that is relevat to this logic. > + ssize_t readsz; > + > + readsz = xread(outfd, os->buffer + os->used, > + sizeof(os->buffer) - os->used); So we read what we could read in the remaining buffer. I notice that you alloated 8k+1 bytes; would this code end up reading that 8k+1 bytes in full under the right condition? I am mainly wondering where the need for +1 comes from. > + if (readsz < 0) { > + return readsz; > + } OK, report an error to the caller by returning negative. I am guessing that you'd have more code in this block that currently have only one statement in the future steps, perhaps. > + os->used += readsz; > + > + if (os->used > 1) { > + send_client_data(1, os->buffer, os->used - 1); > + os->buffer[0] = os->buffer[os->used - 1]; OK, this corresponds to the "*cp++ = buffered" in the original just before xread(). > + os->used = 1; > + } else { > + send_client_data(1, os->buffer, os->used); > + os->used = 0; I am not sure if the code is correct when os->used happens to be 1 (shouldn't we hold the byte, skip the call to send_client_data(), and go back to poll() to expect more data?), but this is a faithful code movement and rewrite of the original. I've read the caller (below, snipped) and the conversion looked sensible there as well. The final flushing of the byte(s) held back in *os also looks good.
diff --git a/upload-pack.c b/upload-pack.c index 5e81f1ff24..cec43e0a46 100644 --- a/upload-pack.c +++ b/upload-pack.c @@ -101,14 +101,51 @@ static int write_one_shallow(const struct commit_graft *graft, void *cb_data) return 0; } +struct output_state { + char buffer[8193]; + int used; +}; + +static int read_pack_objects_stdout(int outfd, struct output_state *os) +{ + /* Data ready; we keep the last byte to ourselves + * in case we detect broken rev-list, so that we + * can leave the stream corrupted. This is + * unfortunate -- unpack-objects would happily + * accept a valid packdata with trailing garbage, + * so appending garbage after we pass all the + * pack data is not good enough to signal + * breakage to downstream. + */ + ssize_t readsz; + + readsz = xread(outfd, os->buffer + os->used, + sizeof(os->buffer) - os->used); + if (readsz < 0) { + return readsz; + } + os->used += readsz; + + if (os->used > 1) { + send_client_data(1, os->buffer, os->used - 1); + os->buffer[0] = os->buffer[os->used - 1]; + os->used = 1; + } else { + send_client_data(1, os->buffer, os->used); + os->used = 0; + } + + return readsz; +} + static void create_pack_file(const struct object_array *have_obj, const struct object_array *want_obj) { struct child_process pack_objects = CHILD_PROCESS_INIT; - char data[8193], progress[128]; + struct output_state output_state = {0}; + char progress[128]; char abort_msg[] = "aborting due to possible repository " "corruption on the remote side."; - int buffered = -1; ssize_t sz; int i; FILE *pipe_fd; @@ -235,39 +272,15 @@ static void create_pack_file(const struct object_array *have_obj, continue; } if (0 <= pu && (pfd[pu].revents & (POLLIN|POLLHUP))) { - /* Data ready; we keep the last byte to ourselves - * in case we detect broken rev-list, so that we - * can leave the stream corrupted. This is - * unfortunate -- unpack-objects would happily - * accept a valid packdata with trailing garbage, - * so appending garbage after we pass all the - * pack data is not good enough to signal - * breakage to downstream. - */ - char *cp = data; - ssize_t outsz = 0; - if (0 <= buffered) { - *cp++ = buffered; - outsz++; - } - sz = xread(pack_objects.out, cp, - sizeof(data) - outsz); - if (0 < sz) - ; - else if (sz == 0) { + int result = read_pack_objects_stdout(pack_objects.out, + &output_state); + + if (result == 0) { close(pack_objects.out); pack_objects.out = -1; - } - else + } else if (result < 0) { goto fail; - sz += outsz; - if (1 < sz) { - buffered = data[sz-1] & 0xFF; - sz--; } - else - buffered = -1; - send_client_data(1, data, sz); } /* @@ -292,9 +305,8 @@ static void create_pack_file(const struct object_array *have_obj, } /* flush the data */ - if (0 <= buffered) { - data[0] = buffered; - send_client_data(1, data, 1); + if (output_state.used > 0) { + send_client_data(1, output_state.buffer, output_state.used); fprintf(stderr, "flushed.\n"); } if (use_sideband)
Subsequent patches will change how the output of pack-objects is processed, so extract that processing into its own function. Currently, at most 1 character can be buffered (in the "buffered" local variable). One of those patches will require a larger buffer, so replace that "buffered" local variable with a buffer array. Signed-off-by: Jonathan Tan <jonathantanmy@google.com> --- upload-pack.c | 80 +++++++++++++++++++++++++++++---------------------- 1 file changed, 46 insertions(+), 34 deletions(-)