On Tue, Jul 19, 2016 at 3:07 AM, Jeff King <peff@xxxxxxxx> wrote:
> On Mon, Jul 18, 2016 at 10:28:25PM -0700, Stefan Beller wrote:
>
>> On Sat, Jul 16, 2016 at 12:56 AM, Jeff King <peff@xxxxxxxx> wrote:
>> >> > + if (use_keepalive == KEEPALIVE_AFTER_NUL && !keepalive_active) {
>> >> > + const char *p = memchr(data, '\0', sz);
>> >> > + if (p) {
>> >> > + /*
>> >> > + * The NUL tells us to start sending keepalives. Make
>> >> > + * sure we send any other data we read along
>> >> > + * with it.
>> >> > + */
>> >> > + keepalive_active = 1;
>> >> > + send_sideband(1, 2, data, p - data, use_sideband);
>> >> > + send_sideband(1, 2, p + 1, sz - (p - data + 1), use_sideband);
>> >> > + continue;
>> >>
>> >> Oh, I see why the turn_on_keepalive_on_NUL doesn't work as well as I thought.
>> >> I wonder if we can use a better read function, that would stop reading at a NUL,
>> >> and return early instead?
>> >
>> > How would you do that, if not by read()ing a byte at a time (which is
>> > inefficient)? Otherwise you have to deal with the leftovers (after the
>> > NUL) in your buffer. It's one of the reasons I went with a single-byte
>> > signal, because otherwise it gets rather complicated to do robustly.
>>
>> I do not question the concept of a single NUL byte, but rather the
>> implementation, i.e. if we had an xread_until_nul you would not need
>> to have a double send_sideband here?
>
> What would xread_until_nul() look like?
>
> The only reasonable implementation I can think of is:
>
> ssize_t xread_until_nul(int fd, char *out, size_t len)
> {
> ssize_t total = 0;
> while (total < len) {
> ssize_t ret = xread(fd, out + total, 1);
> if (ret < 0) {
> /* Oops, anything in out[0..total] is lost, but
> * we have no way of signaling both a partial
> * read and an error at the end; fixable by
> * changing the interface, but doesn't really
> * matter in practice for this application. */
> return -1;
> }
> if (ret == 0)
> break; /* EOF, stop reading */
> if (out[total] == '\0')
> break; /* got our NUL, stop reading */
> total++;
> }
> return total;
> }
>
> There are two problems with this function:
>
> 1. Until we see the NUL, we're making an excessive number of read()
> syscalls looking for it. You could make larger reads, but then what
> do you do with the residual bytes (i.e., the ones after the NUL in
> the buffer you read)? You'd have to somehow save it to return at
> the next xread_until_nul(). Which implie some kind of internal
> buffering.
>
> The reason there are two send_sidebands is to cover the case where
> we have some real data, then the signal byte, then some more data.
> So instead of buffering, we just handle the data immediately.
>
> 2. How does it know when to return?
>
> We want to send the data as soon as it is available, in as large a
> chunk as possible. Using a single xread() as we do now, we get
> whatever the OS has for us, up to our buffer size.
>
> But after each 1-byte read above, we would not want to return;
> there might be more data. So it keeps reading until NUL or we fill
> our buffer. But that may mean the xread() blocks, even though we
> have data that _could_ be shipped over the sideband.
>
> To fix that, you'd have to poll() for each xread(), and return when
> it says nothing's ready.
>
> I know that writing the function myself and then critiquing is the very
> definition of a strawman. :) But it's the best I could think of. Maybe
> you had something more clever in mind?
Actually no, I had not. I was hoping you could come up with a clever thing.
My original point was the perceived added complexity to a simple
seemingly simple function (copy_to_sideband in your original patch),
so my gut reaction was to shove the complexity away into a helper function.
But no matter how it is done, there is always this one function that looks
complex for this problem. So I think your original approach is fine then?
Thanks,
Stefan
>
> -Peff
--
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