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

Pinging specific iface does not work: Fix broken parentheses in setsockopt call (IDFGH-5676) #7397

Closed
wants to merge 1 commit into from

Conversation

ivmarkov
Copy link
Contributor

The call to setsockopt has the parentheses in a wrong way, which causes the whole functionality of pinging a specific netif instance to fail.

@CLAassistant
Copy link

CLAassistant commented Aug 10, 2021

CLA assistant check
All committers have signed the CLA.

@espressif-bot espressif-bot added the Status: Opened Issue is new label Aug 10, 2021
@github-actions github-actions bot changed the title Pinging specific iface does not work: Fix broken parentheses in setsockopt call Pinging specific iface does not work: Fix broken parentheses in setsockopt call (IDFGH-5676) Aug 10, 2021
@Alvin1Zhang
Copy link
Collaborator

Thanks for your contribution.

@AxelLin
Copy link
Contributor

AxelLin commented Aug 10, 2021

Just remind that v4.3 and v4.2 also need the fix.

@AxelLin
Copy link
Contributor

AxelLin commented Aug 14, 2021

Hi @igrr @Alvin1Zhang

This one is a classic case of one-line-fix which fixes a real bug.
I'm wondering if espressif has some standard process to speed up merging such fix.

I asked this because it may take very long time to apply to impacted branches.

Below is another example of such one-line-fix which fixes a real bug.
#7047 (And people are waiting, e.g. #7047 (comment) )

19 May 2021 Issue reported
7 Jun 2021 Commit to master
30 Jun 2021 Commit to release/v4.3
And sync to github time is slightly longer than the commited time.

PS. I'm not asking to take special care of this PR. (@suda-morris did very quick response on this PR)
I'm asking if you have some standard process to speed up merging such fix.

@igrr
Copy link
Member

igrr commented Aug 14, 2021

@AxelLin Thank you for the suggestion. There is no existing special process for one-liner fixes compared to all other PRs. The basic process is: 1) the PR is assigned to a developer, 2) if the PR doesn't need additional work, the developer will create an internal merge request, adding release notes and any required tests, 3) code owners review the merge request, 4) once the merge request is approved it can be merged, 5) backport merge requests are created and handled in a similar way. Indeed PR handling gets stuck at different points, for example this PR is stuck at step 2, since the developer who is assigned happens to be on leave now. We are looking at options to automate more steps in this process to make the handling of PRs more efficient.

Regarding this specific bug, I don't necessarily agree with you that there is a need to speed up merging into release branches, unless the PR author specifically requests the bug to be fixed on a particular release. For bugs that result in random failures or unreliable behavior, we do need to backport them quickly, since users might encounter this issue without even changing much in their applications that are already deployed. On the other hand, for bugs that result in a certain use case being completely broken, we could argue that if noone has reported such issue for a release branch, the feature (or use case) is simply not used. Of course, we do need to fix the issue in release branches eventually. But due to lower impact, I would classify the urgency of backporting as "normal" rather than "high".

@AxelLin
Copy link
Contributor

AxelLin commented Aug 17, 2021

well, this is indeed not a regression because it actually never work.
However the API is there and if you call ping with specific interface
it will run into Guru Meditation Error: Core 1 panic'ed (LoadProhibited). Exception was unhandled.

@espressif-bot espressif-bot added Status: In Progress Work is in progress Resolution: NA Issue resolution is unavailable Status: Done Issue is done internally Resolution: Done Issue is done internally and removed Status: Opened Issue is new Status: In Progress Work is in progress Resolution: NA Issue resolution is unavailable labels Aug 17, 2021
espressif-bot pushed a commit that referenced this pull request Aug 24, 2021
Fix broken parentheses in setsockopt call

Merges #7397
@Alvin1Zhang
Copy link
Collaborator

Changes have been merged with ee531c7, thanks for your contribution again.

@AxelLin
Copy link
Contributor

AxelLin commented Sep 3, 2021

Would you apply this fix for v4.3 branch? (Unfortunately, v4.3.1 does not include this trivial fix).

espressif-bot pushed a commit that referenced this pull request Oct 12, 2021
Fix broken parentheses in setsockopt call

Merges #7397
espressif-bot pushed a commit that referenced this pull request Oct 12, 2021
Fix broken parentheses in setsockopt call

Merges #7397
@BaaBaaGoat
Copy link

just got fucked by that .V4.3 latest stable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Resolution: Done Issue is done internally Status: Done Issue is done internally
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants