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.

Sign up for GitHub

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

Jump to bottom

runtime api enforcement #204

Open
wants to merge 27 commits into
base: master
Choose a base branch
from
Open

runtime api enforcement #204

wants to merge 27 commits into from

Conversation

Xavrax
Copy link
Contributor

@Xavrax Xavrax commented Jan 23, 2025

feat: optional runtime api selection

Add PUBNUB_NTF_RUNTIME_SELECTION flag that allows to select API type during the runtime

parfeon
parfeon requested changes Mar 5, 2025
View reviewed changes
Copy link
Contributor

@parfeon parfeon left a comment

Choose a reason for hiding this comment

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

Do we need to change something according to the review?

CMakeLists.txt
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be possible to compare with make/common/source_files.mk, make/posix_source_files.mk, and windows_source_files.mk? I've noticed that some source files are missing and some placed for wrong environment.

Here is some:
${CMAKE_CURRENT_LIST_DIR}/lib/pubnub_dns_codec.c and ${CMAKE_CURRENT_LIST_DIR}/lib/sockets/pbpal_adns_sockets.c - this one used only in Callback API (at least according to the code)
${CMAKE_CURRENT_LIST_DIR}/core/pubnub_alloc_std.c - present in both if and else branches. Should we just move it to the core files list declaration?
${CMAKE_CURRENT_LIST_DIR}/core/pubnub_memory_block.c - for some reason specified only for Callback API but it is general API (at least according to the code)
${CMAKE_CURRENT_LIST_DIR}/posix/pubnub_version_posix.c - can't be added as is to OS_SOURCEFILES because there exists another version file for WITH_CPP: ${CMAKE_CURRENT_LIST_DIR}/cpp/pubnub_version_posix.cpp. Similar situation with version file for Windows which also depends from WITH_CPP

${CMAKE_CURRENT_LIST_DIR}/lib/sockets/pbpal_sockets.c - I've noticed that we don't have this in cmake settings for non-openssl build (at least it was in original make files)
${CMAKE_CURRENT_LIST_DIR}/posix/pbpal_posix_blocking_io.c - is missing for non-openssl POSIX build

${CMAKE_CURRENT_LIST_DIR}/core/pubnub_ssl.c, ${CMAKE_CURRENT_LIST_DIR}/openssl/pbpal_connect_openssl.c, ${CMAKE_CURRENT_LIST_DIR}/openssl/pbpal_openssl.c, ${CMAKE_CURRENT_LIST_DIR}/openssl/pbpal_openssl_blocking_io.c - looks like this one also missing for openssl build

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ues sure.
I found that there are few differences between makefiles and cmake - probably I will need some help with that.

core/pbauto_heartbeat.c
#if defined(PUBNUB_NTF_RUNTIME_SELECTION)
if (PNA_SYNC == pb->api_policy) {
add_heartbeat_in_progress(pb->thumperIndex);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we have early exit from the function if pb->api_policy is PNA_CALLBACK and #if defined(PUBNUB_NTF_RUNTIME_SELECTION) by moving check to the function start? It will let us have only one check and remove similar code from the end of this function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm.. I didn't think about it.
There were so many places that I changes automatically so probably your solution seems to be better.

core/pbpal_ntf_callback_admin.c
@@ -10,8 +10,19 @@

#include "pubnub_log.h"
#include "pubnub_assert.h"
#include "core/pubnub_ntf_enforcement.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we have this include guarded by #if defined(PUBNUB_NTF_RUNTIME_SELECTION)?
Do we need core in the path?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have guard inside the header file so it shouldn't be required.
but core can be definitely removed - probably IDE added that.

core/pubnub_alloc_static.c
@@ -5,6 +5,7 @@
#include "pubnub_log.h"

#include "pbpal.h"
#include "pubnub_ntf_enforcement.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we have this include guarded by #if defined(PUBNUB_NTF_RUNTIME_SELECTION)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as above

core/pubnub_alloc_std.c
@@ -5,6 +5,7 @@
#include "pubnub_log.h"

#include "pbpal.h"
#include "pubnub_ntf_enforcement.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we have this include guarded by #if defined(PUBNUB_NTF_RUNTIME_SELECTION)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as above

core/pubnub_callback_subscribe_loop.c
pubnub_subloop_t* pubnub_subloop_define(pubnub_t* p,
#else
pubnub_subloop_t* pubnub_callback_subloop_define(pubnub_t* p,
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really care about separate name if enforcement enabled?

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, because C mangling doesn't allow me to have the same name of function twice - however I might be wrong because of the lack of knowladge.

core/pubnub_netcore.c
@@ -7,6 +7,7 @@
#include "core/pubnub_log.h"
#include "core/pubnub_ccore.h"
#include "core/pubnub_ccore_pubsub.h"
#include "pubnub_ntf_enforcement.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we guard include with #if defined(PUBNUB_NTF_RUNTIME_SELECTION)?

core/pubnub_ntf_sync.c
@@ -1,5 +1,7 @@
/* -*- c-file-style:"stroustrup"; indent-tabs-mode: nil -*- */
#include "pubnub_ntf_sync.h"
#include "pubnub_api_types.h"
#include "pubnub_ntf_enforcement.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we guard include with #if defined(PUBNUB_NTF_RUNTIME_SELECTION)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as above

core/pubnub_proxy.c
@@ -6,6 +6,7 @@

#include "pubnub_proxy.h"
#include "pubnub_assert.h"
#include "pubnub_ntf_enforcement.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we guard include with #if defined(PUBNUB_NTF_RUNTIME_SELECTION)?

lib/sockets/pbpal_sockets.c
@@ -6,6 +6,7 @@
#include "core/pubnub_netcore.h"
#include "core/pubnub_assert.h"
#include "core/pubnub_log.h"
#include "core/pubnub_ntf_enforcement.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we guard include with #if defined(PUBNUB_NTF_RUNTIME_SELECTION)?

@Xavrax
Copy link
Contributor Author

Xavrax commented Mar 6, 2025

I will add those changes in the meanwhile.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@parfeon parfeon parfeon requested changes

@seba-aln seba-aln Awaiting requested review from seba-aln seba-aln is a code owner

@KGronek-Pubnub KGronek-Pubnub Awaiting requested review from KGronek-Pubnub KGronek-Pubnub is a code owner

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Xavrax @parfeon @KGronek-Pubnub