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

Add TCP6 Protocol #34

Closed
wants to merge 1 commit into from
Closed

Conversation

Ayush1325
Copy link
Contributor

Signed-off-by: Ayush Singh ayushsingh1325@gmail.com

@Ayush1325
Copy link
Contributor Author

Ayush1325 commented Jul 4, 2022

Is it correct to represent enums as 32-bit constants? I was not sure about that.

Also, I am not sure how to represent the Service Binding Protocol

Signed-off-by: Ayush Singh <ayushsingh1325@gmail.com>
Copy link
Member

@dvdhrm dvdhrm left a comment

Choose a reason for hiding this comment

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

I merged this manually now! I applied some fixed on top. See my inline comments.

Thanks a lot for providing these!

hop_limit: u8,
flow_lable: u32,
receive_timeout: u32,
transmit_timeout: u32,
Copy link
Member

Choose a reason for hiding this comment

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

All the struct-members lack the pub annotation.

prefix_length: u8,
}

pub type NeighboutState = u32;
Copy link
Member

Choose a reason for hiding this comment

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

Neighbout -> Neighbor

Also note that American-English spells it with o instead of ou (as in British English). The spec uses the American style, so I fixed it all over the place.

pub const EFI_NEIGHBOUR_REACHABLE: NeighboutState = 1;
pub const EFI_NEIGHBOUR_STATE: NeighboutState = 2;
pub const EFI_NEIGHBOUR_DELAY: NeighboutState = 3;
pub const EFI_NEIGHBOUR_PROBE: NeighboutState = 4;
Copy link
Member

Choose a reason for hiding this comment

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

The EFI_* prefix can be safely dropped here.

prefix_table: *mut AddressInfo,
icmp_type_count: u32,
icmp_type_list: *mut IcmpType,
}
Copy link
Member

Choose a reason for hiding this comment

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

I added the missing IP6 protocols definitions as well!

pub flush_queues_on_reset: Boolean,
pub enable_receive_timestamps: Boolean,
pub disable_background_polling: Boolean,
}
Copy link
Member

Choose a reason for hiding this comment

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

I completed the protocol with the other bits as well!


pub type ConnectionState = u32;

pub const CONNECTION_CLOSED: ConnectionState = 0;
Copy link
Member

Choose a reason for hiding this comment

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

I dropped the CONNECTION_* prefix, as it is not used in the other values below. I added the STATE_* prefix though, as that follows how we encode C-enums.

pub urgent_flag: Boolean,
pub data_length: u32,
pub fragment_count: u32,
pub fragment_table: *mut FragmentData,
Copy link
Member

Choose a reason for hiding this comment

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

The last part is not a pointer but inline data. This needs to be [FramentData; 0] to follow our conversion-style.

pub urgent: Boolean,
pub data_length: u32,
pub fragment_count: u32,
pub fragment_table: *mut FragmentData,
Copy link
Member

Choose a reason for hiding this comment

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

Same as above.

#[repr(C)]
pub struct IoToken {
pub rx_data: *mut ReceiveData,
pub tx_data: *mut TransmitData,
Copy link
Member

Choose a reason for hiding this comment

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

This is a bit tricky to get right in rust. This needs an extracted union (we cannot nest declarations in rust like the spec does in C; especially anonymous entries are not a stable rust feature).

0x9a,
0x0d,
&[0xd2, 0xe4, 0xcc, 0x16, 0xd6, 0x64],
);
Copy link
Member

Choose a reason for hiding this comment

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

I merged this into tcp6.rs, just like EDK2 does.

@dvdhrm
Copy link
Member

dvdhrm commented Jul 6, 2022

Is it correct to represent enums as 32-bit constants? I was not sure about that.

Also, I am not sure how to represent the Service Binding Protocol

I pushed the ServiceBindingProtocol now. Note that https://docs.rs/r-efi/latest/r_efi/ has a bit of information on the transpose-guidelines we follow. If things are unclear, we usually have precedent somewhere in base.rs or system.rs, or in one of the existing protocols. As it might be rather cumbersome to find it, I don't mind fixing up protocols myself.

If you have questions regarding my fixups, let me know! I usually just try to keep the different protocols in the same style. I also want to avoid deviations from the spec, so people don't have to guess what the names are in r-efi, but can rely on the spec-wording. Lastly, since things are not always trivial, and since the spec has errors on its own, I generally follow what EDK2 / Tianorcore does.

Thanks for the protocols! Very much appreciated! If you want me to push a minor release, let me know!

@dvdhrm dvdhrm closed this Jul 6, 2022
@Ayush1325
Copy link
Contributor Author

Thanks, I will keep things in mind from my next PR

@Ayush1325 Ayush1325 deleted the network_protocols branch July 6, 2022 15:19
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.

None yet

2 participants