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

doc: Examples work when data exceeds buffer size #4811

Closed
wants to merge 1 commit into from

Conversation

garrows
Copy link
Contributor

@garrows garrows commented Jan 22, 2016

The crypto examples would only work when the input data was under 16 bytes. Above that, important data was being thrown away and decrypting them would give a difficult to debug error (see below).

Error given when .update data was being discarded:
Error: error:06065064:digital envelope routines:EVP_DecryptFinal_ex:bad decrypt

@mscdex mscdex added crypto Issues and PRs related to the crypto subsystem. doc Issues and PRs related to the documentations. labels Jan 22, 2016
@jasnell
Copy link
Member

jasnell commented Jan 22, 2016

@nodejs/crypto

@bnoordhuis
Copy link
Member

LGTM but the commit log should conform to the guidelines. Can you update it according to CONTRIBUTING.md?

@shigeki
Copy link
Contributor

shigeki commented Jan 22, 2016

I think it is a kind of topics how to write a stream example in a doc rather than crypto feature.
Consistencies with other stream examples in hash and other API needs to be considered. I would like to have a comment from @nodejs/documentation .

@eljefedelrodeodeljefe
Copy link
Contributor

From a doc point of view LGTM. I think we need to pick up the stream docmore globally.

@shigeki
Copy link
Contributor

shigeki commented Jan 22, 2016

@eljefedelrodeodeljefe Okay, thanks. LGTM too.
@garrows Please revise your commit messages as Ben's comment.

@garrows garrows changed the title Examples work when data exceeds buffer size (16 bytes) doc: Examples work when data exceeds buffer size Jan 22, 2016
@garrows
Copy link
Contributor Author

garrows commented Jan 22, 2016

I've updated the commit message. Cheers.

@jasnell
Copy link
Member

jasnell commented Jan 23, 2016

LGTM

1 similar comment
@indutny
Copy link
Member

indutny commented Jan 23, 2016

LGTM

jasnell pushed a commit that referenced this pull request Jan 23, 2016
PR-URL: #4811
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Shigeki Ohtsu <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@jasnell
Copy link
Member

jasnell commented Jan 23, 2016

Landed in 26073dd

@jasnell jasnell closed this Jan 23, 2016
@garrows
Copy link
Contributor Author

garrows commented Jan 24, 2016

Thanks for accepting!
Will this be backported to the current 5.5.0 docs? It should be safe to go back to 4.x.x.

rvagg pushed a commit that referenced this pull request Jan 25, 2016
PR-URL: #4811
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Shigeki Ohtsu <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@bnoordhuis
Copy link
Member

@jasnell added the lts-watch-v4.x label so this should get back-ported.

@MylesBorins
Copy link
Contributor

this commit relies on a number of other doc changes to land in LTS first.

MylesBorins pushed a commit that referenced this pull request Feb 22, 2016
PR-URL: #4811
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Shigeki Ohtsu <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 22, 2016
PR-URL: #4811
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Shigeki Ohtsu <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 2, 2016
PR-URL: #4811
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Shigeki Ohtsu <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: James M Snell <[email protected]>
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
PR-URL: nodejs#4811
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Shigeki Ohtsu <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crypto Issues and PRs related to the crypto subsystem. doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants