Add an FFI-friendly interface for ZFEC encoding and decoding #3196

exarkun · 2023-01-19T17:13:13Z

Relates to #3190

codecov-commenter · 2023-01-19T21:00:42Z

Codecov Report

Base: 88.09% // Head: 88.10% // Increases project coverage by +0.01% 🎉

Coverage data is based on head (7596a49) compared to base (b469c5d).
Patch coverage: 100.00% of modified lines in pull request are covered.

❗ Current head 7596a49 differs from pull request most recent head f9e20aa. Consider uploading reports for the commit f9e20aa to get more accurate results

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3196      +/-   ##
==========================================
+ Coverage   88.09%   88.10%   +0.01%     
==========================================
  Files         609      610       +1     
  Lines       68300    68342      +42     
  Branches     6811     6818       +7     
==========================================
+ Hits        60166    60214      +48     
+ Misses       5296     5287       -9     
- Partials     2838     2841       +3

Impacted Files	Coverage Δ
src/lib/ffi/ffi_zfec.cpp	`100.00% <100.00%> (ø)`
src/tests/test_ffi.cpp	`96.39% <100.00%> (+0.06%)`	⬆️
src/fuzzer/mode_padding.cpp	`91.07% <0.00%> (-5.42%)`	⬇️
src/lib/utils/mem_ops.cpp	`71.42% <0.00%> (-2.26%)`	⬇️
src/cli/speed.cpp	`90.32% <0.00%> (-0.11%)`	⬇️
src/tests/test_bigint.cpp	`90.81% <0.00%> (-0.03%)`	⬇️
src/lib/misc/zfec/zfec.cpp	`83.90% <0.00%> (+0.48%)`	⬆️
src/lib/tls/tls12/tls_channel_impl_12.cpp	`81.07% <0.00%> (+0.63%)`	⬆️
src/lib/asn1/asn1_time.cpp	`86.95% <0.00%> (+0.72%)`	⬆️
src/lib/pubkey/elgamal/elgamal.cpp	`86.30% <0.00%> (+1.36%)`	⬆️
... and 3 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

randombit

Thanks @exarkun! This already looks very good.

I think the big remaining issue here is to also amend the Python wrapper in src/python/botan2.py with the appropriate ctypes declarations, plus a reasonably Pythonic wrapper and a simple test (along the lines of what you wrote in C++ already) that it works.

randombit · 2023-01-20T13:05:21Z

src/lib/ffi/ffi_zfec.cpp

+*/
+
+#include <botan/ffi.h>
+#include <botan/zfec.h>


The include and the functions themselves should be guarded by BOTAN_HAS_ZFEC, so that it is possible to compile the C layer even if ZFEC is disabled from the build.

The functions themselves should still be defined mind you! And just return BOTAN_FFI_ERROR_NOT_IMPLEMENTED in that case.

exarkun · 2023-02-06T21:41:07Z

@randombit I think this is ready for another look. Thanks!

randombit · 2023-02-07T14:30:51Z

@exarkun Great! I'll review asap. One immediate comment: please rebase and remove the merge commit.

randombit · 2023-02-16T14:55:09Z

@exarkun Sorry I still need to review this! Don't worry though this is definitely going to land for 3.0. I notice there is a merge conflict, can you rebase and resolve? Thanks

…d it here

…rect block indexes

The share sizes are always easily predictable. They are totalSize / K. All share sizes are the same. There's no need to pass them out to the caller. Also rename one `size` parameter to `shareSize` so it is not so easily confused with the total input size parameter.

This avoids uncomfortable questions about the choice of allocator in use on either side of an FFI boundary.

Return a not implemented error code if it is unavailable.

paired with Jean-Paul

randombit

This looks very good and basically ready to merge. There are couple of very small issues, basically just the unnecessary checks on the return value in the Python wrapper, plus needing to change the references to the module in the Python tests.

src/python/botan3.py

src/scripts/test_python.py

The ffi object already has an `errcheck` which will notice error return codes and raise an exception.

exarkun · 2023-02-22T19:55:30Z

Thanks! I believe I have addressed those points.

randombit · 2023-02-22T20:08:09Z

@exarkun Looks good! Just waiting on CI now

exarkun mentioned this pull request Jan 19, 2023

Expose ZFEC functionality via the C-compatible FFI interface #3190

Closed

randombit reviewed Jan 20, 2023

View reviewed changes

exarkun marked this pull request as draft January 23, 2023 16:36

randombit added this to the Botan 3.0.0 milestone Jan 31, 2023

exarkun marked this pull request as ready for review February 6, 2023 21:40

randombit self-requested a review February 6, 2023 22:43

exarkun force-pushed the 3190.ffi-zfec branch 2 times, most recently from 2a5637f to f9e20aa Compare February 7, 2023 14:57

exarkun and others added 17 commits February 17, 2023 08:44

C ABI for ZFEC

5dc2740

expose the API in public

08959a2

Actually pass the out pointers out

94b7227

Start working on a test suite

4f06430

Add decode and make the test pass

c1826ce

There's Z-Botan, too

8732c47

Conditional is wrapped around the test implementation so we don't nee…

2d6c489

…d it here

Deallocate decodedSizes correctly

9a639ee

More thoroughly test the decoder and fix a bug related to using incor…

43eac55

…rect block indexes

Fix the copyright header

180d952

Replace some manual new/delete with std::vector

e69190e

Make the caller responsible for *all* memory management.

f2ff0ec

This avoids uncomfortable questions about the choice of allocator in use on either side of an FFI boundary.

Make the ZFEC C API conditional on BOTAN_HAS_ZFEC.

2c8a856

Return a not implemented error code if it is unavailable.

Add Python bindings and rudimentary tests

e082a4a

paired with Jean-Paul

improve tests

80e6a4f

Attempt to get whitespace consistent with local conventions

c26b49e

exarkun force-pushed the 3190.ffi-zfec branch from f9e20aa to c26b49e Compare February 17, 2023 13:45

randombit reviewed Feb 22, 2023

View reviewed changes

src/python/botan3.py Outdated Show resolved Hide resolved

src/python/botan3.py Outdated Show resolved Hide resolved

src/scripts/test_python.py Outdated Show resolved Hide resolved

exarkun added 2 commits February 22, 2023 14:42

Remove redundant return code checks from the Python ZFEC bindings

d251adf

The ffi object already has an `errcheck` which will notice error return codes and raise an exception.

Fix the Python module name used by tests to reflect recent naming

7136dd7

randombit merged commit ffca2ad into randombit:master Feb 22, 2023

meejah deleted the 3190.ffi-zfec branch February 22, 2023 21:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add an FFI-friendly interface for ZFEC encoding and decoding #3196

Add an FFI-friendly interface for ZFEC encoding and decoding #3196

exarkun commented Jan 19, 2023

codecov-commenter commented Jan 19, 2023 •

edited

Loading

randombit left a comment

randombit Jan 20, 2023

exarkun commented Feb 6, 2023

randombit commented Feb 7, 2023

randombit commented Feb 16, 2023

randombit left a comment

exarkun commented Feb 22, 2023

randombit commented Feb 22, 2023

Add an FFI-friendly interface for ZFEC encoding and decoding #3196

Add an FFI-friendly interface for ZFEC encoding and decoding #3196

Conversation

exarkun commented Jan 19, 2023

codecov-commenter commented Jan 19, 2023 • edited Loading

Codecov Report

randombit left a comment

Choose a reason for hiding this comment

randombit Jan 20, 2023

Choose a reason for hiding this comment

exarkun commented Feb 6, 2023

randombit commented Feb 7, 2023

randombit commented Feb 16, 2023

randombit left a comment

Choose a reason for hiding this comment

exarkun commented Feb 22, 2023

randombit commented Feb 22, 2023

codecov-commenter commented Jan 19, 2023 •

edited

Loading