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

Move MetaAddr serialization into zebra_network::protocol::external #3020

Merged
merged 3 commits into from
Nov 7, 2021

Conversation

teor2345
Copy link
Contributor

@teor2345 teor2345 commented Nov 5, 2021

Motivation

To prepare for addrv2 serialization, we need to deserialize two different formats into the MetaAddr data structure.

Zebra expects each data structure to have a single format. So it doesn't provide a format argument to its deserialization functions. So I want to create two different types for deserialization: AddrV1 and AddrV2.

Solution

This is the first part of that change, which moves the MetaAddr serialization code to the zebra_network::protocol::external module.

I split the code movement from the type changes, to make them both easier to review.

Review

@jvff can review this change.

It's not urgent, but there will be a series of PRs, so it would be great to get it done by Monday.

Reviewer Checklist

  • It's just code movement
  • Existing tests pass

Follow Up Work

@teor2345 teor2345 added A-rust Area: Updates to Rust code C-cleanup Category: This is a cleanup P-Medium labels Nov 5, 2021
@teor2345 teor2345 requested a review from jvff November 5, 2021 00:21
@teor2345 teor2345 self-assigned this Nov 5, 2021
@teor2345 teor2345 added C-enhancement Category: This is an improvement NU-5 Network Upgrade: NU5 specific tasks and removed C-cleanup Category: This is a cleanup labels Nov 5, 2021
@teor2345 teor2345 enabled auto-merge (squash) November 5, 2021 05:07
Copy link
Contributor

@jvff jvff left a comment

Choose a reason for hiding this comment

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

Looks good! I saw one opportunity for improvement, but it's not directly part of the PR as the PR is just moving the code, so feel free to ignore it

Comment on lines +96 to +99
let mut smallest_disallowed_vec = Vec::with_capacity(max_allocation + 1);
for _ in 0..(MetaAddr::max_allocation() + 1) {
smallest_disallowed_vec.push(addr);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Optional suggestion:

Suggested change
let mut smallest_disallowed_vec = Vec::with_capacity(max_allocation + 1);
for _ in 0..(MetaAddr::max_allocation() + 1) {
smallest_disallowed_vec.push(addr);
}
let mut smallest_disallowed_vec = vec![addr; max_allocation + 1];

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I'll fix this on top of PR #3022, because it can be fixed in all the preallocate tests.
(And I need to write new preallocate tests in #3022.)

@teor2345 teor2345 merged commit 88cdf0f into main Nov 7, 2021
@teor2345 teor2345 deleted the meta-addr-codec-move branch November 7, 2021 21:37
@teor2345 teor2345 mentioned this pull request Nov 7, 2021
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rust Area: Updates to Rust code C-enhancement Category: This is an improvement NU-5 Network Upgrade: NU5 specific tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants