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

refactor(nm): NMDBusConnector class refactor #4517

Merged
merged 11 commits into from
Apr 6, 2023

Conversation

pierantoniomerlino
Copy link
Contributor

@pierantoniomerlino pierantoniomerlino commented Mar 31, 2023

Note: We are using the Conventional Commits convention for our pull request titles. Please take a look at the PR title format document for the supported types and scopes.

This PR perform a refactor of the NMDbusConnector class. In particular, it adds the apply(String deviceId) method that is used by the NMConfigurationEnforcementHandler to apply the configuration to a single device/interface and it rewrites the doApply method in a simpler way.

@pierantoniomerlino pierantoniomerlino force-pushed the nm_dbus_connector_apply_single branch from e215432 to 262aca4 Compare April 3, 2023 08:58
@pierantoniomerlino pierantoniomerlino self-assigned this Apr 3, 2023
@pierantoniomerlino pierantoniomerlino marked this pull request as ready for review April 3, 2023 09:20
@pierantoniomerlino pierantoniomerlino force-pushed the nm_dbus_connector_apply_single branch from 262aca4 to e89a422 Compare April 4, 2023 08:10
@pierantoniomerlino pierantoniomerlino force-pushed the nm_dbus_connector_apply_single branch from 5e85de6 to 2cdcc76 Compare April 5, 2023 11:04
@mattdibi
Copy link
Contributor

mattdibi commented Apr 5, 2023

For documentation purpose I will report here my findings on a new behaviour with this PR.

I performed a couple of tests using a Telit HE910 modem. When connected this device creates an additional NetworkManager device of type PPP.

image

image

When a new configuration is issued (for example disconnecting the modem) the following stacktrace appears in the log:

MicrosoftTeams-image (9)

This is due to the fact that the PPP device (which is created upon the modem connecting), disappears and its dbus path invalidated. Since we're iterating over devices here:

https://github.com/eclipse/kura/blob/2cdcc7677f408857165ff164a60c784805639d91/kura/org.eclipse.kura.nm/src/main/java/org/eclipse/kura/nm/NMDbusConnector.java#L316-L324

when we get to the PPP device, it doesn't exist anymore and the DBus path is invalidated. This is all correctly handled by the code (the exception is handled and all remaining operation work as expected). No code chages are required here IMO.

Signed-off-by: pierantoniomerlino <[email protected]>
Signed-off-by: pierantoniomerlino <[email protected]>
Copy link
Contributor

@mattdibi mattdibi left a comment

Choose a reason for hiding this comment

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

LGTM

Signed-off-by: pierantoniomerlino <[email protected]>
Signed-off-by: pierantoniomerlino <[email protected]>
Signed-off-by: pierantoniomerlino <[email protected]>
Signed-off-by: pierantoniomerlino <[email protected]>
Signed-off-by: pierantoniomerlino <[email protected]>
@pierantoniomerlino pierantoniomerlino force-pushed the nm_dbus_connector_apply_single branch from 2cdcc76 to a472769 Compare April 5, 2023 15:13
@mattdibi mattdibi merged commit 841a769 into develop Apr 6, 2023
@mattdibi mattdibi deleted the nm_dbus_connector_apply_single branch April 6, 2023 06:29
@github-actions
Copy link
Contributor

github-actions bot commented Apr 6, 2023

The backport to release-5.3.0 failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-release-5.3.0 release-5.3.0
# Navigate to the new working tree
cd .worktrees/backport-release-5.3.0
# Create a new branch
git switch --create backport-4517-to-release-5.3.0
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 841a7693f9df37d804cbd9179f1b756471a1f2f0
# Push it to GitHub
git push --set-upstream origin backport-4517-to-release-5.3.0
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-release-5.3.0

Then, create a pull request where the base branch is release-5.3.0 and the compare/head branch is backport-4517-to-release-5.3.0.

pierantoniomerlino added a commit that referenced this pull request Apr 6, 2023
* Refectored doApply method in NMDBusConnector class

Signed-off-by: pierantoniomerlino <[email protected]>

* Revert formatting

Signed-off-by: pierantoniomerlino <[email protected]>

* Fixed typo

Signed-off-by: pierantoniomerlino <[email protected]>

* Added method to apply config for single device

Signed-off-by: pierantoniomerlino <[email protected]>

* Fixed tests

Signed-off-by: pierantoniomerlino <[email protected]>

* Improved code readability

Signed-off-by: pierantoniomerlino <[email protected]>

* Updated NMDeviceAddedHandler to use apply(deviceId)

Signed-off-by: pierantoniomerlino <[email protected]>

* Fixed and added tests

Signed-off-by: pierantoniomerlino <[email protected]>

* Revert formatting

Signed-off-by: pierantoniomerlino <[email protected]>

* Changed parameter names

Signed-off-by: pierantoniomerlino <[email protected]>

* Fixed DeviceAddedHandler and modem reset

Signed-off-by: pierantoniomerlino <[email protected]>

---------

Signed-off-by: pierantoniomerlino <[email protected]>
(cherry picked from commit 841a769)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants