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

rewrite zenoh-c API for errno support and reduce stack size #313

Closed
milyin opened this issue Apr 4, 2024 · 2 comments
Closed

rewrite zenoh-c API for errno support and reduce stack size #313

milyin opened this issue Apr 4, 2024 · 2 comments
Labels
release Part of the next release

Comments

@milyin
Copy link
Contributor

milyin commented Apr 4, 2024

Summary of discussion with @yellowhatter , @kydos , @Mallets , @DenisBiryukov91 , @milyin about C API

Problem:
Existing C API follows this pattern:

z_owned_foo_t foo = z_foo_new();
if (!z_check(foo)) {
  // report failure
}
z_process_foo(z_loan(foo));
z_consume_foo(z_move(foo));

This pattern have several problems:

  1. z_owned_foo_t object is created on stack. If developer wants to have it in heap or static memory, the additional copy is performed. This can be bad for performance on hot path and bad for stack size in restrained environments (embedded)
  2. There is no error value returned. It's impossible to know the reason of the failure
  3. we are obliged to do additional call z_check (sometimes it's inlined, but not always). If this is real function call, this may add bad impact to performance
  4. internally both calls to z_process_foo and z_consume_foo do the check for validity of foo, as on rust size the z_owned_foo_t implemented as Option<Foo>. This also may impact the performance

Proposed change: follow this pattern

z_owned_foo_t foo;
int err = z_foo_new(&foo);
if (err < 0) {
  // report failure "err"
}
...

This solves issues 1,2,3. Regarding issue 4 - the functions whcih are expected to be executed frequently (processing of samples, buffers, etc) should use https://docs.rs/unsafe_unwrap/latest/unsafe_unwrap/ instead of normal one, with panic.

@milyin milyin added the release Part of the next release label Apr 4, 2024
@doganulus
Copy link

I have recently switched to zenohc after Python experiments and I agree the current error handling mechanism in zenohc is not very familiar to C/C++ users. Error codes may provide a more familiar error handling besides other problems mentioned above.

Also typedef'ed enums can be more descriptive for error codes compared to plain integers as follows.

#include<errno.h>

typedef enum z_errc {
  no_error = 0,
  no_such_file_or_directory = ENOENT,
  not_a_directory = ENOTDIR,
  timed_out = ETIMEDOUT,
  ...
} z_error_code_t; 

@DenisBiryukov91 DenisBiryukov91 mentioned this issue Apr 29, 2024
5 tasks
@milyin
Copy link
Contributor Author

milyin commented May 29, 2024

done in dev/1.0.0. Relaltes also to #400

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release Part of the next release
Projects
Status: Done
Development

No branches or pull requests

2 participants