-
Notifications
You must be signed in to change notification settings - Fork 71
Add default for Seg6Header #227
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
base: main
Are you sure you want to change the base?
Conversation
Summary of ChangesHello @Aperence, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the usability of the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request aims to make Seg6Header constructible from outside the crate by deriving the Default trait for it and its mode field of type Seg6Mode. This is a good approach for #[non_exhaustive] types. However, the chosen default Seg6Mode::Encap for Seg6Mode causes Seg6Header::default() to create an invalid instance that panics during serialization. My review includes a critical comment explaining the issue and suggesting a fix, which is to change the default mode to Seg6Mode::Inline.
src/route/seg6.rs
Outdated
| /// Netlink attributes for `RTA_ENCAP` with `RTA_ENCAP_TYPE` set to | ||
| /// `LWTUNNEL_ENCAP_SEG6`. | ||
| #[derive(Debug, PartialEq, Eq, Clone)] | ||
| #[derive(Debug, PartialEq, Eq, Clone, Default)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deriving Default here is the correct approach to make Seg6Header constructible. However, it currently leads to a panic because of the Default implementation of Seg6Mode.
Seg6Header::default() will be created with mode: Seg6Mode::Encap (the current default) and an empty segments vector. When this default instance is serialized via emit_value(), the number_segments will be 0. This causes an integer underflow and a panic in debug builds on lines 182-183 when calculating segments_left and first_segment (0 - 1).
It appears Encap mode requires at least one segment to be valid. To fix this, Seg6Mode::Inline should be the default, as it handles an empty segment list correctly.
Please move the #[default] attribute from the Encap variant to the Inline variant in the Seg6Mode enum definition.
|
Changed from a default implementation to a constructor returning a Result<Seg6Header, ...>. |
As the structure Seg6Header is tagged with `non_exhaustive`, there is currently no way for an external library to create a Seg6Header structure. By defining a constructor function `new` for Seg6Header, this patch should allow to create Seg6Header instances. In addition, this constructor checks if the segment list is valid, i.e. if at least one segment is present, preventing an underflow exception in `emit_value`. Signed-off-by: Aperence <[email protected]>
Signed-off-by: Aperence <[email protected]>
Derive default for the Seg6Header structure. As the structure is tagged with
non_exhaustive, there is currently no way to create a Seg6Header structure. By defining the Default trait for Seg6Header, this patch should allow to create Seg6Header instances.