sendfile() can return after a short read/write, so we may need to call it more than once. Furthermore, not all files support sendfile(), so we may need to fall back to read/write. On the read/write path, use write_in_full which deals with short writes. Signed-off-by: Hristo Venev <hristo@venev.name> --- cache.c | 46 ++++++++++++++++++++++++---------------------- 1 file changed, 24 insertions(+), 22 deletions(-) diff --git a/cache.c b/cache.c index 55199e8..85cfbd9 100644 --- a/cache.c +++ b/cache.c @@ -85,40 +85,42 @@ static int close_slot(struct cache_slot *slot) /* Print the content of the active cache slot (but skip the key). */ static int print_slot(struct cache_slot *slot) { -#ifdef HAVE_LINUX_SENDFILE - off_t start_off; - int ret; + off_t off; + ssize_t i; + + off = slot->keylen + 1; - start_off = slot->keylen + 1; +#ifdef HAVE_LINUX_SENDFILE + off_t size; + size = slot->cache_st.st_size; do { - ret = sendfile(STDOUT_FILENO, slot->cache_fd, &start_off, - slot->cache_st.st_size - start_off); - if (ret < 0) { + i = sendfile(STDOUT_FILENO, slot->cache_fd, &off, size - off); + if (i < 0) { if (errno == EAGAIN || errno == EINTR) continue; + /* Fall back to read/write on EINVAL */ + if (errno == EINVAL) + break; return errno; } - return 0; + if (off == size) + return 0; } while (1); -#else - ssize_t i, j; +#endif - i = lseek(slot->cache_fd, slot->keylen + 1, SEEK_SET); - if (i != slot->keylen + 1) + if (lseek(slot->cache_fd, off, SEEK_SET) != off) return errno; do { - i = j = xread(slot->cache_fd, slot->buf, sizeof(slot->buf)); - if (i > 0) - j = xwrite(STDOUT_FILENO, slot->buf, i); - } while (i > 0 && j == i); - - if (i < 0 || j != i) - return errno; - else - return 0; -#endif + i = xread(slot->cache_fd, slot->buf, sizeof(slot->buf)); + if (i < 0) + return errno; + if (i == 0) + return 0; + if (write_in_full(STDOUT_FILENO, slot->buf, i) < 0) + return errno; + } while (1); } /* Check if the slot has expired */ -- 2.31.1
On Fri, Sep 10, 2021 at 05:18:41PM +0300, Hristo Venev wrote: > sendfile() can return after a short read/write, so we may need to call > it more than once. Furthermore, not all files support sendfile(), so we > may need to fall back to read/write. Have you seen these errors in practice, or is this just theoretical? In recent (since v2.6.33) versions of Linux, all files should support sendfile(), especially since we expect out_fd to be a socket or pipe. > On the read/write path, use write_in_full which deals with short writes. > > Signed-off-by: Hristo Venev <hristo@venev.name> > --- > cache.c | 46 ++++++++++++++++++++++++---------------------- > 1 file changed, 24 insertions(+), 22 deletions(-) > > diff --git a/cache.c b/cache.c > index 55199e8..85cfbd9 100644 > --- a/cache.c > +++ b/cache.c > @@ -85,40 +85,42 @@ static int close_slot(struct cache_slot *slot) > /* Print the content of the active cache slot (but skip the key). */ > static int print_slot(struct cache_slot *slot) > { > -#ifdef HAVE_LINUX_SENDFILE > - off_t start_off; > - int ret; > + off_t off; > + ssize_t i; > + > + off = slot->keylen + 1; > > - start_off = slot->keylen + 1; > +#ifdef HAVE_LINUX_SENDFILE > + off_t size; decl-after-stmt if HAVE_LINUX_SENDFILE is set. > + size = slot->cache_st.st_size; > > do { > - ret = sendfile(STDOUT_FILENO, slot->cache_fd, &start_off, > - slot->cache_st.st_size - start_off); > - if (ret < 0) { > + i = sendfile(STDOUT_FILENO, slot->cache_fd, &off, size - off); Why is ret renamed? i is normally a loop index variable, using it for the return value here is strange, please stick with "ret". > + if (i < 0) { > if (errno == EAGAIN || errno == EINTR) > continue; > + /* Fall back to read/write on EINVAL */ > + if (errno == EINVAL) > + break; > return errno; > } > - return 0; > + if (off == size) > + return 0; > } while (1); > -#else > - ssize_t i, j; > +#endif > > - i = lseek(slot->cache_fd, slot->keylen + 1, SEEK_SET); > - if (i != slot->keylen + 1) > + if (lseek(slot->cache_fd, off, SEEK_SET) != off) > return errno; > > do { > - i = j = xread(slot->cache_fd, slot->buf, sizeof(slot->buf)); > - if (i > 0) > - j = xwrite(STDOUT_FILENO, slot->buf, i); > - } while (i > 0 && j == i); > - > - if (i < 0 || j != i) > - return errno; > - else > - return 0; > -#endif > + i = xread(slot->cache_fd, slot->buf, sizeof(slot->buf)); > + if (i < 0) > + return errno; > + if (i == 0) > + return 0; > + if (write_in_full(STDOUT_FILENO, slot->buf, i) < 0) > + return errno; > + } while (1); > } > > /* Check if the slot has expired */ > -- > 2.31.1 >
[-- Attachment #1: Type: text/plain, Size: 4359 bytes --] On Thu, 2021-10-07 at 10:35 +0100, John Keeping wrote: > Have you seen these errors in practice, or is this just theoretical? > > In recent (since v2.6.33) versions of Linux, all files should support > sendfile(), especially since we expect out_fd to be a socket or pipe. Even though I haven't seen any errors from sendfile, I'm not sure if it always works with fuse filesystems and with third-party filesystems like zfs. The sendfile(2) man page says this: "Applications may wish to fall back to read(2)/write(2) in the case where sendfile() fails with EINVAL or ENOSYS." > > On the read/write path, use write_in_full which deals with short > > writes. > > > > Signed-off-by: Hristo Venev <hristo@venev.name> > > --- > > cache.c | 46 ++++++++++++++++++++++++---------------------- > > 1 file changed, 24 insertions(+), 22 deletions(-) > > > > diff --git a/cache.c b/cache.c > > index 55199e8..85cfbd9 100644 > > --- a/cache.c > > +++ b/cache.c > > @@ -85,40 +85,42 @@ static int close_slot(struct cache_slot *slot) > > /* Print the content of the active cache slot (but skip the key). */ > > static int print_slot(struct cache_slot *slot) > > { > > -#ifdef HAVE_LINUX_SENDFILE > > - off_t start_off; > > - int ret; > > + off_t off; > > + ssize_t i; > > + > > + off = slot->keylen + 1; > > > > - start_off = slot->keylen + 1; > > +#ifdef HAVE_LINUX_SENDFILE > > + off_t size; > > decl-after-stmt if HAVE_LINUX_SENDFILE is set. I didn't know that compilers that don't support that exist outside museums. I will fix it in v2. > > + size = slot->cache_st.st_size; > > > > do { > > - ret = sendfile(STDOUT_FILENO, slot->cache_fd, > > &start_off, > > - slot->cache_st.st_size - start_off); > > - if (ret < 0) { > > + i = sendfile(STDOUT_FILENO, slot->cache_fd, &off, > > size - off); > > Why is ret renamed? i is normally a loop index variable, using it for > the return value here is strange, please stick with "ret". I will fix this in v2. > > + if (i < 0) { > > if (errno == EAGAIN || errno == EINTR) > > continue; > > + /* Fall back to read/write on EINVAL */ > > + if (errno == EINVAL) > > + break; > > return errno; > > } > > - return 0; > > + if (off == size) > > + return 0; > > } while (1); > > -#else > > - ssize_t i, j; > > +#endif > > > > - i = lseek(slot->cache_fd, slot->keylen + 1, SEEK_SET); > > - if (i != slot->keylen + 1) > > + if (lseek(slot->cache_fd, off, SEEK_SET) != off) > > return errno; > > > > do { > > - i = j = xread(slot->cache_fd, slot->buf, sizeof(slot- > > >buf)); > > - if (i > 0) > > - j = xwrite(STDOUT_FILENO, slot->buf, i); > > - } while (i > 0 && j == i); > > - > > - if (i < 0 || j != i) > > - return errno; > > - else > > - return 0; > > -#endif > > + i = xread(slot->cache_fd, slot->buf, sizeof(slot- > > >buf)); > > + if (i < 0) > > + return errno; > > + if (i == 0) > > + return 0; > > + if (write_in_full(STDOUT_FILENO, slot->buf, i) < 0) > > + return errno; > > + } while (1); > > } > > > > /* Check if the slot has expired */ > > -- > > 2.31.1 > > [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 858 bytes --]
Hi Hristo,
On Thu, Oct 7, 2021 at 5:45 PM Hristo Venev <hristo@venev.name> wrote:
> I will fix this in v2.
I'm finally culling patches for cgit. Did you want to submit a v2 of this patch?
Jason
[-- Attachment #1: Type: text/plain, Size: 389 bytes --] On Sat, 2022-05-07 at 23:28 +0200, Jason A. Donenfeld wrote: > Hi Hristo, > > On Thu, Oct 7, 2021 at 5:45 PM Hristo Venev <hristo@venev.name> > wrote: > > I will fix this in v2. > > I'm finally culling patches for cgit. Did you want to submit a v2 of > this patch? I submitted it earlier today. https://lists.zx2c4.com/pipermail/cgit/2022-May/004723.html > > Jason [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 833 bytes --]
On Sat, May 7, 2022 at 11:32 PM Hristo Venev <hristo@venev.name> wrote:
> I submitted it earlier today.
>
> https://lists.zx2c4.com/pipermail/cgit/2022-May/004723.html
Thanks. Went to spam because the DKIM signature failed.
Jason