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

net: Remove IFF_DOWN flag to compatible with Linux/*BSD #2647

Merged
merged 1 commit into from
Oct 7, 2024

Conversation

xiaoxiang781216
Copy link
Contributor

Summary

turn off interface by checking IFF_UP flag isn't set: apache/nuttx#1838
depends on apache/nuttx#13842

Impact

improve the combability

Testing

internal test

turn off interface by checking IFF_UP flag isn't set:
apache/nuttx#1838

Signed-off-by: Xiang Xiao <[email protected]>
@nuttxpr
Copy link

nuttxpr commented Oct 6, 2024

[Experimental Bot, please feedback here]

No, the provided PR summary does not meet the NuttX Requirements.

Here's why and how to improve it:

Missing Information in Summary:

  • What functional part is changed? Be specific. Which network driver? Which part of the networking stack?
  • How does it work? Explain the logic of checking IFF_UP to turn off an interface. Don't assume reviewers understand the context from just the issue number.
  • NuttX Apps Issue/PR: If this change affects any NuttX applications, provide those references.

Impact:

  • Improve Compatibility: This is too vague. How does it improve compatibility? With what? Be specific.
  • Other Impact sections: Many are likely relevant:
    • Existing Feature Changed? Turning off an interface is likely an existing feature, so "YES" and describe how the behavior is modified.
    • Impact on User? Will users need to change configurations or how they interact with the network stack due to this change?
    • Impact on Hardware? Does this affect specific architectures or network hardware?
    • Documentation? If the behavior changed, documentation likely needs updating.

Testing:

  • "Internal test" is not sufficient. Provide details:
    • Host OS and Compiler: (e.g., Linux, GCC 12.2.1)
    • Target Architecture and Board: (e.g., ARM, STM32F4 Discovery)
    • Relevant Log Snippets: Show the problem before the change and the improvement after. Highlight the key differences.

Example of an Improved Summary Section:

## Summary

This PR addresses a compatibility issue where network interfaces were not being correctly disabled, as reported in issue #1838. 

The change modifies the `[driver_name]` network driver's `[function_name]` function (which handles disabling the interface) to check the `IFF_UP` flag. If the flag is not set, indicating the interface is already down, the driver will now skip the steps to bring it down, preventing potential errors or unexpected behavior.

This PR depends on the changes introduced in #13842, which ensures the `IFF_UP` flag is correctly managed.

Remember: A good PR description saves everyone time and increases the chances of your contribution being accepted quickly.

Copy link

@cederom cederom left a comment

Choose a reason for hiding this comment

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

Thank you @xiaoxiang781216 :-)

@xiaoxiang781216 xiaoxiang781216 linked an issue Oct 7, 2024 that may be closed by this pull request
1 task
@GUIDINGLI GUIDINGLI merged commit 9b8a027 into apache:master Oct 7, 2024
28 checks passed
@xiaoxiang781216 xiaoxiang781216 deleted the adb branch October 7, 2024 12:22
@fdcavalcanti
Copy link
Contributor

fdcavalcanti commented Oct 7, 2024

@xiaoxiang781216 could you please check for issues on ifconfig? Seems it is not properly changing the information regarding UP, DOWN, RUNNING.
I'm trying to ifdown and ifconfig shows it is still up.

Here's what I had before this change:

nsh> ifup wlan0
ifup wlan0...OK
nsh> ifconfig
wlan0	Link encap:Ethernet HWaddr 60:55:f9:f6:06:04 at UP mtu 576
	inet addr:10.0.0.2 DRaddr:10.0.0.1 Mask:255.255.255.0

nsh> ifdown wlan0
ifdown wlan0...OK
nsh> ifconfig wlan0
wlan0	Link encap:Ethernet HWaddr 60:55:f9:f6:06:04 at DOWN mtu 576
	inet addr:10.0.0.2 DRaddr:10.0.0.1 Mask:255.255.255.0

nsh> 

Here's what I have now, seems to be stuck:

nsh> ifconfig
wlan0	Link encap:Ethernet HWaddr 60:55:f9:f6:06:04 at UP mtu 576
	inet addr:10.0.0.2 DRaddr:10.0.0.1 Mask:255.255.255.0

nsh> ifdown wlan0
ifdown wlan0...OK
nsh> ifconfig
wlan0	Link encap:Ethernet HWaddr 60:55:f9:f6:06:04 at UP mtu 576
	inet addr:10.0.0.2 DRaddr:10.0.0.1 Mask:255.255.255.0

Noticed that the PR it depends on has not been merged. I'll wait until it is and let you know if all good.

@xiaoxiang781216
Copy link
Contributor Author

Thanks for reporting, I am looking into it.

@zhhyu7
Copy link
Contributor

zhhyu7 commented Oct 8, 2024

@fdcavalcanti Whether your local environment contains the patch apache/nuttx#13842 ?

@fdcavalcanti
Copy link
Contributor

Yes, just checked test reports and it's ok. Thanks.

@@ -60,7 +60,7 @@ void alt1250_netdev_unregister(FAR struct alt1250_s *dev)

void alt1250_netdev_ifdown(FAR struct alt1250_s *dev)
{
dev->net_dev.d_flags = IFF_DOWN;
dev->net_dev.d_flags = ~IFF_UP;
Copy link
Contributor

Choose a reason for hiding this comment

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

@xiaoxiang781216
Sorry for the delay, let me confirm.
Is this as you intended? Or is it a mistake?
dev->net_dev.d_flags &= ~IFF_UP;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it's intended to follow how other POSIX OS turn off netdev. After IFF_DOWN is removed, ~IFF_UP represent down.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] wapi_set_ifdown failed
7 participants