eclipse-threadx / netxduo

Eclipse ThreadX - NetXDuo is an advanced, industrial-grade TCP/IP network stack designed specifically for deeply embedded real-time and IoT applications
https://github.com/eclipse-threadx/rtos-docs/blob/main/rtos-docs/netx-duo/index.md
MIT License
247 stars 140 forks source link

nx_azure_iot_json_writer_append_json_text does not take into account required buffer space for commas #47

Closed hwmaier closed 3 years ago

hwmaier commented 3 years ago

When nx_azure_iot_json_writer_append_json_text() is used to write elements of an array, the resulting JSON packet is incomplete. The NX_PACKET's byte count is short of exactly the amount of commas inserted internally by az_json_writer_append_json_text().

This is an issue with the underlaying azure-sdk-for-c and a PR has been submitted here: https://github.com/Azure/azure-sdk-for-c/pull/1905

However as this bug also affects the JSON writing functionality of NetX Duo I believe this should be reported here as well, hoping that a fix can be coordinated between the two Microsoft dev teams and be available with the next NetXDuo release.

ahsonkhan commented 3 years ago

The current implementation is directly accessing internal fields, which should be avoided where possible. The issue is with the total_bytes_written, but that isn't exposed, and is currently meant for testing purposes only, within azure-sdk-for-c. https://github.com/azure-rtos/netxduo/blob/d9d7fb05bd6a7a6b50a54c762e1730d8a8054cbb/addons/azure_iot/nx_azure_iot_json_writer.c#L97-L98

hwmaier commented 3 years ago

Well I am only an end user stumbling over a bug which I reported and provided a fix with my PR. Without a bug fix I cannot proceed with my application.

As an outsider I leave the debate about whether internal data structures should be accessed or not within Microsoft products to the relevant dev teams.

However I would prefer a quick pragmatic solution, and my PR does provide this. It fixes the correctness of the internal data structure and then nx_azure_iot_json_writer_append_json_text() works as expected.

In the long run, maybe exposing _internal.total_bytes_written as an API call would be a solution, that way tests can be added and such a bug would have been picked up by a test case.

ahsonkhan commented 3 years ago

Absolutely! Thank you for flagging the issue and providing the fix. I totally agree that it was the right approach, especially to address customer facing issues like you pointed out. My note above was mainly for contributors to this repo to evaluate current usage of internal-only state. I hadn't realized folks were using and benefiting from the total_bytes_written field in their scenarios :)

hwmaier commented 3 years ago

@ahsonkhan Thank's for the quick resolution. Now I hope the change is rolled into the next NetXDuo release soon.

ahsonkhan commented 3 years ago

cc @yuxin-azrtos, @jeffreyrichter, @rickwinter

TiejunMS commented 3 years ago

cc @hihigupt

hwmaier commented 3 years ago

Fixed in latest 6.1.9 release,