Skip to content
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

rgw: adjust the request_uri to support absoluteURI of http request #5774

Closed
wants to merge 1 commit into from

Conversation

wenjun-huang
Copy link

@ghost
Copy link

ghost commented Sep 2, 2015

@VMCloud I'm not able to review the content of the pull request. Just a comment on the commit message: it needs to be Fixes: #12917 instead of Fixes: 12917. You should also update the Signed-off-by with your name and surname instead of wenjunhuang. Thanks !

@ghost ghost added rgw bug-fix labels Sep 2, 2015
@wenjun-huang wenjun-huang force-pushed the wip-12917 branch 2 times, most recently from 59e5a0a to 9bb9260 Compare September 3, 2015 13:16
@wenjun-huang
Copy link
Author

Hi, @dachary
Sorry for my carelessness before, I have update the commit messages and added some comments in the related source code. Thanks!

@ghost
Copy link

ghost commented Sep 3, 2015

@VMCloud thanks :-)

static string get_abs_path(const string &request_uri) {
int beg_pos = request_uri.find("://") + 3;
int len = request_uri.size();
while(beg_pos < len) {

Choose a reason for hiding this comment

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

why not use find here?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, it is a good suggestion.

@liewegas
Copy link
Member

@cbodley @yehudasa @mattbenjamin needs review?

// S3 authorization and some other processes depending on the requestURI
// The absoluteURI can start with "http://", "https://", "ws://" or "wss://"
static string get_abs_path(const string& request_uri) {
const string ABS_PREFIXS[] = {"http://", "https://", "ws://", "wss://"};
Copy link
Contributor

Choose a reason for hiding this comment

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

can we make this array static so we don't construct the std::strings on every call?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, good advice!

@wenjun-huang wenjun-huang deleted the wip-12917 branch February 17, 2016 06:51
@wenjun-huang
Copy link
Author

Have made the const array static

@tchaikov
Copy link
Contributor

@VMCloud did you removed your remote branch of wip-12917 by accident? as a side effect, it closes the pull request automatically.

@wenjun-huang
Copy link
Author

@tchaikov yes, I am trying to reopen it.

@wenjun-huang
Copy link
Author

I have recreate a PR for this bug: #7675

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