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

drivers/encx24j600: Implemented missing drop case #10415

Merged
merged 1 commit into from
Mar 11, 2019

Conversation

maribu
Copy link
Member

@maribu maribu commented Nov 16, 2018

Contribution description

The netdev_driver_t::recv implementation of the encx24j600 does not provide the drop feature. This PR adds it.

Testing procedure

It would be nice to add a test application for this, as many drivers are affected...

Issues/PRs references

#10410

The netdev_driver_t::recv implementation of the encx24j600 does not provide the
drop feature. This commit adds it.

Fixes: RIOT-OS#10410
@maribu
Copy link
Member Author

maribu commented Nov 16, 2018

I do not have the hardware to test this PR. This PR is therefore completely untested.

@maribu maribu added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Area: network Area: Networking Area: drivers Area: Device drivers labels Nov 16, 2018
@aabadie
Copy link
Contributor

aabadie commented Dec 20, 2018

Maybe @kaspar030 has the hardware and could test ? since he is the author of this driver.

@jia200x jia200x added Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines labels Jan 7, 2019
@jia200x
Copy link
Member

jia200x commented Jan 7, 2019

I can ACK the fundamentals and code design:

  • buf not NULL => receive, flush and return packet size
  • buf NULL && len == 0 => return packet size
  • buf NULL && len > 0 => flush and return packet size.

@jia200x jia200x added Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines labels Jan 7, 2019
@jia200x
Copy link
Member

jia200x commented Jan 7, 2019

nothing interesting to report on coding style here, and documentation doesn't apply

@maribu
Copy link
Member Author

maribu commented Feb 5, 2019

@bergzand, @smlng and @miri64 also have worked on the driver. Maybe someone of them has the hardware and can test it?

@maribu
Copy link
Member Author

maribu commented Mar 8, 2019

@bergzand, @smlng, @miri64, @kaspar030: Does someone of you have a device for testing?

Also: If there is so little interest in this driver that security fixes do not get merged due to lack of testing, maybe we should consider to rather delete the driver than to continue shipping the vulnerable code?

@smlng
Copy link
Member

smlng commented Mar 11, 2019

I only have the enc28j60, sorry

@bergzand
Copy link
Member

Same here, only the enc28j60 😢

@maribu
Copy link
Member Author

maribu commented Mar 11, 2019

How about we add a feature to the build system to warn about modules that are in desperate need of love? Maybe even with a warning, something like: "If no one stands up to help or at least speaks up to keep this module for three releases, this module is going to be deleted."

To me, this driver as well as a bunch of seemingly no longer used boards should be marked as such. If there is indeed no longer a user of that stuff, maintaining the code would be wasted effort.

(To me it is important that hose warnings would make it in the release. Maybe an end user never cares to test with the current master, but only with the release. If we would add such a message, we should make sure that any potential user gets the hint and ideally contributes the testing result, or donates a piece of hardware to a willing core developer, or something.)

@kaspar030
Copy link
Contributor

I'm testing this now. The driver was basically broken since #8601...

@kaspar030 kaspar030 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Reviewed: 3-testing The PR was tested according to the maintainer guidelines labels Mar 11, 2019
@kaspar030
Copy link
Contributor

Driver still works as before with basic testing.

Copy link
Contributor

@kaspar030 kaspar030 left a comment

Choose a reason for hiding this comment

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

ACK.

@maribu
Copy link
Member Author

maribu commented Mar 11, 2019

Thanks for testing :-)

@kaspar030 kaspar030 merged commit 4dcd958 into RIOT-OS:master Mar 11, 2019
@danpetry danpetry added this to the Release 2019.04 milestone Mar 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: drivers Area: Device drivers Area: network Area: Networking CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants