* Bug: rawdiff output truncated on cache save @ 2017-04-24 15:54 mricon 2017-04-24 18:52 ` [PATCH] cache: flush stdio before restoring FDs john 0 siblings, 1 reply; 5+ messages in thread From: mricon @ 2017-04-24 15:54 UTC (permalink / raw) Hello, all: Very reminiscent of a fix for ui-patch.c that fixed patch truncation [1] there's a similar problem with cached rawdiff output. E.g.: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/rawdiff/?id=v4.11-rc7&id2=v4.11-rc8 You will notice that the output is truncated (if it isn't just reload to get the cached version). I would love a speedy fix, as we're trying to use this in place of publishing pre-generated patches. Best, -Konstantin [1] https://git.zx2c4.com/cgit/commit/?id=2efb59ed -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: not available URL: <http://lists.zx2c4.com/pipermail/cgit/attachments/20170424/eba0c8be/attachment.asc> ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] cache: flush stdio before restoring FDs 2017-04-24 15:54 Bug: rawdiff output truncated on cache save mricon @ 2017-04-24 18:52 ` john 2017-04-24 18:55 ` Jason ` (2 more replies) 0 siblings, 3 replies; 5+ messages in thread From: john @ 2017-04-24 18:52 UTC (permalink / raw) As described in commit 2efb59e (ui-patch: Flush stdout after outputting data, 2014-06-11), we need to ensure that stdout is flushed before restoring the file descriptor when writing to the cache. It turns out that it's not just ui-patch that is affected by this but also raw diff which writes to stdout internally. Let's avoid risking more places doing this by ensuring that stdout is flushed after writing in fill_slot(). Signed-off-by: John Keeping <john at keeping.me.uk> --- On Mon, Apr 24, 2017 at 11:54:40AM -0400, Konstantin Ryabitsev wrote: > Very reminiscent of a fix for ui-patch.c that fixed patch truncation [1] > there's a similar problem with cached rawdiff output. E.g.: > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/rawdiff/?id=v4.11-rc7&id2=v4.11-rc8 > > You will notice that the output is truncated (if it isn't just reload to > get the cached version). > > I would love a speedy fix, as we're trying to use this in place of > publishing pre-generated patches. I guess it's the same problem we fixed before for ui-patch, so here's a patch to move the fix into the common path for all pages. cache.c | 6 ++++++ ui-patch.c | 2 -- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/cache.c b/cache.c index 6736a01..2ccdc4e 100644 --- a/cache.c +++ b/cache.c @@ -224,6 +224,12 @@ static int fill_slot(struct cache_slot *slot) /* Generate cache content */ slot->fn(); + /* Make sure any buffered data is flushed to the file */ + if (fflush(stdout)) { + close(tmp); + return errno; + } + /* update stat info */ if (fstat(slot->lock_fd, &slot->cache_st)) { close(tmp); diff --git a/ui-patch.c b/ui-patch.c index 047e2f9..6745b69 100644 --- a/ui-patch.c +++ b/ui-patch.c @@ -92,6 +92,4 @@ void cgit_print_patch(const char *new_rev, const char *old_rev, log_tree_commit(&rev, commit); printf("-- \ncgit %s\n\n", cgit_version); } - - fflush(stdout); } -- 2.12.2.648.g6730d8bc62.dirty ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] cache: flush stdio before restoring FDs 2017-04-24 18:52 ` [PATCH] cache: flush stdio before restoring FDs john @ 2017-04-24 18:55 ` Jason 2017-04-28 16:36 ` mricon 2017-09-25 12:51 ` mricon 2 siblings, 0 replies; 5+ messages in thread From: Jason @ 2017-04-24 18:55 UTC (permalink / raw) Hey John, Wow, that was quick. Konstantin and I were just talking about this. I'll review this and merge things asap. Another related thing were were discussing and I was just investigating is the inherent buffering that our cache system uses. When it's filling a slot, everything is buffered until the end, when the slot is printed. This doesn't work well for huge tars, which might buffer for more than an HTTP timeout. I'm looking into calling fork() before slot->fn() and then calling splice in the parent... Jason ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] cache: flush stdio before restoring FDs 2017-04-24 18:52 ` [PATCH] cache: flush stdio before restoring FDs john 2017-04-24 18:55 ` Jason @ 2017-04-28 16:36 ` mricon 2017-09-25 12:51 ` mricon 2 siblings, 0 replies; 5+ messages in thread From: mricon @ 2017-04-28 16:36 UTC (permalink / raw) On Mon, Apr 24, 2017 at 2:57 PM John Keeping <john at keeping.me.uk> wrote: > As described in commit 2efb59e (ui-patch: Flush stdout after outputting > data, 2014-06-11), we need to ensure that stdout is flushed before > restoring the file descriptor when writing to the cache. It turns out > that it's not just ui-patch that is affected by this but also raw diff > which writes to stdout internally. > > Let's avoid risking more places doing this by ensuring that stdout is > flushed after writing in fill_slot(). > Thanks, John, this is working and fixes the truncation bug for both patches and rawdiffs. -Konstantin -- Konstantin Ryabitsev Director, IT Infrastructure Security The Linux Foundation +1-971-258-2363 -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.zx2c4.com/pipermail/cgit/attachments/20170428/2d549480/attachment.html> ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] cache: flush stdio before restoring FDs 2017-04-24 18:52 ` [PATCH] cache: flush stdio before restoring FDs john 2017-04-24 18:55 ` Jason 2017-04-28 16:36 ` mricon @ 2017-09-25 12:51 ` mricon 2 siblings, 0 replies; 5+ messages in thread From: mricon @ 2017-09-25 12:51 UTC (permalink / raw) Hi, all: As far as I can tell, the fix for this is still not in any released version of cgit, so the rawdiff cache truncation bug is still present in the latest release (as I just found out because EPEL's cgit has the unpatched version of the package). Best, -K On Mon, Apr 24, 2017 at 07:52:40PM +0100, John Keeping wrote: >As described in commit 2efb59e (ui-patch: Flush stdout after outputting >data, 2014-06-11), we need to ensure that stdout is flushed before >restoring the file descriptor when writing to the cache. It turns out >that it's not just ui-patch that is affected by this but also raw diff >which writes to stdout internally. > >Let's avoid risking more places doing this by ensuring that stdout is >flushed after writing in fill_slot(). > >Signed-off-by: John Keeping <john at keeping.me.uk> >--- >On Mon, Apr 24, 2017 at 11:54:40AM -0400, Konstantin Ryabitsev wrote: >> Very reminiscent of a fix for ui-patch.c that fixed patch truncation [1] >> there's a similar problem with cached rawdiff output. E.g.: >> >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/rawdiff/?id=v4.11-rc7&id2=v4.11-rc8 >> >> You will notice that the output is truncated (if it isn't just reload to >> get the cached version). >> >> I would love a speedy fix, as we're trying to use this in place of >> publishing pre-generated patches. > >I guess it's the same problem we fixed before for ui-patch, so here's a >patch to move the fix into the common path for all pages. > > cache.c | 6 ++++++ > ui-patch.c | 2 -- > 2 files changed, 6 insertions(+), 2 deletions(-) > >diff --git a/cache.c b/cache.c >index 6736a01..2ccdc4e 100644 >--- a/cache.c >+++ b/cache.c >@@ -224,6 +224,12 @@ static int fill_slot(struct cache_slot *slot) > /* Generate cache content */ > slot->fn(); > >+ /* Make sure any buffered data is flushed to the file */ >+ if (fflush(stdout)) { >+ close(tmp); >+ return errno; >+ } >+ > /* update stat info */ > if (fstat(slot->lock_fd, &slot->cache_st)) { > close(tmp); >diff --git a/ui-patch.c b/ui-patch.c >index 047e2f9..6745b69 100644 >--- a/ui-patch.c >+++ b/ui-patch.c >@@ -92,6 +92,4 @@ void cgit_print_patch(const char *new_rev, const char *old_rev, > log_tree_commit(&rev, commit); > printf("-- \ncgit %s\n\n", cgit_version); > } >- >- fflush(stdout); > } ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-09-25 12:51 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-04-24 15:54 Bug: rawdiff output truncated on cache save mricon 2017-04-24 18:52 ` [PATCH] cache: flush stdio before restoring FDs john 2017-04-24 18:55 ` Jason 2017-04-28 16:36 ` mricon 2017-09-25 12:51 ` mricon
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).