Implement multi-block upload #747

szczys · 2025-02-13T19:06:11Z

Multi-block upload implements a blockwise upload using repeated API calls:

Create a context
Call the multi-block set API once for each block in the upload
Destroy the context

github-actions · 2025-02-13T19:23:37Z

Visit the preview URL for this PR (updated for commit a3ed77d):

https://golioth-firmware-sdk-doxygen-dev--pr747-szczys-impleme-7rd9tksg.web.app

_{(expires Sat, 22 Feb 2025 18:52:15 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

_{Sign: a9993e61697a3983f3479e468bcb0b616f9a0578}

sam-golioth · 2025-02-13T21:53:34Z

src/coap_blockwise.h

+/* Blockwise Multi-Part Upload */
+
+// Note: callback_arg will be the same for all blocks
+struct blockwise_transfer_multi *golioth_blockwise_multi_upload_ctx_create(


multi_upload is confusing here, it can imply that we're uploading multiple distinct pieces of data instead of a single piece of data in parts. A generic term for this kind of API is "multipart", and the CoAP terminology is "blockwise", but a multipart API isn't typically explicitly called multipart in the function names. Additionally, the ctx bit is implied by the return type, we don't need it in the function name. All of which is to say, I don't think we need to explicitly call this "multi", this is just an API for doing blockwise uploads.

Suggested change

struct blockwise_transfer_multi *golioth_blockwise_multi_upload_ctx_create(

struct blockwise_transfer *golioth_blockwise_upload_create(...);

int golioth_blockwise_upload_post_block(struct blockwise_transfer *transfer, ...);

void golioth_blockwise_upload_destroy(struct blockwise_transfer *transfer);

Finally, start() and finish() might be clearer than create()/destroy(). Either works for me if you feel strongly, but I lean towards start/finish over create/destroy because start/finish makes it clear that the context is only valid for one transaction, while create/destroy typically indicates that the object may be used repeatedly for the life of the program (like a mutex or semaphore).

sam-golioth · 2025-02-14T17:50:11Z

include/golioth/stream.h

+/// Multi-block transfer context
+///
+/// All memory is owned by user and must persist until the final block is sent
+struct blockwise_transfer_multi;


This should be defined in coap_blockwise.h, otherwise only the stream service will be able to use the blockwise API.

sam-golioth · 2025-02-14T17:58:48Z

src/coap_blockwise.c

 #include <assert.h>
 #include "coap_client.h"
 #include "coap_blockwise.h"

+LOG_TAG_DEFINE(golioth_coap_blockwise);


Let's match the filename and save a few bytes.

Suggested change

LOG_TAG_DEFINE(golioth_coap_blockwise);

LOG_TAG_DEFINE(coap_blockwise);

sam-golioth · 2025-02-14T18:00:35Z

src/coap_blockwise.c

+    memset(ctx->path, 0, sizeof(ctx->path));
+    memcpy(ctx->path, path, strlen(path));


We can just use a single strncpy() call instead of these two calls.

sam-golioth · 2025-02-14T18:02:38Z

src/coap_blockwise.c

@@ -283,6 +311,72 @@ enum golioth_status golioth_blockwise_post(struct golioth_client *client,
    return status;
 }

+// Create an async upload context
+struct blockwise_transfer_multi *golioth_blockwise_multi_upload_ctx_create(


Can we increase code re-use between these new functions and the existing single shot API? The single shot API should be able to use these functions for part of its implementation.

This is a good call-out, thanks!

I added a commit that breaks out almost all initialization into the existing blockwise_upload_init() and also added a blockwise_upload_free(). This way, all block upload functions can use these two functions.

There were a couple of choices made as part of this:

The existing sync function guarantees path is not freed before the the block download completes, but this new multipart approach can't make that guarantee. I added char *stored_path as a context member and store the path there when needed

I have a bit of angst about path_prefix. This is only used internally to the SDK and we pass a #define constant. (But that's not enforced or documented.) As long as we continue this practice and don't pass a prefix from the user API we avoid use-after-free issues. I left this as is.

The two upload approaches use different callback types. I removed callback (and callback_arg) from the init function. These should be set by the caller (if desired) after initialization.

sam-golioth · 2025-02-14T18:03:43Z

src/coap_blockwise.c

@@ -39,6 +42,15 @@ struct blockwise_transfer
    void *callback_arg;
 };

+struct blockwise_transfer_multi


Let's increase code re-use by including this in the context for the single shot API instead of duplicating fields. All of these fields are relevant to a blockwise transfer, whether the API is single shot or multipart.

sam-golioth · 2025-02-14T18:04:15Z

src/coap_blockwise.c

@@ -39,6 +42,15 @@ struct blockwise_transfer
    void *callback_arg;
 };

+struct blockwise_transfer_multi


Since this is the public facing struct, let's keep it simple and clear for users, and rename the context struct for the single shot API.

Suggested change

struct blockwise_transfer_multi

struct blockwise_transfer

Extend `blockwise_upload_init()` the handle memory allocation and return a pointer to an initialized context. Add `blockwise_upload_free()` to free dynamically allocated context. The init function no longer takes a callback/callback_arg. This is because `struct blockwise_transfer` uses a union for the callback member. The appropriate callback type should be set by the caller after initializing the context. Signed-off-by: Mike Szczys <[email protected]>

Caller creates and holds `blockwise_transfer`, supplying path, content_type, and optional callback and callback_arg as parameters. Each block is then sent by repeatedly calling `golioth_blockwise_upload_block()`. The context can be destroyed after the final blocks have been received by the server. Signed-off-by: Mike Szczys <[email protected]>

- Call `golioth_stream_set_blockwise_start()` to create a context. - Call `golioth_stream_set_blockwise_send()` for each block - Call `golioth_stream_set_blockwise_stop()` to destroy the context. Signed-off-by: Mike Szczys <[email protected]>

Signed-off-by: Mike Szczys <[email protected]>

github-actions · 2025-02-15T19:03:57Z

Code Coverage

Package	Line Rate	Branch Rate	Health
include.golioth	75%	50%	✔
port.linux	62%	34%	➖
port.utils	58%	46%	➖
port.zephyr	53%	22%	➖
src	68%	30%	➖
Summary	66% (2756 / 4164)	30% (1142 / 3771)	➖

codecov · 2025-02-15T19:04:11Z

Codecov Report

Attention: Patch coverage is 66.66667% with 23 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
src/coap_blockwise.c	63.49%	14 Missing and 9 partials ⚠️

Files with missing lines	Coverage Δ
src/stream.c	`100.00% <100.00%> (ø)`
src/coap_blockwise.c	`65.42% <63.49%> (-1.06%)`	⬇️

... and 4 files with indirect coverage changes

szczys force-pushed the szczys/implement-async-block branch from 5a588a4 to 3aa76d1 Compare February 13, 2025 19:22

szczys force-pushed the szczys/implement-async-block branch 5 times, most recently from 061e018 to 4024723 Compare February 13, 2025 20:52

szczys changed the title ~~Implement async block~~ Implement multi-block upload Feb 13, 2025

szczys force-pushed the szczys/implement-async-block branch from 4024723 to c827d51 Compare February 13, 2025 21:00

szczys changed the base branch from szczys/async-block-upload to szczys/multi-block-upload February 13, 2025 21:34

szczys marked this pull request as ready for review February 13, 2025 21:34

szczys requested review from mniestroj, sam-golioth and hasheddan February 13, 2025 21:34

sam-golioth requested changes Feb 14, 2025

View reviewed changes

szczys force-pushed the szczys/implement-async-block branch 2 times, most recently from 82b3e6c to 92d0f89 Compare February 15, 2025 18:47

szczys added 4 commits February 15, 2025 12:51

stream: add multipart upload

07c56b5

- Call `golioth_stream_set_blockwise_start()` to create a context. - Call `golioth_stream_set_blockwise_send()` for each block - Call `golioth_stream_set_blockwise_stop()` to destroy the context. Signed-off-by: Mike Szczys <[email protected]>

hil: add test for multipart upload

a3ed77d

Signed-off-by: Mike Szczys <[email protected]>

szczys force-pushed the szczys/implement-async-block branch from 92d0f89 to a3ed77d Compare February 15, 2025 18:51

szczys requested a review from sam-golioth February 15, 2025 18:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement multi-block upload #747

Implement multi-block upload #747

szczys commented Feb 13, 2025 •

edited

Loading

github-actions bot commented Feb 13, 2025 •

edited

Loading

sam-golioth Feb 13, 2025

sam-golioth Feb 14, 2025

sam-golioth Feb 14, 2025

sam-golioth Feb 14, 2025

sam-golioth Feb 14, 2025

szczys Feb 15, 2025

sam-golioth Feb 14, 2025

sam-golioth Feb 14, 2025

github-actions bot commented Feb 15, 2025

codecov bot commented Feb 15, 2025

-struct blockwise_transfer_multi *golioth_blockwise_multi_upload_ctx_create(
+struct blockwise_transfer *golioth_blockwise_upload_create(...);
+int golioth_blockwise_upload_post_block(struct blockwise_transfer *transfer, ...);
+void golioth_blockwise_upload_destroy(struct blockwise_transfer *transfer);

	LOG_TAG_DEFINE(golioth_coap_blockwise);
	LOG_TAG_DEFINE(coap_blockwise);

		memset(ctx->path, 0, sizeof(ctx->path));
		memcpy(ctx->path, path, strlen(path));

Implement multi-block upload #747

Are you sure you want to change the base?

Implement multi-block upload #747

Conversation

szczys commented Feb 13, 2025 • edited Loading

github-actions bot commented Feb 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 15, 2025

Code Coverage

codecov bot commented Feb 15, 2025

Codecov Report

szczys commented Feb 13, 2025 •

edited

Loading

github-actions bot commented Feb 13, 2025 •

edited

Loading