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

parse: improve the parsing of access-points (LP: #1809994) #413

Merged
merged 2 commits into from
Oct 16, 2023

Conversation

daniloegea
Copy link
Collaborator

@daniloegea daniloegea commented Oct 6, 2023

When the parser requires a second pass, it will report access-points it already parsed as duplicates and error out.

With this change, access-points will be stored in a local hash table, which will be used to find duplicates in the same netdef, and moved to the netdef->access_points hash table in the end. Access-points already present in the netdef hash table will be considered already parsed and skipped.

Add a test to check the problem is fixed and another one to test if merging the access-points when the same interface is present in multiple files is working properly.

See LP#1809994 for more details.

Description

Checklist

  • Runs make check successfully.
  • Retains 100% code coverage (make check-coverage).
  • New/changed keys in YAML format are documented.
  • (Optional) Adds example YAML for new feature.
  • (Optional) Closes an open bug in Launchpad.

Copy link
Collaborator

@slyon slyon left a comment

Choose a reason for hiding this comment

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

Thank you, this looks mostly good to me! There's one thought mentioned inline, which should be fixed, though.

Also, I think we have a more generic issue here, too: Usually we allow merging of NetDefs on a per-setting level, while we're now skipping/replacing full access-points/SSID blocks, just merging full access-points/SSID blocks from different files (ignoring their individual settings). This is still better than what we had before, where we didn't allow skipping/replacing any duplicated SSID blocks. But we should think about how we could allow merging of individual AP settings across YAML files.

src/parse.c Outdated Show resolved Hide resolved
src/parse.c Show resolved Hide resolved
When the parser requires a second pass, it will report access-points it
already parsed as duplicates and error out.

With this change, access-points will be stored in a local hash table,
which will be used to find duplicates in the same netdef, and moved to
the netdef->access_points hash table in the end. Access-points already
present in the netdef hash table will be considered already parsed and
skipped.

Add a test to check the problem is fixed and another one to test if
merging the access-points when the same interface is present in multiple
files is working properly.
If we find an AP with the same name in the same interface, we drop the
first one and use the new one. This is done to maintain the documented
behavior of respecting the order in which the files are parsed. We still
need to implement support for merging AP settings.
Copy link
Collaborator

@slyon slyon left a comment

Choose a reason for hiding this comment

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

lgtm.

@daniloegea daniloegea merged commit b6020cb into canonical:main Oct 16, 2023
14 checks passed
@isness
Copy link

isness commented Nov 9, 2023

When will the update be released? I have this issue on multiple Ubuntu 23.10 machines that results in them becoming unable to connect until network definitions are manually deleted from /etc/netplan. Manually deleting them is not that user-friendly for ordinary users.

@slyon
Copy link
Collaborator

slyon commented Jan 25, 2024

It has been released as part of the 0.107.1 bugfix release, currently available in Ubuntu 24.04 "Noble Numbat" development release and Debian testing.
We plan to backport this into Ubuntu 23.10 (and 22.04) in the March/April timeframe.

@slyon
Copy link
Collaborator

slyon commented Jan 25, 2024

Actually, this seems to be fixed in Ubuntu 23.10 already (as of Nov 15), see https://launchpad.net/ubuntu/+source/netplan.io/0.107-5ubuntu0.2

@isness Can you confirm this?

@isness
Copy link

isness commented Jan 25, 2024

@slyon Yes, it seems it's fixed now.

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.

3 participants