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

Brctl #866

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

Brctl #866

wants to merge 3 commits into from

Conversation

steelman
Copy link
Contributor

Alternatively addbr could also get completion of available bridges to enable avoiding clashes.

@steelman
Copy link
Contributor Author

Why? With brctl show $1 (w/o quotes) I (rightfully so) got an error massage about possible splitting.

completions/brctl Outdated Show resolved Hide resolved
@@ -1,4 +1,19 @@
# bash completion for brctl -*- shell-script -*-
_brctl_interfaces()
Copy link
Owner

Choose a reason for hiding this comment

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

We have some new naming conventions for functions in effect, please refer to https://github.com/scop/bash-completion/blob/master/doc/api-and-naming.md#naming

Copy link
Contributor Author

@steelman steelman Jan 12, 2023

Choose a reason for hiding this comment

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

So this one's going to be _comp_xfunc_brctl_interfaces? I mean, there is no other consumer now, but makes perfect sense to have a function that returns a list of bridged interfaces. Then, I believe, a check like [[ -x brctl ]] || return needs to be added in case brctl isn't installed.

Copy link
Owner

Choose a reason for hiding this comment

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

Maybe, but then again we can also do that when there is a known consumer for that function. No strong opinions.

The current implementation of the function already redirects stderr to /dev/null, so I don't think a separate existence/executability check is required. We tend to not sprinkle those unless there's a specific reason to.

Also, the brctl to invoke should be passed to the function as a parameter. Random in-tree example:

Comment on lines 6 to 10
if [[ -z "$1" ]]; then
brctl show
else
brctl show "$1"
fi
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if [[ -z "$1" ]]; then
brctl show
else
brctl show "$1"
fi
brctl show ${1:+"$1"}

When adding a network interface to a bridge we most probably want to
use one that hasn't been configured yet.
When completing a delif command offer only interfaces that are part of
the selected bridge.
The show subcommand accepts a bridge name as a paramenter so let's
offer completion.  On the other hand addbr creates new interface and
requires a name that doesn't exist yet.

Longer awk condition prevents offering interfaces that are parts of
bridges. They are all displayed in the last column but with all other
columns being blang on the second and following lines of each bridge
the last column is also the first.
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

3 participants