buffering: Don't execute move-handle-ony case if handle is of metadata type (atomic) and must be kept fully buffered. Manage handle corruption guard and handle buffering with one set of logic which allows reading of the maximum amount of data without overflow. 'FIXME' regarding handle corruption guard is really part of expected operation when thread that does the handle closing hasn't yet performed the delegated task before rebuffering starts.

git-svn-id: svn://svn.rockbox.org/rockbox/trunk@29270 a1c6a512-1295-4272-9138-f99709370657

1 files changed, 17 insertions, 21 deletions

diff --git a/apps/buffering.c b/apps/buffering.c
index 1482d6790b..14db47bb91 100644
--- a/apps/buffering.c
+++ b/apps/buffering.c
@@ -666,31 +666,25 @@ static bool buffer_handle(int handle_id)
     while (h->filerem > 0 && !stop)
     {
         /* max amount to copy */
-        size_t copy_n = MIN( MIN(h->filerem, BUFFERING_DEFAULT_FILECHUNK),
+        ssize_t copy_n = MIN( MIN(h->filerem, BUFFERING_DEFAULT_FILECHUNK),
                              buffer_len - h->widx);
+        uintptr_t offset = h->next ? ringbuf_offset(h->next) : buf_ridx;
+        ssize_t overlap = ringbuf_add_cross(h->widx, copy_n, offset);
 
-        ssize_t overlap;
-        uintptr_t next_handle = ringbuf_offset(h->next);
+        if (!h->next)
+            overlap++; /* sub one more below to avoid buffer overflow */
 
-        /* stop copying if it would overwrite the reading position */
-        if (ringbuf_add_cross(h->widx, copy_n, buf_ridx) >= 0)
-            return false;
-
-        /* FIXME: This would overwrite the next handle
-         * If this is true, then there's a handle even though we have still
-         * data to buffer. This should NEVER EVER happen! (but it does :( ) */
-        if (h->next && (overlap
-                = ringbuf_add_cross(h->widx, copy_n, next_handle)) > 0)
+        if (overlap > 0)
         {
-            /* stop buffering data for now and post-pone buffering the rest */
+            /* read only up to available space and stop if it would overwrite
+               the reading position or the next handle */
             stop = true;
-            DEBUGF( "%s(): Preventing handle corruption: h1.id:%d h2.id:%d"
-                    " copy_n:%lu overlap:%ld h1.filerem:%lu\n", __func__,
-                    h->id, h->next->id, (unsigned long)copy_n, (long)overlap,
-                    (unsigned long)h->filerem);
             copy_n -= overlap;
         }
 
+        if (copy_n <= 0)
+            return false; /* no space for read */
+
         /* rc is the actual amount read */
         int rc = read(h->fd, &buffer[h->widx], copy_n);
 
@@ -830,12 +824,14 @@ static void shrink_handle(struct memory_handle *h)
     if (!h)
         return;
 
-    if (h->next && h->filerem == 0 &&
-            (h->type == TYPE_ID3 || h->type == TYPE_CUESHEET ||
-             h->type == TYPE_BITMAP || h->type == TYPE_CODEC ||
-             h->type == TYPE_ATOMIC_AUDIO))
+    if (h->type == TYPE_ID3 || h->type == TYPE_CUESHEET ||
+        h->type == TYPE_BITMAP || h->type == TYPE_CODEC ||
+        h->type == TYPE_ATOMIC_AUDIO)
     {
         /* metadata handle: we can move all of it */
+        if (!h->next || h->filerem != 0)
+            return; /* Last handle or not finished loading */
+
         uintptr_t handle_distance =
             ringbuf_sub(ringbuf_offset(h->next), h->data);
         delta = handle_distance - h->available;