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

Add generic functions for configuration #2066

Closed
wants to merge 4 commits into from
Closed

Conversation

IvanNardi
Copy link
Collaborator

TODO

@IvanNardi IvanNardi marked this pull request as draft July 31, 2023 13:35
@IvanNardi IvanNardi force-pushed the config branch 4 times, most recently from 7418858 to fb409cb Compare August 4, 2023 08:11
@sonarcloud
Copy link

sonarcloud bot commented Aug 4, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@IvanNardi
Copy link
Collaborator Author

This code is only a proof-of-concept to illustrate a new way to provide (more) configuration options in nDPI. The important stuff is at the end of ndpi_main.c file: the ndpi_set_config() function and the cfg_params data structure

The idea is to have a simple way to configure (most of) nDPI:

  • only one function to set any configuration parameters (in the present or on in the future)
  • try to keep this function prototype as agnostic as possible.

This way, anytime we need to add a new configuration parameter:

  • we don't need to add two public functions (a getter and a setter)
  • we don't break API/ABI compatibility of the library; even changing the parameter type (from integer to a list of integer, for example) doesn't break the compatibility

Basically ndpi_set_config() is an extended version of ndpi_set_detection_preferences() [if you prefer, we can keep this name...]

[There is a LOT of work to do to have some kind of API/ABI compatibility in nDPI (if we ever want to do that...), and the configuration might be a good (and simple) starting point...]

As example of how to extend the configuration, I added the ability to enable/disable the extraction of the sha1 fingerprint of the TLS certificates.

This logic is not new: lots of libraries already uses it

If this idea is accepted, we might remove ndpi_set_detection_preferences(), enum  ndpi_prefs, ndpi_set_protocol_aggressiveness(), ndpi_set_opportunistic_tls()....

@sonarcloud
Copy link

sonarcloud bot commented Oct 23, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@utoni
Copy link
Collaborator

utoni commented Oct 23, 2023

[There is a LOT of work to do to have some kind of API/ABI compatibility in nDPI (if we ever want to do that...), and the configuration might be a good (and simple) starting point...]

I think there are legitimate reasons to break API and ABI compatibility in some rare case, because there is always be an ongoing development progress and some changes might require to do so.
But as we do not have any API/ABI check in the CI, I think it is not always easy to verify that (I mean in a automated manner).

@utoni
Copy link
Collaborator

utoni commented Oct 23, 2023

Basically ndpi_set_config() is an extended version of ndpi_set_detection_preferences() [if you prefer, we can keep this name...]

But why not e xtending ndpi_set_detection_preferences() and break the API/ABI for once?

@IvanNardi
Copy link
Collaborator Author

[There is a LOT of work to do to have some kind of API/ABI compatibility in nDPI (if we ever want to do that...), and the configuration might be a good (and simple) starting point...]

I think there are legitimate reasons to break API and ABI compatibility in some rare case, because there is always be an ongoing development progress and some changes might require to do so. But as we do not have any API/ABI check in the CI, I think it is not always easy to verify that (I mean in a automated manner).

Sw compatibility across releases is not the topic itself of this PR: I cited it only because it is a topic where we, historically, never paid attention and where we could likely do better.
If we are able to create a configuration API which, as a side effect, guarantees some kind of API/ABI compatibility, I think it would be nice. I stress out "as side effect" part, it is not the primary goal

@IvanNardi
Copy link
Collaborator Author

Basically ndpi_set_config() is an extended version of ndpi_set_detection_preferences() [if you prefer, we can keep this name...]

But why not e xtending ndpi_set_detection_preferences() and break the API/ABI for once?

As I said, the naming is open to discussion: we definitely could keep the old ndpi_set_detection_preferences name...

@IvanNardi
Copy link
Collaborator Author

A simpler and updated version is available #2190

@IvanNardi
Copy link
Collaborator Author

Superseded by #2249

@IvanNardi IvanNardi closed this Jan 15, 2024
@IvanNardi IvanNardi deleted the config branch February 15, 2024 19:17
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.

2 participants