Skip to content

Support for RFC 7233 (Range Requests). #318

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jtjbrady
Copy link
Contributor

This pull request adds support for HTTP range requests to handleFileDownload.

Range requests allow a client to request only a portion of a file to be transferred. What I actually use this for is for transferring data or log files which grow over time, rather than retreiving the whole file every time, this allows the client to resume the download.

If you're happy to add support for range requests I'll write some unit tests.

Copy link
Member

@dfaure-kdab dfaure-kdab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No objections in principle, assuming unittests indeed.

BTW CI says: ```
KDSoap/src/KDSoapServer/KDSoapServerSocket.cpp:498:58: error: Mixing iterators with const_iterators [-Wclazy-strict-iterators]
next = requestedRanges.erase(next);

@jtjbrady
Copy link
Contributor Author

Yes, I spotted that one, I guess you don't see the "review", maybe it only goes to the author, so I asked myself to review it?
What I said was:

Is the correct change to stop clazy complaining about this

next = requestedRanges.erase(QVector<QPair<int, int>>::const_iterator(next));

I'm assuming this is a clazy false positive or am I missing something?

That part of the code is:

                    // Coalesce
                    for (auto first = requestedRanges.begin(), next = first + 1; next != requestedRanges.end();) {
                        if (next->first <= first->second) {
                            // Next is directly after (or overlaps) first, so merge it into first.
                            first->second = qMax(first->second, next->second);
                            next = requestedRanges.erase(next);
                        } else {
                            first = next;
                            ++next;
                        }
                    }

As far as I am concerned "first" has to be an iterator not a const_iterator because of the line first->second = qMax(first->second, next->second);
The erase function has always confused me to a degree by taking a const_iterator parameter and returning an iterator.

@dfaure-kdab
Copy link
Member

The erase function has always confused me to a degree by taking a const_iterator parameter and returning an iterator.

Then clazy is technically correct, this code does convert a const_iterator into an iterator ;-) But yes it's a kind of false positive because it's a case where it doesn't matter (IIRC the goal of detecting these conversions is to avoid implicit detaching in iterator-based for loops).

We have the same issue in our in-house application, you can add the same suppression:

it = m_entries.erase(it); // clazy:exclude=strict-iterators

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants