Feature/http range/v23 #6312
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -78,6 +78,7 @@ static int ContainerUrlRangeSet(void *dst, void *src) | |
| } | ||
| RB_INIT(&dst_s->fragment_tree); | ||
| dst_s->flags = 0; | ||
| dst_s->lastsize = 0; | ||
| dst_s->totalsize = 0; | ||
| dst_s->hdata = NULL; | ||
| dst_s->error = false; | ||
|
|
@@ -131,6 +132,7 @@ static inline bool ContainerValueRangeTimeout(HttpRangeContainerFile *cu, struct | |
| // we only timeout if we have no flow referencing us | ||
| if ((uint32_t)ts->tv_sec > cu->expire || cu->error) { | ||
| if (SC_ATOMIC_GET(cu->hdata->use_cnt) == 0) { | ||
| DEBUG_VALIDATE_BUG_ON(cu->fileOwned); | ||
| return true; | ||
| } | ||
| } | ||
|
|
@@ -260,6 +262,11 @@ static HttpRangeContainerBlock *HttpRangeOpenFileAux(HttpRangeContainerFile *c, | |
| DEBUG_VALIDATE_BUG_ON(c->files == NULL); | ||
|
|
||
| if (c->files->tail == NULL) { | ||
| DEBUG_VALIDATE_BUG_ON(c->fileOwned); | ||
| /* this is the first request, we open a single file in the file container | ||
| * this could be part of ContainerUrlRangeSet if we could have | ||
| * all the arguments there | ||
| */ | ||
| if (FileOpenFileWithId(c->files, sbcfg, 0, name, name_len, NULL, 0, flags) != 0) { | ||
| SCLogDebug("open file for range failed"); | ||
| return NULL; | ||
|
|
@@ -270,29 +277,45 @@ static HttpRangeContainerBlock *HttpRangeOpenFileAux(HttpRangeContainerFile *c, | |
| c->error = true; | ||
| return NULL; | ||
| } | ||
| curf->fileOwning = false; | ||
| if (total > c->totalsize) { | ||
| // we grow to the maximum size indicated by different range requests | ||
| // we could add some warning/app-layer event in this case where | ||
| // different range requests indicate different total sizes | ||
| c->totalsize = total; | ||
| } | ||
| const uint64_t buflen = end - start + 1; | ||
|
|
||
| /* The big part of this function is now to decide which kind of HttpRangeContainerBlock | ||
| * we will return : | ||
| * - skipping already processed data | ||
| * - storing out of order data for later use | ||
| * - directly appending to the file if we are at the right offset | ||
| */ | ||
| if (start == c->lastsize && !c->fileOwned) { | ||
| // easy case : append to current file | ||
| curf->container = c; | ||
| curf->fileOwning = true; | ||
| // If we see 2 duplicate range requests with the same range, | ||
| // the first one takes the ownership with fileOwned | ||
| // protected by the lock from caller HTPFileOpenWithRange | ||
| c->fileOwned = true; | ||
| return curf; | ||
| } else if (start < c->lastsize && c->lastsize - start >= buflen) { | ||
| // only overlap | ||
| // redundant to be explicit that this block is independent | ||
| curf->toskip = buflen; | ||
| return curf; | ||
| } else if (start < c->lastsize && c->lastsize - start < buflen && !c->fileOwned) { | ||
| // some overlap, then some data to append to the file | ||
| curf->toskip = c->lastsize - start; | ||
| curf->fileOwning = true; | ||
| c->fileOwned = true; | ||
| curf->container = c; | ||
| return curf; | ||
| } | ||
| // Because we are not in the previous cases, we will store the data for later use | ||
|
|
||
| // block/range to be inserted in ordered linked list | ||
| if (!(THASH_CHECK_MEMCAP(ContainerUrlRangeList.ht, buflen))) { | ||
| // skips this range | ||
|
|
@@ -319,6 +342,8 @@ static HttpRangeContainerBlock *HttpRangeOpenFileAux(HttpRangeContainerFile *c, | |
| } | ||
| range->buflen = buflen; | ||
| range->start = start; | ||
| range->offset = 0; | ||
| range->gap = 0; | ||
| curf->current = range; | ||
| return curf; | ||
| } | ||
|
|
@@ -335,42 +360,47 @@ HttpRangeContainerBlock *HttpRangeOpenFile(HttpRangeContainerFile *c, uint64_t s | |
| return r; | ||
| } | ||
|
|
||
| int HttpRangeAppendData(HttpRangeContainerBlock *c, const uint8_t *data, uint32_t len) | ||
| { | ||
| if (len == 0) { | ||
| return 0; | ||
| } | ||
| // first check if we need to skip all bytes | ||
| if (c->toskip >= len) { | ||
| c->toskip -= len; | ||
| return 0; | ||
| } | ||
| // then if we need to skip only some bytes | ||
| if (c->toskip > 0) { | ||
| int r; | ||
| if (c->fileOwning) { | ||
| DEBUG_VALIDATE_BUG_ON(c->container->files == NULL); | ||
| if (data == NULL) { | ||
| // gap overlaping already known data | ||
| r = FileAppendData(c->container->files, NULL, len - c->toskip); | ||
| } else { | ||
| r = FileAppendData(c->container->files, data + c->toskip, len - c->toskip); | ||
| } | ||
| } | ||
| c->toskip = 0; | ||
| return r; | ||
| } | ||
| // If we are owning the file to append to it, let's do it | ||
| if (c->fileOwning) { | ||
| DEBUG_VALIDATE_BUG_ON(c->container->files == NULL); | ||
| SCLogDebug("update files (FileAppendData)"); | ||
| return FileAppendData(c->container->files, data, len); | ||
| } | ||
| // If we are not in the previous cases, only one case remains | ||
| DEBUG_VALIDATE_BUG_ON(c->current == NULL); | ||
| // So we have a current allocated buffer to copy to | ||
| // in the case of an unordered range being handled | ||
| if (c->current) { | ||
| SCLogDebug("update current: adding %u bytes to block %p", len, c); | ||
| // GAP "data" | ||
| if (data == NULL) { | ||
| // just save the length of the gap | ||
| c->current->gap += len; | ||
| // data, but we're not yet complete | ||
| } else if (c->current->offset + len < c->current->buflen) { | ||
| memcpy(c->current->buffer + c->current->offset, data, len); | ||
|
|
@@ -386,14 +416,7 @@ int HttpRangeAppendData(HttpRangeContainerBlock *c, const uint8_t *data, uint32_ | |
| c->current->offset = c->current->buflen; | ||
| } | ||
| return 0; | ||
| } | ||
| } | ||
|
|
||
| static void HttpRangeFileClose(HttpRangeContainerFile *c, uint16_t flags) | ||
|
|
@@ -413,17 +436,14 @@ File *HttpRangeClose(HttpRangeContainerBlock *c, uint16_t flags) | |
| { | ||
| SCLogDebug("c %p c->container %p c->current %p", c, c->container, c->current); | ||
|
|
||
| DEBUG_VALIDATE_BUG_ON(c->container == NULL); | ||
|
|
||
| /* we're processing an OOO chunk, won't be able to get us a full file just yet */ | ||
| if (c->current) { | ||
| SCLogDebug("processing ooo chunk as c->current is set %p", c->current); | ||
| // some out-or-order range is finished | ||
| if (c->container->files->tail && | ||
| c->container->lastsize >= c->current->start + c->current->offset) { | ||
| // if the range has become obsolete because we received the data already | ||
| // we just free it | ||
| (void)SC_ATOMIC_SUB(ContainerUrlRangeList.ht->memuse, c->current->buflen); | ||
|
|
@@ -438,6 +458,7 @@ File *HttpRangeClose(HttpRangeContainerBlock *c, uint16_t flags) | |
| HTTP_RANGES_RB_INSERT(&c->container->fragment_tree, c->current); | ||
| if (res) { | ||
| SCLogDebug("duplicate range fragment"); | ||
| (void)SC_ATOMIC_SUB(ContainerUrlRangeList.ht->memuse, c->current->buflen); | ||
|
There was a problem hiding this comment. @victorjulien is this correct ? Everywhere else we There was a problem hiding this comment. I don't understand the question. We update the memcap on this free, right? There was a problem hiding this comment.
I added this in this latest PR. There was a problem hiding this comment. Gotcha. This is why I don't like updates to existing PRs btw. |
||
| SCFree(c->current->buffer); | ||
| SCFree(c->current); | ||
| } | ||
|
|
@@ -455,18 +476,30 @@ File *HttpRangeClose(HttpRangeContainerBlock *c, uint16_t flags) | |
| } | ||
|
|
||
| // we just finished an in-order block | ||
| DEBUG_VALIDATE_BUG_ON(c->container->fileOwned == false); | ||
| c->container->fileOwned = false; | ||
| DEBUG_VALIDATE_BUG_ON(c->container->files->tail == NULL); | ||
| // we update the file size now that we own it again | ||
| c->container->lastsize = c->container->files->tail->size; | ||
| File *f = c->container->files->tail; | ||
|
|
||
| /* See if we can use our stored fragments to (partly) reconstruct the file */ | ||
| HttpRangeContainerBuffer *range, *safe = NULL; | ||
| RB_FOREACH_SAFE (range, HTTP_RANGES, &c->container->fragment_tree, safe) { | ||
| if (f->size < range->start) { | ||
| // this next range is not reached yet | ||
| break; | ||
| } | ||
| if (f->size == range->start) { | ||
| // a new range just begins where we ended, append it | ||
| if (range->gap > 0) { | ||
| // if the range had a gap, begin by it | ||
| if (FileAppendData(c->container->files, NULL, range->gap) != 0) { | ||
| HttpRangeFileClose(c->container, flags | FILE_TRUNCATED); | ||
| c->container->error = true; | ||
| return f; | ||
| } | ||
| } | ||
| if (FileAppendData(c->container->files, range->buffer, range->offset) != 0) { | ||
| HttpRangeFileClose(c->container, flags | FILE_TRUNCATED); | ||
| c->container->error = true; | ||
|
|
@@ -476,6 +509,14 @@ File *HttpRangeClose(HttpRangeContainerBlock *c, uint16_t flags) | |
| // the range starts before where we ended | ||
| uint64_t overlap = f->size - range->start; | ||
| if (overlap < range->offset) { | ||
| if (range->gap > 0) { | ||
| // if the range had a gap, begin by it | ||
| if (FileAppendData(c->container->files, NULL, range->gap) != 0) { | ||
| HttpRangeFileClose(c->container, flags | FILE_TRUNCATED); | ||
| c->container->error = true; | ||
| return f; | ||
| } | ||
| } | ||
| // And the range ends beyond where we ended | ||
| // in this case of overlap, only add the extra data | ||
| if (FileAppendData(c->container->files, range->buffer + overlap, | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it looks like this is accessed w/o a lock in the
HTPFileStoreChunkpath. It may work due toc->fileOwning, but I'm not happy with this setup. I don't want to see any access of a shared data structure w/o holding a lock.Like I wrote in another comment, I thought the idea was to move
c->container->filesintoHttpRangeContainerBlockfor the flow handling the in-order chunk. That way we can append to it w/o needing to access the shared data atc->containerThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reworking with the
FileContainerpointer from comment above