Skip to content

Hotfix of local_ioctl for fcntl with O_NONBLOCK to work#8728

Closed
jturnsek wants to merge 1 commit intoapache:masterfrom
edgepro-dev:hotfix/local_sockif
Closed

Hotfix of local_ioctl for fcntl with O_NONBLOCK to work#8728
jturnsek wants to merge 1 commit intoapache:masterfrom
edgepro-dev:hotfix/local_sockif

Conversation

@jturnsek
Copy link
Copy Markdown
Contributor

@jturnsek jturnsek commented Mar 5, 2023

Summary

Calling fcntl with O_NONBLOCK flag on local socket was not working without this change.

Impact

local_ioctl function

Testing

Working with nng library, I have found the problem and this hotfix has helped.

{
FAR struct local_conn_s *conn;
int ret = OK;
int ret = -ENOTTY;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@xiaoxiang781216 @anchao could you please take a look? I recall some changes around FIONBIO made some time ago related to default behavior.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

it's already be done at line 878.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

it's already be done at line 878.

I do not think it is the same. If you see the first case FIONBIO, I think there is a way to go through this case without setting ret variable, thus ret was set to OK at init and this is not the proper return value in that case. Am I wrong here?
In nuttx-apps repo, I have submitted a PR for nng with Pub/Sub example and you can test it.

Copy link
Copy Markdown
Contributor

@xiaoxiang781216 xiaoxiang781216 Mar 6, 2023

Choose a reason for hiding this comment

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

Oh, in most case, lc_infile/lc_outfile shouldn't NULL. In your case, is both NULL?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I do not remember exactly, because it was quite some time I did the debugging. I will try again the nng example this evening with the latest code and see what happens.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, is a listen socket. NNG code is calling (void) fcntl(fd, F_SETFL, O_NONBLOCK) to set the socket in non block mode. You can see from callstack above the call to fcntl. When local_ioctl returns ENOTTY, netdev_file_ioctl is called next, which is then responsible to set s_flags for the socket. In case of OK, this doesn't happen.

Copy link
Copy Markdown
Contributor

@xiaoxiang781216 xiaoxiang781216 Mar 7, 2023

Choose a reason for hiding this comment

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

Ok, unterstand. #8751 should fix your problem and contain more context information.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Will test with NNG.
Yesterday, when testing NNG PubSub example, I have found out that example doesn't work as it was a few months ago. Although I am getting to nng_recv (before that ENOTTY code blocked in dial) function in client thread, nothing seems to get through, altough server thread is pushing messages. Something must have been changed in local socket implementation?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Do we close this PR?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

yes, if my patch fix your problem.

@jturnsek
Copy link
Copy Markdown
Contributor Author

jturnsek commented Mar 7, 2023

Closing this PR in favor of #8751.

@jturnsek jturnsek closed this Mar 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants