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 command brctl #2971

Open
wants to merge 25 commits into
base: main
Choose a base branch
from
Open

Add command brctl #2971

wants to merge 25 commits into from

Conversation

leongross
Copy link
Contributor

@leongross leongross commented Apr 25, 2024

Add cmd/brctl which aims to be a (near) feature complete implementation of brctl.

TODOs

  • Enhance test coverage
    • add vmtest
  • minifiy str_to_jiffies
  • getBridge interface parsing
  • Setgcint command
  • get interfaces from bridges
  • improve documentation

@10000TB 10000TB self-assigned this Apr 25, 2024
@10000TB 10000TB requested a review from a team April 25, 2024 16:48
@rminnich
Copy link
Member

nice! This is going to be very useful.

@rminnich rminnich added the Awaiting author Waiting for new changes or feedback for author. label Apr 25, 2024
cmds/core/brctl/brctl.go Outdated Show resolved Hide resolved
@leongross leongross force-pushed the cmds/brctl branch 7 times, most recently from 4a4d9bf to b30fce5 Compare April 26, 2024 16:17
cmds/core/brctl/brctl.go Outdated Show resolved Hide resolved
qemu.ArbitraryArgs("-nic", fmt.Sprintf("user,id=%s", BRCTL_TEST_IFACE_0)),
qemu.ArbitraryArgs("-nic", fmt.Sprintf("user,id=%s", BRCTL_TEST_IFACE_1)),
)),
)
Copy link
Member

Choose a reason for hiding this comment

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

Do we have steps to validate traffic are transferred properly ?

@10000TB
Copy link
Member

10000TB commented Apr 30, 2024

Edit: NVM -- discussed offline, I mis-understood the github UI. I was told this PR was opened from a fork of u-root.


@leongross Hi Leon, looks like this Draft is pushed against upstream u-root

leongross wants to merge 13 commits into u-root:main from leongross:cmds/brctl

Could you push against a fork of u-root, and make this as a PR ?

This looks like review-worthy.

@leongross
Copy link
Contributor Author

leongross commented Apr 30, 2024

@10000TB I'm not quite sure what kind of review process you suggest.
My PR goes from my private fork to the upstream u-root. What benefit is there to create a PR for another fork or even my own? There still needs to be a PR for upstream u-root after all, so why create another layer?
As I see it the procedure of this PR adheres to CONTRIBUTING.md guide. Maybe I misunderstood your suggestion, so I'd be glad if you could clarify.

@leongross leongross marked this pull request as ready for review April 30, 2024 09:41
@10000TB
Copy link
Member

10000TB commented May 2, 2024

@10000TB I'm not quite sure what kind of review process you suggest. My PR goes from my private fork to the upstream u-root. What benefit is there to create a PR for another fork or even my own? There still needs to be a PR for upstream u-root after all, so why create another layer? As I see it the procedure of this PR adheres to CONTRIBUTING.md guide. Maybe I misunderstood your suggestion, so I'd be glad if you could clarify.

you good. I mis-understood the github UI ;)

@leongross leongross force-pushed the cmds/brctl branch 4 times, most recently from 2a9acf3 to 032a98a Compare May 3, 2024 09:44
@leongross leongross force-pushed the cmds/brctl branch 4 times, most recently from 3d8137e to 3772a48 Compare May 15, 2024 18:43
leongross and others added 17 commits June 12, 2024 07:54
Signed-off-by: leongross <leon.gross@9elements.com>
Signed-off-by: leongross <leon.gross@9elements.com>
Signed-off-by: leongross <leon.gross@9elements.com>
Signed-off-by: leongross <leon.gross@9elements.com>
…lCase

Signed-off-by: leongross <leon.gross@9elements.com>
Signed-off-by: leongross <leon.gross@9elements.com>
Signed-off-by: leongross <leon.gross@9elements.com>
Signed-off-by: leongross <leon.gross@9elements.com>
Signed-off-by: leongross <leon.gross@9elements.com>
Signed-off-by: leongross <leon.gross@9elements.com>
Signed-off-by: leongross <leon.gross@9elements.com>
conversion

Signed-off-by: leongross <leon.gross@9elements.com>
Signed-off-by: Christopher Meis <christopher.meis@9elements.com>
Signed-off-by: leongross <leon.gross@9elements.com>
Signed-off-by: leongross <leon.gross@9elements.com>
Signed-off-by: leongross <leon.gross@9elements.com>
Signed-off-by: leongross <leon.gross@9elements.com>
Signed-off-by: Christopher Meis <christopher.meis@9elements.com>
@ChriMarMe ChriMarMe force-pushed the cmds/brctl branch 6 times, most recently from 7a4b249 to 67d8c32 Compare June 12, 2024 07:20
Signed-off-by: Christopher Meis <christopher.meis@9elements.com>
@ChriMarMe
Copy link
Contributor

Well, tests run or being skipped if eth0/eth1 are not present in setup. Dunno how to fix the coverage complains.

@10000TB
Copy link
Member

10000TB commented Jun 12, 2024

Well, tests run or being skipped if eth0/eth1 are not present in setup. Dunno how to fix the coverage complains.

If you look at each file in this PR, there are alerts from "Codecov / codecov/patch" at each specific line telling you relevant lines are missing tests.

The codecov seems to suggest this PR brings down coverage by -38%. That is not acceptable :(

codecov/patch Failing after 1s — 34.50% of diff hit (target 57.32%)
[Details](https://github.com/u-root/u-root/pull/2971/checks?check_run_id=26115385569)
@codecov
codecov/patch/everything Failing after 1s — 34.50% of diff hit (target 56.45%)
[Details](https://github.com/u-root/u-root/pull/2971/checks?check_run_id=26115386655)
@codecov
codecov/project Failing after 1s — 56.85% (-0.47%) compared to 65d9fc3
[Details](https://github.com/u-root/u-root/pull/2971/checks?check_run_id=26115416946)
@codecov
codecov/project/cmds/exp — 30.54% (-38.96%) compared to 65d9fc3

@10000TB 10000TB added this to the M5: Coreutils milestone Jun 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting author Waiting for new changes or feedback for author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants