ystero-dev / scalpel

Packet Dissection and sculpting in Rust
Other
3 stars 6 forks source link

Adding `wasm` as a feature during crate compilation #66

Closed gabhijit closed 5 months ago

gabhijit commented 5 months ago

Making several conditional compilation changes for wasm feature -

Also made the pyo3 dependency not applicable for wasm targets.

gabhijit commented 5 months ago

@csking101 : This is a PR that would use wasm as a feature (mutually exclusive with python-bindings for now).

I still haven't solved the examples issue, will update that. Take a look, let me know if makes sense.

csking101 commented 5 months ago

Yes sir, got it! How do you recommend we move forth with this? I think we can plan which APIs will be supported by wasm or is dissect_packet enough for now?

gabhijit commented 5 months ago

Yes sir, got it! How do you recommend we move forth with this? I think we can plan which APIs will be supported by wasm or is dissect_packet enough for now?

@csking101 : Not sure? Don't you think the current mechanism to get started is a bit involved? As far as POC API is concerned dissect_packet as an API might be good enough, but I am just thinking is there a way to integrate that API into CI/CD? Also some example using deno ~crate~? so that we can programmatically include it? Then of course the current documentation is also there.

I am still not entirely convinced the changes in this PR are good enough to be merged (but I don't have a much better answer as yet). So let's see how it goes.

Makes sense?

gabhijit commented 5 months ago

@csking101 : I believe the current implementation now has basic support. I have taken from your draft PR the API, added the feature called wasm and added a few tests along with enabling some unit tests for wasm.

Take a look, I believe this is good enough to be merged.

This would mean initial wasm support is in place.

gabhijit commented 5 months ago

Take a look, I believe this is good enough to be merged.

I am going to take another look at all that conditional compilation spaghetti :) . Will see if that can be cleaned up a bit.

csking101 commented 5 months ago

Sorry for not looking the earlier messages sir.

The current commits look good to me!

Would you like us to give additional test cases for the same?

Also sir, I think we would have to discuss the support for different ENCAP_TYPES, in the dissect_packet function, I believe another parameter can be added in the function (for use in the JS) to determine which type it is, but do you think hardcoding and mapping the types is a good idea?

gabhijit commented 5 months ago

Would you like us to give additional test cases for the same?

For now the main test cases in src/packet.rs are good I believe. Having said so - more test cases are always welcome.

Also sir, I think we would have to discuss the support for different ENCAP_TYPES, in the dissect_packet function, I believe another parameter can be added in the function (for use in the JS) to determine which type it is, but do you think hardcoding and mapping the types is a good idea?

Yes, I agree probably a good idea to not hard-code this. Here is what I suggest, there are a couple of items that need to be done with respect to the API.

  1. The API should return a Result and accept encap_type. ( Returning Result. We'll need to explore a bit how the Result can be returned in the wasm module and how the caller would use it)
  2. We should also have the console_error_panic_handler enabled if wasm is being compiled. So that panics can be logged using console.error
  3. The ENCAP_TYPE_* constants should be exposed from the wasm modules. (We will need to explore this a bit as well).
  4. See if the current feature soup can be simplified.

What we can do is - this is a basic working support with CI integration, I will merge this and then we can make the improvements above in a separate PR.

@csking101 : Would you like to take up 1, 2, and 3 above? I will take up 4 (but may be a bit later).

csking101 commented 5 months ago

Sure sir, that would be great. I guess now that we have the basic working support, the enhancements can be made in a separate PR. I will look into 1,2 and 3.