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

Change the way NDPI_API_VERSION generated. #1499

Closed
wants to merge 2 commits into from
Closed

Change the way NDPI_API_VERSION generated. #1499

wants to merge 2 commits into from

Conversation

socketpair
Copy link

No description provided.

@sonarcloud
Copy link

sonarcloud bot commented Mar 26, 2022

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

src/include/ndpi_typedefs.h \
src/include/ndpi_protocol_ids.h \
src/include/ndpi_api.h.in \
| md5sum | cut -c1-16 \
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it better to be extra cautious here and add all the headers in src/include/?
For example, ndpi_define.h.in and ndpi_patricia_typedefs.hare definitely needed, not sure about the others...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Am I right that by replacing the time based approach with a checksum based one, the API version is now not constantly increasing? That may break (sooner or later) applications which use nDPI's dev branch for compilation.

Copy link
Author

@socketpair socketpair Mar 28, 2022

Choose a reason for hiding this comment

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

It always has been random. (by number of lines in files). So I did not make it worse.

Copy link
Author

Choose a reason for hiding this comment

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

  1. @IvanNardi Please give me comprehensive list of files to include.
  2. @lnslbrty Time-based logic does not work in presence of branches and backports. Or, we should use "epoch" increased by hand. Actually, this is not needed because backward-compatibiliy is absent, and also API versions are never compared in the code. (only == and not < or >).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually, this is not needed because backward-compatibiliy is absent, and also API versions are never compared in the code. (only == and not < or >).

The API version may not be used by libnDPI itself. But other developers may use it for their own applications. The current API version is pretty much obsolete. But making it even more obsolete shouldn't be the goal, instead I would pledge to remove the API version.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, removing API version looks OK to me.

@@ -1108,7 +1109,7 @@ int main(int argc, char ** argv)
printf("usage: %s [PCAP-FILE-OR-INTERFACE]\n"
"----------------------------------\n"
"nDPI version: %s\n"
" API version: %u\n"
" API version: %" PRIu64 "\n"
Copy link
Collaborator

Choose a reason for hiding this comment

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

PRIu64 may not work for all Windows CCs.
Better use %llu and cast (if necessary) the format param to (unsigned long long int).

Copy link
Author

Choose a reason for hiding this comment

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

How is it possible it may not work? It's the standard. No? Also, what is Windows CC ?

Copy link
Collaborator

@utoni utoni Mar 28, 2022

Choose a reason for hiding this comment

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

MSVC wants %I64u :/

%llu should be the most portable way to print 64-bit uint's

src/include/ndpi_typedefs.h \
src/include/ndpi_protocol_ids.h \
src/include/ndpi_api.h.in \
| md5sum | cut -c1-16 \
Copy link
Collaborator

Choose a reason for hiding this comment

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

Am I right that by replacing the time based approach with a checksum based one, the API version is now not constantly increasing? That may break (sooner or later) applications which use nDPI's dev branch for compilation.

@utoni
Copy link
Collaborator

utoni commented May 3, 2022

I am closing this PR due to inactivity.
There is also a decision to make:

  1. keep the API_VERSION as it is
  2. remove API_VERSION w/o replacement
  3. increase the size of the API_VERSION, but let it constantly increase (may not work as API_VERSION generation differs for Git-Sources and Dist-Tarballs)
  4. increase the size of the API_VERSION and use a hash based approach (basically this PR)
  5. remove API_VERSION and use semantic versioning (the method I would prefer, but results in some additional overhead)

@utoni utoni closed this May 3, 2022
utoni added a commit to utoni/nDPI that referenced this pull request May 16, 2022
 * Related to ntop#1545 and ntop#1494 as well

Signed-off-by: Toni Uhlig <matzeton@googlemail.com>
utoni added a commit to utoni/nDPI that referenced this pull request May 16, 2022
…ocketpair).

 * Related to ntop#1545, ntop#1494 and ntop#1189 as well

Signed-off-by: Toni Uhlig <matzeton@googlemail.com>
utoni added a commit to utoni/nDPI that referenced this pull request May 16, 2022
…ocketpair).

 * Related to ntop#1545, ntop#1494 and ntop#1189 as well

Signed-off-by: Toni Uhlig <matzeton@googlemail.com>
utoni added a commit to utoni/nDPI that referenced this pull request May 16, 2022
…ocketpair).

 * Related to ntop#1545, ntop#1494 and ntop#1189 as well

Signed-off-by: Toni Uhlig <matzeton@googlemail.com>
IvanNardi pushed a commit that referenced this pull request May 16, 2022
…tpair).

 * Related to #1545, #1494 and #1189 as well

Signed-off-by: Toni Uhlig <matzeton@googlemail.com>
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