This repository was archived by the owner on Aug 14, 2020. It is now read-only.
schema/types: rework empty volume type code #541
Merged
Conversation
This file contains hidden or 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
Contributor
Author
|
/cc @iaguis |
Merged
schema/types/volume_test.go
Outdated
tl;dr: this patch: - fixes a panic in the Volume type's String() method - ensures that the serialization of a Volume to/from a string is reversible - i.e., `s == VolumeFromString(s).String()` Full details: The `Volume` type is polymorphic; it can be of several different Kinds, and for each Kind only a select few fields are valid. When new fields (`mode`, `uid`, `gid`) were added to the `empty` volume Kind (via 26af7ce), they were simply added as new fields. However, this means that Volume structs of Kinds for which those fields are not valid (e.g. the `host` Kind) would necessarily have values set in those fields - since in Go any fields will be automatically set to their zero value. To mitigate this, the previous code set special sentinel values to those fields (e.g. -1 to the `uid`/`mode` fields) to indicate that they were "unset"; but this is slightly awkward since it necessitates manually setting those sentinel values at certain points. This patch instead change those fields to be pointers, so that it is trivially possible to determine whether or not those fields are really set, and disambiguate from the zeroed value case. This also means that a "zeroed" Volume type is really unset and does not need modification to set the sentinels appropriately. Another change is in how exactly a Volume type is serialized to a string. `String()` previously attempted to dereference the `readOnly` field even if it was not necessarily set. Even though such a nil dereference would ordinarily cause the Go runtime to panic, the `fmt.Printf` function (and friends) uses a `recover()` to actually _catch_ any panic occurred, so instead this results in a weird panic trace _within the returned string_, but business otherwise continues as usual (smdh). As well as fixing this panic (by checking the pointer before dereferencing), we now also construct the serialized Volume string piece-by-piece, only if fields are set; for example, if an `empty` Volume's `mode` field is set to the default value, we will omit it from the serialized string.
jonboulle
force-pushed
the
voltypes
branch
2 times, most recently
from
0f625bf to
69f4046
Compare
Contributor
|
lgtm |
Contributor
There was a problem hiding this comment.
even if it works this looks weird
Contributor
Author
There was a problem hiding this comment.
it's idiomatic actually :/
https://golang.org/doc/effective_go.html
search for "req := req"
jonboulle
added a commit
that referenced
this pull request
Nov 26, 2015
schema/types: rework empty volume type code
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
5 participants
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.
tl;dr: this patch:
reversible - i.e.,
s == VolumeFromString(s).String()Full details:
The
Volumetype is polymorphic; it can be of several different Kinds,and for each Kind only a select few fields are valid. When new fields
(
mode,uid,gid) were added to theemptyvolume Kind (via26af7ce), they were simply added as new
fields. However, this means that Volume structs of Kinds for which those
fields are not valid (e.g. the
hostKind) would necessarily havevalues set in those fields - since in Go any fields will be
automatically set to their zero value. To mitigate this, the previous
code set special sentinel values to those fields (e.g. -1 to the
uid/modefields) to indicate that they were "unset"; but this isslightly awkward since it necessitates manually setting those sentinel
values at certain points.
This patch instead change those fields to be pointers, so that it is
trivially possible to determine whether or not those fields are really
set, and disambiguate from the zeroed value case. This also means that a
"zeroed" Volume type is really unset and does not need modification to
set the sentinels appropriately.
Another change is in how exactly a Volume type is serialized to a
string.
String()previously attempted to dereference thereadOnlyfield even if it was not necessarily set. Even though such a nil
dereference would ordinarily cause the Go runtime to panic, the
fmt.Printffunction (and friends) uses arecover()to actuallycatch any panic occurred, so instead this results in a weird panic
trace within the returned string, but business otherwise continues as
usual (smdh).
As well as fixing this panic (by checking the pointer before
dereferencing), we now also construct the serialized Volume string
piece-by-piece, only if fields are set; for example, if an
emptyVolume's
modefield is set to the default value, we will omit it fromthe serialized string.