Skip to content

Commit

Permalink
Update StorageProperties for S3 support (#41)
Browse files Browse the repository at this point in the history
* Change StorageProperties::filename field to StorageProperties::uri.

* Add `s3_is_supported` field to StoragePropertyMetadata.

* Support file:// URI scheme.

* Include <cstring>

* Validate filename only.

* Set URI without file:// prefix.

* Use `storage_properties_copy`

* Validate URI str and nbytes.

* Add access_key_id and secret_access_key to StorageProperties.

* Update changelog.

* Fix and test `storage_properties_set_access_key_and_secret`.
  • Loading branch information
aliddell authored Apr 16, 2024
1 parent 4444f5c commit 85a5d90
Show file tree
Hide file tree
Showing 14 changed files with 434 additions and 66 deletions.
6 changes: 6 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## Unreleased

### Added

- Users can specify access key ID and secret access key for S3 storage in `StorageProperties`.

### Fixed

- A bug where changing device identifiers for the storage device was not being handled correctly.
Expand All @@ -16,6 +20,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

- `reserve_image_shape` is now called in `acquire_configure` rather than `acquire_start`.
- Users can now specify the names, ordering, and number of acquisition dimensions.
- The `StorageProperties::filename` field is now `StorageProperties::uri`.
- Files can be specified by URI with an optional `file://` prefix.

## 0.2.0 - 2024-01-05

Expand Down
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ This is an Acquire Driver that exposes commonly used devices.
- **tiff** - Streams to a [bigtiff] file. Metadata is stored in the `ImageDescription` tag for each frame as a `json`
string.
- **tiff-json** - Stores the video stream in a *bigtiff* (as above) and stores metadata in a `json` file. Both are
located in a folder identified by the `filename` property.
located in a folder identified by the `uri` property.
- **Trash** - Writes nothing. Discards incoming data.

[bigtiff]: http://bigtiff.org/
Expand Down
118 changes: 91 additions & 27 deletions acquire-core-libs/src/acquire-device-properties/device/props/storage.c
Original file line number Diff line number Diff line change
Expand Up @@ -160,14 +160,14 @@ Error:;
}

int
storage_properties_set_filename(struct StorageProperties* out,
const char* filename,
size_t bytes_of_filename)
storage_properties_set_uri(struct StorageProperties* out,
const char* uri,
size_t bytes_of_uri)
{
const struct String s = { .is_ref = 1,
.nbytes = bytes_of_filename,
.str = (char*)filename };
return copy_string(&out->filename, &s);
.nbytes = bytes_of_uri,
.str = (char*)uri };
return copy_string(&out->uri, &s);
}

int
Expand All @@ -181,6 +181,26 @@ storage_properties_set_external_metadata(struct StorageProperties* out,
return copy_string(&out->external_metadata_json, &s);
}

int
storage_properties_set_access_key_and_secret(struct StorageProperties* out,
const char* access_key_id,
size_t bytes_of_access_key_id,
const char* secret_access_key,
size_t bytes_of_secret_access_key)
{
const struct String s = { .is_ref = 1,
.nbytes = bytes_of_access_key_id,
.str = (char*)access_key_id };
CHECK(copy_string(&out->access_key_id, &s));

const struct String t = { .is_ref = 1,
.nbytes = bytes_of_secret_access_key,
.str = (char*)secret_access_key };
return copy_string(&out->secret_access_key, &t);
Error:
return 0;
}

int
storage_properties_set_dimension(struct StorageProperties* out,
int index,
Expand Down Expand Up @@ -238,16 +258,16 @@ storage_properties_set_enable_multiscale(struct StorageProperties* out,
int
storage_properties_init(struct StorageProperties* out,
uint32_t first_frame_id,
const char* filename,
size_t bytes_of_filename,
const char* uri,
size_t bytes_of_uri,
const char* metadata,
size_t bytes_of_metadata,
struct PixelScale pixel_scale_um,
uint8_t dimension_count)
{
// Allocate and copy filename
memset(out, 0, sizeof(*out)); // NOLINT
CHECK(storage_properties_set_filename(out, filename, bytes_of_filename));
CHECK(storage_properties_set_uri(out, uri, bytes_of_uri));

// Set external metadata
CHECK(storage_properties_set_external_metadata(
Expand All @@ -272,22 +292,37 @@ storage_properties_copy(struct StorageProperties* dst,
{
// 1. Copy everything except the strings
{
struct String tmp_fname, tmp_meta;
memcpy(&tmp_fname, &dst->filename, sizeof(struct String)); // NOLINT
memcpy(&tmp_meta, // NOLINT
struct String tmp_uri, tmp_meta, tmp_access_key, tmp_secret_key;
memcpy(&tmp_uri, &dst->uri, sizeof(struct String)); // NOLINT
memcpy(&tmp_meta, // NOLINT
&dst->external_metadata_json,
sizeof(struct String));
memcpy(dst, src, sizeof(*dst)); // NOLINT
memcpy(&dst->filename, &tmp_fname, sizeof(struct String)); // NOLINT
memcpy(&dst->external_metadata_json, // NOLINT
memcpy(&tmp_access_key,
&dst->access_key_id,
sizeof(struct String)); // NOLINT
memcpy(&tmp_secret_key,
&dst->secret_access_key,
sizeof(struct String)); // NOLINT

memcpy(dst, src, sizeof(*dst)); // NOLINT
memcpy(&dst->uri, &tmp_uri, sizeof(struct String)); // NOLINT
memcpy(&dst->external_metadata_json, // NOLINT
&tmp_meta,
sizeof(struct String));
memcpy(&dst->access_key_id,
&tmp_access_key,
sizeof(struct String)); // NOLINT
memcpy(&dst->secret_access_key,
&tmp_secret_key,
sizeof(struct String)); // NOLINT
}

// 2. Reallocate and copy the Strings
CHECK(copy_string(&dst->filename, &src->filename));
CHECK(copy_string(&dst->uri, &src->uri));
CHECK(
copy_string(&dst->external_metadata_json, &src->external_metadata_json));
CHECK(copy_string(&dst->access_key_id, &src->access_key_id));
CHECK(copy_string(&dst->secret_access_key, &src->secret_access_key));

// 3. Copy the dimensions
if (src->acquisition_dimensions.data) {
Expand All @@ -309,8 +344,10 @@ storage_properties_copy(struct StorageProperties* dst,
void
storage_properties_destroy(struct StorageProperties* self)
{
struct String* const strings[] = { &self->filename,
&self->external_metadata_json };
struct String* const strings[] = { &self->uri,
&self->external_metadata_json,
&self->access_key_id,
&self->secret_access_key };
for (int i = 0; i < countof(strings); ++i) {
if (strings[i]->is_ref == 0 && strings[i]->str) {
free(strings[i]->str);
Expand Down Expand Up @@ -348,9 +385,9 @@ unit_test__storage__storage_property_string_check()
sizeof(metadata),
pixel_scale_um,
0));
CHECK(props.filename.str[props.filename.nbytes - 1] == '\0');
ASSERT_EQ(int, "%d", props.filename.nbytes, sizeof(filename));
ASSERT_EQ(int, "%d", props.filename.is_ref, 0);
CHECK(props.uri.str[props.uri.nbytes - 1] == '\0');
ASSERT_EQ(int, "%d", props.uri.nbytes, sizeof(filename));
ASSERT_EQ(int, "%d", props.uri.is_ref, 0);

CHECK(props.external_metadata_json
.str[props.external_metadata_json.nbytes - 1] == '\0');
Expand All @@ -375,9 +412,9 @@ unit_test__storage__storage_property_string_check()
sizeof(metadata),
pixel_scale_um,
0));
CHECK(src.filename.str[src.filename.nbytes - 1] == '\0');
CHECK(src.filename.nbytes == sizeof(filename));
CHECK(src.filename.is_ref == 0);
CHECK(src.uri.str[src.uri.nbytes - 1] == '\0');
CHECK(src.uri.nbytes == sizeof(filename));
CHECK(src.uri.is_ref == 0);
CHECK(src.pixel_scale_um.x == 1);
CHECK(src.pixel_scale_um.y == 2);

Expand All @@ -388,9 +425,9 @@ unit_test__storage__storage_property_string_check()

CHECK(storage_properties_copy(&props, &src));
storage_properties_destroy(&src);
CHECK(props.filename.str[props.filename.nbytes - 1] == '\0');
CHECK(props.filename.nbytes == sizeof(filename));
CHECK(props.filename.is_ref == 0);
CHECK(props.uri.str[props.uri.nbytes - 1] == '\0');
CHECK(props.uri.nbytes == sizeof(filename));
CHECK(props.uri.is_ref == 0);

CHECK(props.external_metadata_json
.str[props.external_metadata_json.nbytes - 1] == '\0');
Expand Down Expand Up @@ -505,6 +542,33 @@ unit_test__storage__copy_string()
return 0;
}

int
unit_test__storage_properties_set_access_key_and_secret()
{
struct StorageProperties props = { 0 };
const char access_key_id[] = "access_key_id";
const char secret_access_key[] = "secret_access_key";

CHECK(
storage_properties_set_access_key_and_secret(&props,
access_key_id,
sizeof(access_key_id),
secret_access_key,
sizeof(secret_access_key)));

CHECK(0 == strcmp(props.access_key_id.str, access_key_id));
CHECK(props.access_key_id.nbytes == sizeof(access_key_id));
CHECK(0 == props.access_key_id.is_ref);

CHECK(0 == strcmp(props.secret_access_key.str, secret_access_key));
CHECK(props.secret_access_key.nbytes == sizeof(secret_access_key));
CHECK(0 == props.secret_access_key.is_ref);

return 1;
Error:
return 0;
}

int
unit_test__dimension_init()
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,11 @@ extern "C"
/// Properties for a storage driver.
struct StorageProperties
{
struct String filename;
struct String uri;
struct String external_metadata_json;
struct String access_key_id;
struct String secret_access_key;

uint32_t first_frame_id;
struct PixelScale pixel_scale_um;

Expand All @@ -68,6 +71,7 @@ extern "C"
uint8_t chunking_is_supported;
uint8_t sharding_is_supported;
uint8_t multiscale_is_supported;
uint8_t s3_is_supported;
};

/// Initializes StorageProperties, allocating string storage on the heap
Expand All @@ -76,10 +80,10 @@ extern "C"
/// @param[out] out The constructed StorageProperties object.
/// @param[in] first_frame_id (unused; aiming for future file rollover
/// support
/// @param[in] filename A c-style null-terminated string. The file to create
/// for streaming.
/// @param[in] bytes_of_filename Number of bytes in the `filename` buffer
/// including the terminating null.
/// @param[in] uri A c-style null-terminated string. The file to create
/// for streaming.
/// @param[in] bytes_of_uri Number of bytes in the `uri` buffer,
/// including the terminating null.
/// @param[in] metadata A c-style null-terminated string. Metadata string
/// to save along side the created file.
/// @param[in] bytes_of_metadata Number of bytes in the `metadata` buffer
Expand All @@ -90,8 +94,8 @@ extern "C"
/// to zero.
int storage_properties_init(struct StorageProperties* out,
uint32_t first_frame_id,
const char* filename,
size_t bytes_of_filename,
const char* uri,
size_t bytes_of_uri,
const char* metadata,
size_t bytes_of_metadata,
struct PixelScale pixel_scale_um,
Expand All @@ -105,28 +109,47 @@ extern "C"
int storage_properties_copy(struct StorageProperties* dst,
const struct StorageProperties* src);

/// @brief Set the filename string in `out`.
/// @brief Set the uri string in `out`.
/// Copies the string into storage owned by the properties struct.
/// @returns 1 on success, otherwise 0
/// @param[in,out] out The storage properties to change.
/// @param[in] filename pointer to the beginning of the filename buffer.
/// @param[in] bytes_of_filename the number of bytes in the filename buffer.
/// Should include the terminating NULL.
int storage_properties_set_filename(struct StorageProperties* out,
const char* filename,
size_t bytes_of_filename);
/// @param[in] uri Pointer to the beginning of the uri buffer.
/// @param[in] bytes_of_uri The number of bytes in the uri buffer.
/// Should include the terminating NULL.
int storage_properties_set_uri(struct StorageProperties* out,
const char* uri,
size_t bytes_of_uri);

/// @brief Set the metadata string in `out`.
/// Copies the string into storage owned by the properties struct.
/// @returns 1 on success, otherwise 0
/// @param[in,out] out The storage properties to change.
/// @param[in] metadata pointer to the beginning of the metadata buffer.
/// @param[in] bytes_of_filename the number of bytes in the metadata buffer.
/// @param[in] bytes_of_metadata the number of bytes in the metadata buffer.
/// Should include the terminating NULL.
int storage_properties_set_external_metadata(struct StorageProperties* out,
const char* metadata,
size_t bytes_of_metadata);

/// @brief Set the access key id string in `out`.
/// Copies the string into storage owned by the properties struct.
/// @returns 1 on success, otherwise 0
/// @param[in,out] out The storage properties to change.
/// @param[in] access_key_id Pointer to the beginning of the access key id
/// buffer.
/// @param[in] bytes_of_access_key_id The number of bytes in the access key
/// id.
/// @param[in] secret_access_key Pointer to the beginning of the secret
/// access key buffer.
/// @param[in] bytes_of_secret_access_key The number of bytes in the secret
/// access key.
int storage_properties_set_access_key_and_secret(
struct StorageProperties* out,
const char* access_key_id,
size_t bytes_of_access_key_id,
const char* secret_access_key,
size_t bytes_of_secret_access_key);

/// @brief Set the value of the StorageDimension struct at index `index` in
/// `out`.
/// @param[out] out The StorageProperties struct containing the
Expand Down
2 changes: 2 additions & 0 deletions acquire-core-libs/tests/unit-tests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ extern "C"
// device-properties
int unit_test__storage__storage_property_string_check();
int unit_test__storage__copy_string();
int unit_test__storage_properties_set_access_key_and_secret();
int unit_test__dimension_init();
int unit_test__storage_properties_dimensions_init();
int unit_test__storage_properties_dimensions_destroy();
Expand Down Expand Up @@ -98,6 +99,7 @@ main()
CASE(unit_test__monotonic_clock_increases_monotonically),
CASE(unit_test__storage__storage_property_string_check),
CASE(unit_test__storage__copy_string),
CASE(unit_test__storage_properties_set_access_key_and_secret),
CASE(unit_test__dimension_init),
CASE(unit_test__storage_properties_dimensions_init),
CASE(unit_test__storage_properties_dimensions_destroy),
Expand Down
21 changes: 16 additions & 5 deletions acquire-driver-common/src/storage/raw.c
Original file line number Diff line number Diff line change
Expand Up @@ -35,15 +35,27 @@ static enum DeviceState
raw_set(struct Storage* self_, const struct StorageProperties* properties)
{
struct Raw* self = containerof(self_, struct Raw, writer);
const char* filename = properties->filename.str;
const size_t nbytes = properties->filename.nbytes;
CHECK(properties->uri.str);
CHECK(properties->uri.nbytes);

const size_t offset = strlen(properties->uri.str) >= 7 &&
strncmp(properties->uri.str, "file://", 7) == 0
? 7
: 0;
const char* filename = properties->uri.str + offset;
const size_t nbytes = properties->uri.nbytes - offset;

// Validate
CHECK(file_is_writable(filename, nbytes));

// copy in the properties
CHECK(storage_properties_copy(&self->properties, properties));

// update the URI if it has a file:// prefix
if (offset) {
storage_properties_set_uri(&self->properties, filename, nbytes);
}

return DeviceState_Armed;
Error:
return DeviceState_AwaitingConfiguration;
Expand All @@ -69,9 +81,8 @@ static enum DeviceState
raw_start(struct Storage* self_)
{
struct Raw* self = containerof(self_, struct Raw, writer);
CHECK(file_create(&self->file,
self->properties.filename.str,
self->properties.filename.nbytes));
CHECK(file_create(
&self->file, self->properties.uri.str, self->properties.uri.nbytes));
LOG("RAW: Frame header size %d bytes", (int)sizeof(struct VideoFrame));
return DeviceState_Running;
Error:
Expand Down
Loading

0 comments on commit 85a5d90

Please sign in to comment.