refactor command funcs arguments set up #226
Merged
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
phlogistonjohn
force-pushed
the
jjm-command-funcs-args-refactor
branch
from
04300ac to
997e802
Compare
3 tasks
phlogistonjohn
force-pushed
the
jjm-command-funcs-args-refactor
branch
2 times, most recently
from
5c086df to
87589cc
Compare
phlogistonjohn
changed the title
nixpanic
reviewed
May 1, 2020
phlogistonjohn
force-pushed
the
jjm-command-funcs-args-refactor
branch
from
87589cc to
be62492
Compare
nixpanic
approved these changes
May 1, 2020
Now we have sufficient boilerplate in our code for interacting with various types and ceph calls with similar needs we establish a new internal package, "cutil" (C utilities). Note that many of the return types are wrapped. This is due to the limits placed on us by cgo. Despite the irritating limitations Go places on "exporting" C types it still ought to help in the long run for patterns that are very common or patterns that are subtle and we want to write specific tests for. Signed-off-by: John Mulligan <[email protected]>
Signed-off-by: John Mulligan <[email protected]>
Replace the argument handling (wrangling?) around *_command functions, such as MonCommand, with the new command input and output helper types from the internal/cutil package. No external change should be visible. Signed-off-by: John Mulligan <[email protected]>
Similar to the functions in the rados pkg, cephfs package has a function MdsCommand that is mostly boilerplate that to set up the C function's arguments. Replace it with the types from the new cutil internal package. Signed-off-by: John Mulligan <[email protected]>
The *_command functions in librados and libcephfs document the use of specific free functions for data allocated. These functions are currently just wrappers around C's free() function. However, to be more strictly compliant this change adds a free-function callback to the CommandOutput type and the specific free functions are now used outside the unit tests. Signed-off-by: John Mulligan <[email protected]>
phlogistonjohn
force-pushed
the
jjm-command-funcs-args-refactor
branch
from
29e2edf to
2b54175
Compare
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.
Why we may want to do this:
The various *Command and *CommandWithInputBuffer functions all behave similarly but we are effectively copy-n-pasting the argument and result processing over and over. These changes establish a new
cutilinternal package to help with this and other future c-and-go conversions that are common. It makes two new types one for handling the input, and one for the output. Then we convert the functions in rados and cephfs.Why we may not want to do this:
Go's cgo support is pretty nice when used entirely within one package but the current design of go/cgo is pretty hostile to allowing cgo details to transition across packages. You can not use the cgo types directly and thus have to resort to workarounds, like overuse (abuse?) of unsafe.Pointer.
As this is an internal package I'm less conerened about unsafe.Pointer because we are not advertising these types for general use, and we who work on the library itself should know what we're doing (famous last words).
It does mean that the code reduction is smaller than I'd like.
Checklist