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

modify dissect_packet function #69

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

JnanaN
Copy link
Contributor

@JnanaN JnanaN commented Apr 22, 2024

This PR adds following features

  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
    as mentioned in Adding wasm as a feature during crate compilation #66 under Adding wasm as a feature during crate compilation #66 (comment)

Signed-off-by: JnanaN <jnanabhushan943@gmail.com>
Copy link
Collaborator

@gabhijit gabhijit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This mostly looks okay, here are a few suggestions using -

  1. Conditional compilation for wasm feature.
  2. Supporting new API that can directly use binary data
  3. Test cases for different encap types.

@@ -19,6 +19,10 @@ serde = { version = "1.0", features = ["derive"] }
serde_json = { version = "1.0" }
erased-serde = "0.4"
log = { version = "0.4", optional = true }
console_error_panic_hook = "0.1.6"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This and next three two dependencies should be added only when feature wasm is used.

#[wasm_bindgen_test]
fn simple_dissect_test() {
let bytestream = "003096e6fc3900309605283888470001ddff45c0002800030000ff06a4e80a0102010a2200012af90017983210058dd58ea55010102099cd00000000";
scalpel::dissect_packet(bytestream.to_string());
let encap_type = scalpel::ENCAP_TYPE_ETH;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This and other ENCAP_TYPE_* constants should be exported to the JS world. Not sure whether this is possible. I

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's indeed not possible to export constants we can ignore this one. I tried around a bit, but couldn't find an easy way to do it.

scalpel::dissect_packet(bytestream.to_string());
let encap_type = scalpel::ENCAP_TYPE_ETH;

match scalpel::dissect_packet(encap_type, bytestream.to_string()) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also add another API something like dissect_packet_bin when the data is available as actual binary data. While using this, we realized there's an unnecessary round trip to hex string and back. So see if that can be avoided by defining new API.

@@ -5,11 +5,23 @@
extern crate wasm_bindgen_test;
use wasm_bindgen_test::*;

use web_sys::console::error_1;
use web_sys::console::log_1;
wasm_bindgen_test_configure!(run_in_browser);

#[wasm_bindgen_test]
fn simple_dissect_test() {
let bytestream = "003096e6fc3900309605283888470001ddff45c0002800030000ff06a4e80a0102010a2200012af90017983210058dd58ea55010102099cd00000000";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If possible try to add a test case or two for other encap types as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants