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

Deepstream 5.0 Subscription Implementation #1

Open
wants to merge 32 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
2e29abb
Add msgapi_subscribe implementation
Shubhamturakhia Apr 5, 2021
2a7a3d6
Modify broker.c file
Shubhamturakhia Apr 6, 2021
9a5bc7a
Added subcribe implementation
Shubhamturakhia Apr 7, 2021
b2c6940
Added Subscribe implementation
Shubhamturakhia Apr 7, 2021
bf90453
Minor changes
Shubhamturakhia Apr 7, 2021
67dffec
Changes according to PR review
Shubhamturakhia Apr 7, 2021
5ca5006
Add callback wrapper
Shubhamturakhia Apr 8, 2021
4821a94
Added unsubscribe feature
Shubhamturakhia Apr 12, 2021
adc30df
Changes as per PR review
Shubhamturakhia Apr 12, 2021
f8f0099
Minor change
Shubhamturakhia Apr 14, 2021
7aa84e7
Minor change
Shubhamturakhia Apr 14, 2021
9a8ff42
Fix formatting
Shubhamturakhia Apr 14, 2021
bd3310d
Update aws_protocol_adaptor/device_client/aws_nvmsgbroker.c
Shubhamturakhia Apr 14, 2021
7e4b8d9
Add msgapi connection signature mwthod
Shubhamturakhia Apr 15, 2021
f2cd2b9
Merge branch 'DS-5.0-subscription-implementation' of https://github.c…
Shubhamturakhia Apr 15, 2021
3707004
added connection signature method
Shubhamturakhia Apr 16, 2021
2fd20b1
Minor changes
Shubhamturakhia Apr 16, 2021
072dfbb
Modified utilities for conn signature
Shubhamturakhia Apr 16, 2021
93deacc
Formatting and minor refactoring
Shubhamturakhia Apr 19, 2021
9c1449a
Changes as per PR review
Shubhamturakhia Apr 20, 2021
fa94cc3
Modifications for Hash code
Shubhamturakhia Apr 21, 2021
525ee45
Changes as per review
Shubhamturakhia Apr 21, 2021
579d7b4
Minor indentation change
Shubhamturakhia Apr 21, 2021
d330130
Hash code refactoring
Shubhamturakhia Apr 22, 2021
819cdc0
Add hash code implementation for config file
Shubhamturakhia Apr 23, 2021
0bac762
Fix Incorrect Return Types
lummish Apr 23, 2021
ef454fb
Minor changes
Shubhamturakhia Apr 23, 2021
9c61435
Resolve Compilation Errors
lummish Apr 23, 2021
d9fa5a6
Link OpenSSL in Makefile
lummish Apr 23, 2021
54fff91
Statically Link to libssl and libcrypto
lummish Apr 27, 2021
c99d2de
Reorder SSL Linkages
lummish Apr 27, 2021
0c7895e
Reorder Linkage to SSL Libraries
lummish Apr 27, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions aws_protocol_adaptor/device_client/aws_iot_config.h
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@

#define AWS_IOT_MAX_SEND_INTERVAL_SEC 120 ///< Have to yield every this amount of time before time-out disconnect
#define MAX_SUBSCRIPTIONS 50
#define NVDS_MSGAPI_VERSION 2.0
#define NVDS_MSGAPI_PROTOCOL "AWS"

// Thing Shadow specific configs
#define SHADOW_MAX_SIZE_OF_RX_BUFFER (AWS_IOT_MQTT_RX_BUF_LEN + 1) ///< Maximum size of the SHADOW buffer to store the received Shadow message, including terminating NULL byte.
Expand Down
79 changes: 79 additions & 0 deletions aws_protocol_adaptor/device_client/aws_nvmsgbroker.c
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,12 @@
#include "aws_config_parser.h"
#include "nvds_msgapi.h"
#include "aws_nvmsgbroker.h"
#include <openssl/sha.h>

NvDsMsgApiHandle (*nvds_msgapi_connect_ptr)(char *connection_str, nvds_msgapi_connect_cb_t connect_cb, char *config_path);
NvDsMsgApiErrorType (*nvds_msgapi_send_ptr)(NvDsMsgApiHandle conn, char *topic, const uint8_t *payload, size_t nbuf);
NvDsMsgApiErrorType (*nvds_msgapi_disconnect_ptr)(NvDsMsgApiHandle h_ptr);
NvDsMsgApiErrorType (*nvds_msgapi_connection_signature_ptr)(char *connection_str, char *config_path, char *output_str, int max_len);
static GMutex thread_mutex;
static GQueue *work_queue;
static struct timespec last_send_time_stamp; // this is to make sure we send or yield frequent enough so we do not get disconnected.
Expand Down Expand Up @@ -353,3 +355,80 @@ void nvds_msgapi_do_work(NvDsMsgApiHandle h_ptr)
last_send_time_stamp = current_time_stamp;
return;
}

char *nvds_msgapi_getversion()
{
return (char *)NVDS_MSGAPI_VERSION;
Copy link

Choose a reason for hiding this comment

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

NVDS_MSGAPI_VERSION is not a string literal, it's a float.

Copy link
Author

Choose a reason for hiding this comment

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

According to the documentation, this function returns a string that identifies the nvds_msgapi version, so I implemented according to that, please let me know if you need any changes

Copy link

Choose a reason for hiding this comment

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

That's all fine—the issue was that the macro was of the incorrect type. I see it's now fixed.

}

char *nvds_msgapi_get_protocol_name()
{
return (char *)NVDS_MSGAPI_PROTOCOL;
Copy link

Choose a reason for hiding this comment

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

You shouldn't need to cast this

Copy link
Author

Choose a reason for hiding this comment

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

Just clarifying on this, what should I return instead of this?

Copy link

Choose a reason for hiding this comment

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

That's what you should return, you just don't need to add the explicit cast.

Copy link
Author

Choose a reason for hiding this comment

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

Does that go for the get_version method as well?

Copy link

Choose a reason for hiding this comment

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

Yes

}

bool is_valid_connection_str(char *connection_str, char *&burl, char *&bport)
Copy link

Choose a reason for hiding this comment

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

What are these types? char *&?

Copy link

Choose a reason for hiding this comment

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

Why are you providing burl and bport? I thought you were parsing these from the connection string?

{
if (connection_str == NULL)
{
IOT_ERROR("connection string cant be NULL");
return false;
}

char conn_str[] = connection_str;
int i = 0;

char *token = strtok(conn_str, ";");
char *data[2];

while (token)
{
data[i++] = token;
token = strtok(NULL, ";");
}
Comment on lines +383 to +390
Copy link

Choose a reason for hiding this comment

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

Don't think this will work properly. The second call to strtok shouldn't give you a second token. Instead, try something like this:

int i = 0;
char *data[2];

for (i = 0; i < 2; i++) {
  data[i] = strtok(conn_str, ";");
}

for (i = 0; i < 2; i++)
{
printf("%s\n", data[i]);
}
char *burl = data[0];
char *bport = data[1];

if (burl == "" || bport == "")
{
IOT_ERROR("connection string is invalid. hostname or port is empty\n");
return false;
}
return true;
}

char *generate_sha256_hash(char *str)
{
unsigned char hashval[SHA256_DIGEST_LENGTH];
int len = SHA256_DIGEST_LENGTH * 2 + 1;
char res[len];
Copy link

Choose a reason for hiding this comment

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

res is a statically allocated variable, but you're trying to return it. What do you think will happen as a result?

Copy link

Choose a reason for hiding this comment

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

To remedy the problem, you should supply the output string as an argument to the function. Note: it will need to have space for at least 65 characters (which you'll need to add a check for).

Copy link
Author

Choose a reason for hiding this comment

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

The max_len is actually the length of the output_str and I have added a check in the connection signature to check if has at least 65 characters

Copy link

Choose a reason for hiding this comment

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

Sounds good, but you still need to resolve the issue of returning a statically allocated variable. res is created on the stack, not the heap, so its lifetime is only that of the enclosing scope. The value of res will be invalidated once this function returns.

SHA256_CTX sha256;
SHA256_Init(&sha256);
SHA256_Update(&sha256, str, strlen(str));
SHA256_Final(hashval, &sha256);
for (int i = 0; i < SHA256_DIGEST_LENGTH; i++)
{
res + (i * 2), "%02x", hashval[i];
Copy link

Choose a reason for hiding this comment

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

Was this supposed to be a function call?

}
return res;
}

NvDsMsgApiErrorType nvds_msgapi_connection_signature(char *broker_str, char *cfg, char *output_str, int max_len)
Copy link

Choose a reason for hiding this comment

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

max_len isn't used anywhere. This suggests that your string operations are unsafe.

{
strcpy(output_str, "");
Copy link

Choose a reason for hiding this comment

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

Why?

Copy link
Author

Choose a reason for hiding this comment

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

As per the documentation, the output_str would give the required hash code if successful or return empty string if unsuccessful so just initializing it as empty string and then updating it with code if successful, please let me know if you require any changes

Copy link

Choose a reason for hiding this comment

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

@Shubhamturakhia Got it. Add a comment to explain this line but the code is good.

if (broker_str == NULL || cfg == NULL)
{
IOT_ERROR("nvds_msgapi_connection_signature: broker_str or cfg path cant be NULL\n");
return NVDS_MSGAPI_ERR;
}
char *burl = "", *bport = "";
if (!is_valid_connection_str(broker_str, burl, bport))
Copy link

Choose a reason for hiding this comment

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

Always enclose conditional blocks with curly braces

return NVDS_MSGAPI_ERR;

char *aws_connection_signature = generate_sha256_hash(burl + bport);
Copy link

Choose a reason for hiding this comment

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

Why do you need to split the connection string in the first place? Couldn't you just supply the connection string on its own? Also, this is not how you concatenate strings in c.

strcpy(output_str, aws_connection_signature);
Copy link

Choose a reason for hiding this comment

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

Why call strcpy here instead of using output_str to contain the sha256 hash directly?

return NVDS_MSGAPI_OK;
}
1 change: 1 addition & 0 deletions aws_protocol_adaptor/device_client/aws_nvmsgbroker.h
Original file line number Diff line number Diff line change
Expand Up @@ -43,3 +43,4 @@ NvDsMsgApiErrorType nvds_msgapi_send(NvDsMsgApiHandle conn, char *topic, const u
NvDsMsgApiErrorType nvds_msgapi_send_async(NvDsMsgApiHandle h_ptr, char *topic, const uint8_t *payload, size_t nbuf, nvds_msgapi_send_cb_t send_callback, void *user_ptr);
NvDsMsgApiErrorType nvds_msgapi_subscribe(NvDsMsgApiHandle h_ptr, char **topics, int num_topics, nvds_msgapi_subscribe_request_cb_t cb, void *user_ctx);
void nvds_msgapi_do_work(NvDsMsgApiHandle h_ptr);
NvDsMsgApiErrorType nvds_msgapi_connection_signature(char *broker_str, char *cfg, char *output_str, int max_len);
lummish marked this conversation as resolved.
Show resolved Hide resolved