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

log session id #1164

Merged
merged 4 commits into from Feb 1, 2017

Conversation

Projects
None yet
2 participants
@yannick
Contributor

yannick commented Jan 14, 2017

replaces #1147 because somehow i messed that up.

i tried to fix the issues, but i'm unsure about the correct algo for h2o_base64_encode_capacity.

http://stackoverflow.com/questions/1533113/calculate-the-size-to-a-base-64-encoded-message
suggests:
((len * 4) / 3) + (len / 96) + 6

also not sure why the ci fails

@yannick yannick referenced this pull request Jan 14, 2017

Closed

log session_id #1147

@@ -178,6 +178,11 @@ inline int h2o_lcstris(const char *target, size_t target_len, const char *test,
return h2o__lcstris_core(target, test, test_len);
}
inline size_t h2o_base64_encode_capacity(unsigned len)

This comment has been minimized.

@kazuho

kazuho Jan 16, 2017

Member

The reason you are seeing CI failure is because the function is not defined is static. Since it is not defined as static, the function is defined multiple times as each source file includes the header file, ending up with a multiple definition error.

The rule I'd like you to follow is to declare a function as static first, and then define a function as inline. For example, please see the occurrences of h2o_lcstris (you'd find the declaration and the definition within the header file).

@kazuho

kazuho Jan 16, 2017

Member

The reason you are seeing CI failure is because the function is not defined is static. Since it is not defined as static, the function is defined multiple times as each source file includes the header file, ending up with a multiple definition error.

The rule I'd like you to follow is to declare a function as static first, and then define a function as inline. For example, please see the occurrences of h2o_lcstris (you'd find the declaration and the definition within the header file).

@kazuho

This comment has been minimized.

Show comment
Hide comment
@kazuho

kazuho Jan 16, 2017

Member

i'm unsure about the correct algo for h2o_base64_encode_capacity.

http://stackoverflow.com/questions/1533113/calculate-the-size-to-a-base-64-encoded-message
suggests:
((len * 4) / 3) + (len / 96) + 6

The logic we had in #1147 is fine.

The confusion is due to the fact that there are various flavors in how base64 is encoded. Depending on the encoder, the output may include line feeds or trailers (i.e. =).

In case of h2o_base64_encode, we do not split lines or add trailers. But we add a NUL character at the end. h2o_base64_encode_capacity should match the requirement.

And the fact that there are various flavors is why I suggested that the function should be named h2o_base64_encode_capacity instead of something like h2o_base64_capacity. I would like to have some indication in the name of the capacity function that it is bound to a specific base64 encoder.

Member

kazuho commented Jan 16, 2017

i'm unsure about the correct algo for h2o_base64_encode_capacity.

http://stackoverflow.com/questions/1533113/calculate-the-size-to-a-base-64-encoded-message
suggests:
((len * 4) / 3) + (len / 96) + 6

The logic we had in #1147 is fine.

The confusion is due to the fact that there are various flavors in how base64 is encoded. Depending on the encoder, the output may include line feeds or trailers (i.e. =).

In case of h2o_base64_encode, we do not split lines or add trailers. But we add a NUL character at the end. h2o_base64_encode_capacity should match the requirement.

And the fact that there are various flavors is why I suggested that the function should be named h2o_base64_encode_capacity instead of something like h2o_base64_capacity. I would like to have some indication in the name of the capacity function that it is bound to a specific base64 encoder.

@kazuho

This comment has been minimized.

Show comment
Hide comment
@kazuho

kazuho Jan 16, 2017

Member

PS. I would appreciate it if you could extend the end-to-end test in https://github.com/h2o/h2o/blob/v2.1.0-beta4/t/50access-log.t#L147-L173 to cover the proposed extension.

Member

kazuho commented Jan 16, 2017

PS. I would appreciate it if you could extend the end-to-end test in https://github.com/h2o/h2o/blob/v2.1.0-beta4/t/50access-log.t#L147-L173 to cover the proposed extension.

@yannick

This comment has been minimized.

Show comment
Hide comment
@yannick

yannick Jan 16, 2017

Contributor

PS. I would appreciate it if you could extend the end-to-end test in https://github.com/h2o/h2o/blob/v2.1.0-beta4/t/50access-log.t#L147-L173 to cover the proposed extension.

ok will do. thanks for the patience

Contributor

yannick commented Jan 16, 2017

PS. I would appreciate it if you could extend the end-to-end test in https://github.com/h2o/h2o/blob/v2.1.0-beta4/t/50access-log.t#L147-L173 to cover the proposed extension.

ok will do. thanks for the patience

@kazuho kazuho added this to the v2.2 milestone Jan 19, 2017

@kazuho kazuho merged commit 76098c3 into h2o:master Feb 1, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

kazuho added a commit that referenced this pull request Feb 1, 2017

@kazuho

This comment has been minimized.

Show comment
Hide comment
@kazuho

kazuho Feb 1, 2017

Member

Thank you for adding the tests! Merged to master.

Member

kazuho commented Feb 1, 2017

Thank you for adding the tests! Merged to master.

@yannick yannick deleted the tamediadigital:new_features/log-session-id2 branch Feb 1, 2017

kazuho added a commit that referenced this pull request Feb 24, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment