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

Switchlink header file cleanup #85

Merged
merged 10 commits into from
Mar 9, 2024
Merged

Switchlink header file cleanup #85

merged 10 commits into from
Mar 9, 2024

Conversation

ffoulkes
Copy link
Contributor

@ffoulkes ffoulkes commented Mar 4, 2024

These changes emerged from an effort to implement Bazel support for the Kernel Monitor.

  • Some of them make the name of a header file better reflect its contents.
  • Some of them remove dead code.
  • Some of them reduce coupling and improve separation of concerns between between source and header files. (Reducing coupling is one of the things you can do to speed up the build.)
  • One of them breaks a cyclical dependency between header files, which is something Bazel won't tolerate.
  • And there are a few Boy Scout Rule edits, to leave the codebase better than I found it.

I know it looks like a lot, but I think you'll see a small number of patterns being repeated across the files. I rated it medium because of the number of files changed. In truth, it's probably a minor effort (15 minutes or less).

  1. Renamed switchlink_handle.h to switchlink_handlers.h, to make it clear that this file defines interfaces to switchlink handlers, not switchlink_handle_t. (readability)
  2. Renamed switchlink_link.h to switchlink_link_types.h, to make it clear that this file defines switchlink data types, and is not the header file for switchlink_link.c. (readability)
  3. Moved declarations for switchlink_globals.c to a new switchlink_globals.h header file. (separation of concerns; cohesiveness)
  4. Deleted unused SWITCHLINK_LOG_XXX and g_log_level definitions. (dead code)
  5. Deleted unused definition of switchlink_get_nl_sock(). (dead code)
  6. Deleted unused definition of switchlink_entry_type. (dead code)
  7. Made switchlink_nhop_using_by a simple enum instead of a typedef. (reducing complexity and coupling.)

@ffoulkes ffoulkes requested a review from 5abeel March 4, 2024 00:24
@ffoulkes ffoulkes added the medium effort Moderate effort required label Mar 4, 2024
@ffoulkes ffoulkes marked this pull request as ready for review March 4, 2024 04:36
@ffoulkes ffoulkes changed the title Switchlink refactoring Switchlink housekeeping Mar 5, 2024
@ffoulkes ffoulkes changed the title Switchlink housekeeping Switchlink housekeeping and refactoring Mar 5, 2024
@ffoulkes ffoulkes changed the title Switchlink housekeeping and refactoring Switchlink header file cleanup Mar 5, 2024
ffoulkes added 10 commits March 6, 2024 09:30
Signed-off-by: Derek G Foster <[email protected]>
Signed-off-by: Derek G Foster <[email protected]>
Signed-off-by: Derek G Foster <[email protected]>
- Deleted unused definition of switchlink_get_nl_sock().

- Deleted unused definition of switchlink_entry_type.

- Made switchlink_nhop_using_by a simple enum instead of a typedef.

Signed-off-by: Derek G Foster <[email protected]>
- Deleted switchlink_neigh.h and switchlink_route.h, which are no
  longer used. They duplicate definitions in switchlink_int.h.

Signed-off-by: Derek G Foster <[email protected]>
Signed-off-by: Derek G Foster <[email protected]>
@ffoulkes ffoulkes force-pushed the switchlink-refactor branch from 7600cc1 to 18ef633 Compare March 6, 2024 17:30
Copy link
Contributor

@nupuruttarwar nupuruttarwar left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@5abeel 5abeel left a comment

Choose a reason for hiding this comment

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

LGTM

@ffoulkes ffoulkes merged commit f1f0495 into main Mar 9, 2024
4 checks passed
@ffoulkes ffoulkes deleted the switchlink-refactor branch March 9, 2024 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
medium effort Moderate effort required
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants