-
Notifications
You must be signed in to change notification settings - Fork 24
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
rpc: avoid use of cgo by hard-coding maxPathSize #42
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems clean
Has anyone resynced from scratch using this PR? It shouldn't affect syncing, but I'm going to do it anyway. |
if len(endpoint) > int(max_path_size) { | ||
log.Warn(fmt.Sprintf("The ipc endpoint is longer than %d characters. ", max_path_size), | ||
// account for null-terminator too | ||
if len(endpoint)+1 > maxPathSize { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you replace it with len(endpoint)+1? The value of maxPathSize stayed the same (108).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check the comment above this line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have not tested this but can if you think it will be helpful.
const ( | ||
// On Linux, sun_path is 108 bytes in size | ||
// see http://man7.org/linux/man-pages/man7/unix.7.html | ||
maxPathSize = int(108) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why int(108) and not just 108?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sumamu did you resolve this question?
These changes are consistent with this discussion here: ethereum/go-ethereum#27447 (comment) |
Implemented ethereum/go-ethereum#27447