lib: unify how constant names are exposed and used #230
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## main #230 +/- ##
=======================================
Coverage 99.04% 99.05%
=======================================
Files 57 57
Lines 9524 9599 +75
=======================================
+ Hits 9433 9508 +75
Misses 91 91
Continue to review full report at Codecov.
|
schopin-pro
force-pushed
the
lib_names_coherent_api
branch
2 times, most recently
from
f58aa9b
to
d950faa
Compare
3 tasks
slyon
requested changes
Sep 27, 2021
There was a problem hiding this comment.
Thank you Simon, I really like the consolidation of all *_name functions and moving them into a single compilation unit!
I do not really like the np_* API naming, though, as I think it is too generic. What do you think about netplan1_ (for the pending v1 API)?
I left some other smaller inline comments about formatting and missing copyright headers.
schopin-pro
force-pushed
the
lib_names_coherent_api
branch
4 times, most recently
from
2cdfb49
to
0ba72f3
Compare
|
Posted a V2, with the following changelog: V2:
|
slyon
approved these changes
Sep 28, 2021
src/names.c
Outdated
|
|
||
| /* ABI compatibility definitions */ | ||
|
|
||
| const char * |
There was a problem hiding this comment.
Suggested change
| const char * | |
| const char* |
Just a tiny nitpick...
This patch moves all the various enum-to-string arrays into a single compilation unit instead of exposing them directly into headers, and exposes the data through simple functions. This approach has multiple advantages: * If we want to expose one or several of those conversion functions to the ABI, we now can add new constants without breaking the ABI, as the functions can deal with unknown constants in a sensible manner. Directly exposing the arrays in the headers as was previously done was a recipy for disaster, as the array is baked into the compiled client code, and will not be resized if we decide to add new values to the enums in new versions of the library. * It's marginally faster to compile, as the compiler doesn't have to copy around multiple times the same arrays, only to deduplicate them at link time (if it even does that). V2: * Drop the np_ prefix in favor of the classical netplan_ prefix * Whitespace fixes * Copyright headers * Remove a couple of defines that sneaked up where they didn't belong
slyon
force-pushed
the
lib_names_coherent_api
branch
from
0ba72f3
to
17254d1
Compare
|
Actually... I just did that myself. Let's merge this! |
slyon
pushed a commit
that referenced
this pull request
Sep 30, 2021
The goal of this PR is to properly determine what the public interface of libnetplan is. There is potentially a bit of ABI breakage, as symbols now default to hidden status unless explicitly marked otherwise, and said marking was done manually by looking at the current consumers I had on my system and trying to fill in the blanks. The API, on the other hand, has been conscientiously broken, as most of what was exposed in the headers are now safely hidden away in our internal headers. This is by design, and AFAICT should not break existing code. This PR depends on #230, as denoted by the merge commit in there. It should make independent review a bit easier :). Given the fact that the main purpose of this PR is to limit the amount of symbols we export, we can safely assume that strictly speaking, there is ABI breakage. However, the whole point was to only expose the symbols that are in actual use out there in the real world. As usual, the patchset has been edited expecting a review commit by commit, as the whole diff can be a bit daunting. COMMITS: * libnetplan: rework netplan,parse{-nm},util.h as public headers This work involves splitting out some things from those headers into new internal headers, and moving definitions around so that the public headers are as self-contained as possible. For the new internal headers, I decided to have a big "types.h" header containing all the types used in a network definition. In itself, these types aren't related to parsing except that the parser module is the only producer, so I decided they could live on their own. This is also the place where type-specific helpers can be found, such as reset_netdef(). The various macros used to generate YAML were gathered up from both source headers into a new yaml-helpers.h header, whereas the various global state definitions were split off into their own headers, making it easier to spot which areas of the code still rely on global state. The remainder functions were moved into util-internal.h This allows us to minimize the API exposed to the outside world. V2: * Remove the extraneous struct net_definition line, as it is redundant with the typedef * Normalize all the symbol declarations to have the symbol name at the beginning of their own line, at least in the headers we touched. V3: * Remove extraneous stdbool.h header * Split parse-helpers.h into yaml-helpers.h and util-internal.h * Add uuid as a dependency of the dbus generator to make it compile with the new lay of the land. * Add some missing copyright headers * libnetplan: Properly mark the lib/bin interface This patch cleanly marks which functions/objects are part of the public library API, and which are meant to be consumed by our own binaries. In order to reduce as much as possible the ABI dependencies between our binaries and the library, I move the various generator modules into the library as it make sense for them to access directly the objects. On the other hand, both generate.c and dbus.c should be relatively trivial to change to use getters and setters instead of direct structure access. Those changes will be the object of later patches. V3: * Makefile: consolidate the pkg-config calls and add back the LDFLAGS variable * Normalize the touched function declarations to always have the symbol name at the start of the line. * libnetplan: add back ABI compatibility Re-export (and recreate) various symbols that are needed by the `generate` binary as shipped in the Ubuntu package 0.103-0ubuntu5 I have chosen a separate marker for those, in order to distinguish what was exposed deliberately and by accident. That way, if we need to do a SONAME bump, we can clean up the symbols marked with NETPLAN_ABI. V3: * Normalize the declarations that are touched to always have the symbol name at the start of the line * libnetplan: export Python-used symbols as internal * libnetplan: hide all symbols by default Only those marked by NETPLAN_{PUBLIC,INTERNAL,ABI} will be available to the loader.
Merged
4 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This patch moves all the various enum-to-string arrays into a single
compilation unit instead of exposing them directly into headers, and
exposes the data through simple functions.
This approach has multiple advantages:
the ABI, we now can add new constants without breaking the ABI, as the
functions can deal with unknown constants in a sensible manner.
Directly exposing the arrays in the headers as was previously done was
a recipy for disaster, as the array is baked into the compiled client code,
and will not be resized if we decide to add new values to the enums
in new versions of the library.
copy around multiple times the same arrays, only to deduplicate them
at link time (if it even does that).
V2:
Checklist
make checksuccessfully.make check-coverage).