Conversation
- I used claude code, sonnet 4.6 to perform a code review of liboni
covering
- oni.h / oni.c
- onidefs.h
- onidriver.h
- onidriverloader.h / onidriverloader.c
- The following issues were revealed and corrected
1. Both `ONI_OPT_BLOCKREADSIZE` and `ONI_OPT_BLOCKWRITESIZE` cases
were missing a return after the state guard
2. Several cases where possible `NULL` value return of malloc was not checked
3. `dlerror()` was called twice in `onidriverloader.c`.
- dlerror() resets on each call. The second call always returns NULL and prints nothing.
4. `oni_destroy_driver` was not being used in `oni_destory_ctx` which leaked the library handle.
5. return value of `_oni_reset_routine` during `ONI_OPT_RESET` case was not used.
6. The padding calculation for read and write buffer allocations was wrong
- We were adding the remainder of `ctx->max_read_frame_size % sizeof(oni_fifo_dat_t)`
instead of the complement of the remainder.
7. `oni_write_frame` did not check for `NULL` `ctx` or run state
8. Remove trailing semicolon from the `BYTE_TO_FIFO_SHIFT` macro
9. `_ref_inc`/`_ref_dec` took `const struct ref *` but mutated `&ref->count`, so `const`
needed to be casted away.
10. `ONI_FRAMEHEADERSZ` was incorrectly and redundantly defined in `onidefs.h`
11. `oni_frame_t.data` was `char *` but `char` signedness is implementation-defined.
It was changed to `uint8_t *`
- When data rates are low, `oni_read_frame` can take some time to fill a buffer and return - `WaitForSingleObject` was supplied with 500 ms timeout which would race with `oni_read_frame` resulting in a failure to read from stream error if `oni_read_frame` lost the race.
| const oni_fifo_dat_t dev_idx; // Device index that produced or accepts the frame | ||
| const oni_fifo_dat_t data_sz; // Size in bytes of data buffer | ||
| char *data; // Raw data block | ||
| uint8_t *data; // Raw data block |
There was a problem hiding this comment.
This change is causing some warnings (which on our linux build are treated as errors) because now we are mixing this uint8_t* with calls and buffers declared as char* (so there is a sign mismatch).
Specifically, the issues are in
Line 757 in b799fc9
Line 792 in b799fc9
function signatures and allocated buffers should be all changed to match this new type
There was a problem hiding this comment.
or we could revert back to char since this is how its been done for centuries anyway. I'm fine with this.
| ctx->block_write_size = ctx->block_write_size < 4096 ? 4096 : ctx->block_write_size; | ||
| size_t align = sizeof(oni_fifo_dat_t); | ||
| size_t r = ctx->max_read_frame_size % align; | ||
| ctx->block_read_size = ctx->max_read_frame_size + (r ? align - r : 0); |
There was a problem hiding this comment.
In my ONIv2 implementation I'm using
ctx->block_read_size = (ctx->max_read_frame_size + align - 1) & ~(align - 1);which basically does the same for power of 2 sizes in a very fast way
here is not critical but I'll be using it on some other more critical places, so it could be a good moment to standardize
There was a problem hiding this comment.
sounds good. i can change that.
2 participants
I used claude code, sonnet 4.6 to perform a code review of liboni
covering
The following issues were revealed and corrected
ONI_OPT_BLOCKREADSIZEandONI_OPT_BLOCKWRITESIZEcaseswere missing a return after the state guard
NULLvalue return of malloc was not checkeddlerror()was called twice inonidriverloader.c.oni_destroy_driverwas not being used inoni_destory_ctxwhich leaked the library handle._oni_reset_routineduringONI_OPT_RESETcase was not used.ctx->max_read_frame_size % sizeof(oni_fifo_dat_t)instead of the complement of the remainder.
oni_write_framedid not check forNULLctxor run stateBYTE_TO_FIFO_SHIFTmacro_ref_inc/_ref_dectookconst struct ref *but mutated&ref->count, soconstneeded to be casted away.
ONI_FRAMEHEADERSZwas incorrectly and redundantly defined inonidefs.honi_frame_t.datawaschar *butcharsignedness is implementation-defined.It was changed to
uint8_t *