Skip to content

Conversation

giuseppe
Copy link
Member

ghcr.io converts a multirange request to a 200 response when the
client request too much data.

If the server replies with a 200 status to a partial request then
split the body ignoring any additional content that wasn't requested
by the caller.

Signed-off-by: Giuseppe Scrivano gscrivan@redhat.com

@giuseppe giuseppe force-pushed the partial-support-200-ret-code branch 4 times, most recently from 03ab20d to c998c7c Compare August 25, 2021 16:21
@giuseppe
Copy link
Member Author

ready for review

Copy link
Collaborator

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

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

Thanks!

Looks good overall, primarily I’m unsure what guarantees the right order of chunks for the 200 implementation to work correctly; at least I can’t find anything explicitly promising or ensuring that is the case.

for _, c := range chunks {
if c.Offset != currentOffset {
if c.Offset < currentOffset {
errs <- errors.New("invalid chunk offset specified")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does anything guarantee the chunks are in increasing, and non-overlapping, offset order? If this is a caller’s responsibility, it needs to be very prominently documented throughout the call stack.

add inline documentation to clarify the expectation for the chunks
specified to the GetBlobAt() function.

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
ghcr.io converts a multirange request to a 200 response when the
client request too much data.

If the server replies with a 200 status to a partial request then
split the body ignoring any additional content that wasn't requested
by the caller.

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
@giuseppe giuseppe force-pushed the partial-support-200-ret-code branch from c998c7c to ea19c43 Compare August 25, 2021 19:38
@giuseppe
Copy link
Member Author

thanks for the review! Pushed a new version

Copy link
Collaborator

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks again!

@mtrmac mtrmac mentioned this pull request Aug 25, 2021
@mtrmac mtrmac merged commit 17bb73d into containers:main Aug 25, 2021
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