Skip to content

fix(available_interfaces): fix regression of unwanted trailing colons #1131

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.

Sign up for GitHub

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

Merged
1 commit merged into from
Mar 10, 2024
Merged

fix(available_interfaces): fix regression of unwanted trailing colons #1131

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion bash_completion
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1737,7 +1737,7 @@ _comp_compgen_available_interfaces()
fi
} 2>/dev/null | _comp_awk \
'/^[^ \t]/ { if ($1 ~ /^[0-9]+:/) { print $2 } else { print $1 } }')" &&
_comp_compgen -U generated set "${generated[@]}"
_comp_compgen -U generated set "${generated[@]%%[[:punct:]]*}"
Copy link
Owner

@scop scop Mar 10, 2024

Choose a reason for hiding this comment

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

On a second thought, this is not correct, it will break display of things like br-something and lo:0 (aliases only come out from ifconfig that way it seems).

Copy link
Collaborator Author

@akinomyoga akinomyoga Mar 10, 2024

Choose a reason for hiding this comment

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

Hmm, thank you for pointing this out. @yedayak I think you can make a proper fix for that. Could you take a look?

Copy link
Collaborator

@yedayak yedayak Mar 10, 2024

Choose a reason for hiding this comment

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

I just checked some stuff, and it seems you can create interfaces with most punctuation in them, for example a_b-c and an interface named veth2@veth3. I didn't manage to create interfaces with : in them, but as @scop mentioned ifconfig can output them like that. I think the best solution will be to remove the last colon always, and maybe to use ip -json when available?

Copy link
Collaborator

@yedayak yedayak Mar 10, 2024

Choose a reason for hiding this comment

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

Although maybe interfaces with @ in them are a edge case we shouldn't care about, in which case we should strip everything from there until the end

Copy link
Collaborator Author

@akinomyoga akinomyoga Mar 12, 2024

Choose a reason for hiding this comment

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

Does that mean you suggest something like "${generated[@]%:}"?

Copy link
Owner

@scop scop Mar 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Re how to create interfaces with colons in them, e.g.

ip addr add 127.0.0.42 dev lo label lo:42
ifconfig lo:43 127.0.0.43 up

These show up in ifconfig output like

lo:42: flags=73<UP,LOOPBACK,RUNNING>  mtu 65536
        inet 127.0.0.42  netmask 255.255.255.255
        loop  txqueuelen 1000  (Local Loopback)

lo:43: flags=73<UP,LOOPBACK,RUNNING>  mtu 65536
        inet 127.0.0.43  netmask 255.0.0.0
        loop  txqueuelen 1000  (Local Loopback)

They do not show up in ip link output for me. They would show up in ip addr output, but not in a way we'd currently parse anyway.

There's also

ip link set lo alias lo:45

...but that does not show up in ifconfig output. It does in ip link output, but again not in a way we'd currently parse.

Copy link
Owner

@scop scop Mar 13, 2024

Choose a reason for hiding this comment

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

I would suggest just stripping the trailing colon as well (did also in #1133).

JSON output is nice in theory, but opens the can of worms that with what would we parse the JSON with? jq, yq, ...? Probably quite a bit of code and even more fallbacks involved, not sure if it's worth it.

Copy link
Collaborator

@yedayak yedayak Mar 13, 2024

Choose a reason for hiding this comment

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

Does that mean you suggest something like "${generated[@]%:}"?

Pretty much, although I'm not sure I understand how it works. only thing is it still completes with @ if ifconfig isn't available. I think @ and following characters should be stripped, at least when ip is being used. Maybe making a case for that is worth it?

JSON output is nice in theory, but opens the can of worms that with what would we parse the JSON with? jq, yq, ...? Probably quite a bit of code and even more fallbacks involved, not sure if it's worth it.

Agreed about that.

}

# Echo number of CPUs, falling back to 1 on failure.
Expand Down
1 change: 1 addition & 0 deletions test/t/unit/Makefile.am
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ EXTRA_DIST = \
test_unit_abspath.py \
test_unit_command_offset.py \
test_unit_compgen.py \
test_unit_compgen_available_interfaces.py \
test_unit_compgen_commands.py \
test_unit_count_args.py \
test_unit_delimited.py \
Expand Down
25 changes: 25 additions & 0 deletions test/t/unit/test_unit_compgen_available_interfaces.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
import pytest

from conftest import assert_bash_exec


@pytest.mark.bashcomp(cmd=None)
class TestUtilCompgenAvailableInterfaces:
@pytest.fixture
def functions(self, bash):
assert_bash_exec(
bash,
"_comp__test_dump() { ((${#arr[@]})) && printf '<%s>' \"${arr[@]}\"; echo; }",
)
assert_bash_exec(
bash,
'_comp__test_compgen() { local -a arr=(00); _comp_compgen -v arr "$@"; _comp__test_dump; }',
)

def test_1_trailing_colons(self, bash, functions):
output = assert_bash_exec(
bash,
"_comp__test_compgen available_interfaces",
want_output=True,
)
assert ":" not in output.strip()