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

fix crash at port open #1772

Merged
merged 1 commit into from
Jan 11, 2019
Merged

fix crash at port open #1772

merged 1 commit into from
Jan 11, 2019

Conversation

nowakpl
Copy link
Contributor

@nowakpl nowakpl commented Jan 10, 2019

Fix for #1771

Copy link

@ernstnaezer ernstnaezer left a comment

Choose a reason for hiding this comment

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

I can confirm that this patch fixed my issue with espruino/EspruinoTools#89

@reconbot
Copy link
Member

@zbjornson mind taking a look?

@reconbot
Copy link
Member

Thank you for this, I haven't had time to dig too deeply.

Copy link
Contributor

@zbjornson zbjornson left a comment

Choose a reason for hiding this comment

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

Fix makes sense. Odd that that didn't crash for me. Sorry for the mistake!

It would probably be simpler to keep the struct as it was, and change the call signature of setBaudRate to something like

int setBaudRate(const int fd, const int baudRate, char** errorString)

thereby avoiding the need to create a new instance of the baton at all.

Actually, I'm not sure why a copy of the connection options needs to be created in the first place. Is there a reason we can't just use data? I'm not super familiar with this code yet.

@reconbot
Copy link
Member

reconbot commented Jan 10, 2019

I think it used the struct to communicate the error, no reason why it couldn't have the string passed in or a different way to do it

@zbjornson
Copy link
Contributor

zbjornson commented Jan 10, 2019

Just looked more closely -- I was proposing passing a char* by reference for the error data (although I forgot a size parameter earlier). I think that, or changing the hierarchy so that there's a base struct with errorData that all the uv structs inherit from, would be cleaner overall, but will be a bigger commit, so: I'd go with this PR as-is. I might propose a clean-up PR later (but understand if you want to minimize changes to minimize risk too).

I also see now why this didn't crash for me -- there's no equivalent error in serialport_win.cpp.

zbjornson added a commit to zbjornson/node-serialport that referenced this pull request Jan 10, 2019
@reconbot reconbot merged commit 415891c into serialport:master Jan 11, 2019
@reconbot
Copy link
Member

I have tested this with some hardware, we're good! thank you so much @nowakpl [email protected] has been released

@lock lock bot locked as resolved and limited conversation to collaborators Jul 11, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants