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.

Sign up for GitHub

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

generate: Use non-null validation instead of initialization #540

Merged
merged 1 commit into from
Dec 13, 2017
Merged

generate: Use non-null validation instead of initialization #540

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 32 additions & 8 deletions generate/generate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -534,7 +534,10 @@ func (g *Generator) AddLinuxResourcesBlockIOLeafWeightDevice(major int64, minor

// DropLinuxResourcesBlockIOLeafWeightDevice drops a item form g.spec.Linux.Resources.BlockIO.WeightDevice.LeafWeight
func (g *Generator) DropLinuxResourcesBlockIOLeafWeightDevice(major int64, minor int64) {
g.initSpecLinuxResourcesBlockIO()
if g.spec == nil || g.spec.Linux == nil || g.spec.Linux.Resources == nil || g.spec.Linux.Resources.BlockIO == nil {
return
}

for i, weightDevice := range g.spec.Linux.Resources.BlockIO.WeightDevice {
if weightDevice.Major == major && weightDevice.Minor == minor {
if weightDevice.Weight != nil {
Expand Down Expand Up @@ -575,7 +578,10 @@ func (g *Generator) AddLinuxResourcesBlockIOWeightDevice(major int64, minor int6

// DropLinuxResourcesBlockIOWeightDevice drops a item form g.spec.Linux.Resources.BlockIO.WeightDevice.Weight
func (g *Generator) DropLinuxResourcesBlockIOWeightDevice(major int64, minor int64) {
g.initSpecLinuxResourcesBlockIO()
if g.spec == nil || g.spec.Linux == nil || g.spec.Linux.Resources == nil || g.spec.Linux.Resources.BlockIO == nil {
return
}

for i, weightDevice := range g.spec.Linux.Resources.BlockIO.WeightDevice {
if weightDevice.Major == major && weightDevice.Minor == minor {
if weightDevice.LeafWeight != nil {
Expand All @@ -601,7 +607,10 @@ func (g *Generator) AddLinuxResourcesBlockIOThrottleReadBpsDevice(major int64, m

// DropLinuxResourcesBlockIOThrottleReadBpsDevice drops a item from g.spec.Linux.Resources.BlockIO.ThrottleReadBpsDevice.
func (g *Generator) DropLinuxResourcesBlockIOThrottleReadBpsDevice(major int64, minor int64) {
g.initSpecLinuxResourcesBlockIO()
if g.spec == nil || g.spec.Linux == nil || g.spec.Linux.Resources == nil || g.spec.Linux.Resources.BlockIO == nil {
return
}

throttleDevices := dropBlockIOThrottleDevice(g.spec.Linux.Resources.BlockIO.ThrottleReadBpsDevice, major, minor)
g.spec.Linux.Resources.BlockIO.ThrottleReadBpsDevice = throttleDevices
}
Expand All @@ -615,7 +624,10 @@ func (g *Generator) AddLinuxResourcesBlockIOThrottleReadIOPSDevice(major int64,

// DropLinuxResourcesBlockIOThrottleReadIOPSDevice drops a item from g.spec.Linux.Resources.BlockIO.ThrottleReadIOPSDevice.
func (g *Generator) DropLinuxResourcesBlockIOThrottleReadIOPSDevice(major int64, minor int64) {
g.initSpecLinuxResourcesBlockIO()
if g.spec == nil || g.spec.Linux == nil || g.spec.Linux.Resources == nil || g.spec.Linux.Resources.BlockIO == nil {
return
}

throttleDevices := dropBlockIOThrottleDevice(g.spec.Linux.Resources.BlockIO.ThrottleReadIOPSDevice, major, minor)
g.spec.Linux.Resources.BlockIO.ThrottleReadIOPSDevice = throttleDevices
}
Expand All @@ -629,7 +641,10 @@ func (g *Generator) AddLinuxResourcesBlockIOThrottleWriteBpsDevice(major int64,

// DropLinuxResourcesBlockIOThrottleWriteBpsDevice drops a item from g.spec.Linux.Resources.BlockIO.ThrottleWriteBpsDevice.
func (g *Generator) DropLinuxResourcesBlockIOThrottleWriteBpsDevice(major int64, minor int64) {
g.initSpecLinuxResourcesBlockIO()
if g.spec == nil || g.spec.Linux == nil || g.spec.Linux.Resources == nil || g.spec.Linux.Resources.BlockIO == nil {
return
}

throttleDevices := dropBlockIOThrottleDevice(g.spec.Linux.Resources.BlockIO.ThrottleWriteBpsDevice, major, minor)
g.spec.Linux.Resources.BlockIO.ThrottleWriteBpsDevice = throttleDevices
}
Expand All @@ -643,7 +658,10 @@ func (g *Generator) AddLinuxResourcesBlockIOThrottleWriteIOPSDevice(major int64,

// DropLinuxResourcesBlockIOThrottleWriteIOPSDevice drops a item from g.spec.Linux.Resources.BlockIO.ThrottleWriteIOPSDevice.
func (g *Generator) DropLinuxResourcesBlockIOThrottleWriteIOPSDevice(major int64, minor int64) {
g.initSpecLinuxResourcesBlockIO()
if g.spec == nil || g.spec.Linux == nil || g.spec.Linux.Resources == nil || g.spec.Linux.Resources.BlockIO == nil {
return
}

throttleDevices := dropBlockIOThrottleDevice(g.spec.Linux.Resources.BlockIO.ThrottleWriteIOPSDevice, major, minor)
g.spec.Linux.Resources.BlockIO.ThrottleWriteIOPSDevice = throttleDevices
}
Expand Down Expand Up @@ -709,7 +727,10 @@ func (g *Generator) AddLinuxResourcesHugepageLimit(pageSize string, limit uint64

// DropLinuxResourcesHugepageLimit drops a hugepage limit from g.spec.Linux.Resources.HugepageLimits.
func (g *Generator) DropLinuxResourcesHugepageLimit(pageSize string) error {
g.initSpecLinuxResources()
if g.spec == nil || g.spec.Linux == nil || g.spec.Linux.Resources == nil {
Copy link

Choose a reason for hiding this comment

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

Almost every drop functions have this, I think you can add functions like initSpecLinuxResources() to form them.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can add functions like initSpecLinuxResources() to form them.

You don't need to create ancestor properties just to unset some descendant property. If any ancestor is unset, you know the descendant is already unset. For example, currently:

$ oci-runtime-tool generate --temlate <(echo '{}') | jq .linux
null
$ oci-runtime-tool generate --template <(echo '{}') --linux-hugepage-limits-drop 2MB | jq .linux
{
  "resources": {}
}

With this commit we'd no longer create the useless linux.resources entry.

However, even without this commit, we are currently not clearing useless entries after we empty an array. Calling g.initSpecLinuxResources() and seeing that the entry in question is already empty, and then removing the the useless entries is a bit of busy work, but wouldn't be the end of the world. And if you decide to not bother removing the useless entries, that wouldn't be the end of the world either.

Copy link

Choose a reason for hiding this comment

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

I think you misunderstood me. I don't mean to keep initSpecLinuxResources(), I agree to remove it in Drop*() functions.
I mean to add a new functions to judge nil as init*() functions.
As we have almost same if g.spec == nil || g.spec.* == nil sentence in many place, I think we'd better add new functions to form them. Looks more elegant.

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean to add a new functions to judge nil...

Ah. That would work, although I don't have a problem with the inline || chain either.

Copy link
Author

Choose a reason for hiding this comment

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

I mean to add a new functions to judge nil as init*() functions.

Fine, I'll update.

Copy link
Author

Choose a reason for hiding this comment

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

updated, PTAL

return nil
}

for i, pageLimit := range g.spec.Linux.Resources.HugepageLimits {
if pageLimit.Pagesize == pageSize {
g.spec.Linux.Resources.HugepageLimits = append(g.spec.Linux.Resources.HugepageLimits[:i], g.spec.Linux.Resources.HugepageLimits[i+1:]...)
Expand Down Expand Up @@ -785,7 +806,10 @@ func (g *Generator) AddLinuxResourcesNetworkPriorities(name string, prio uint32)

// DropLinuxResourcesNetworkPriorities drops one item from g.spec.Linux.Resources.Network.Priorities.
func (g *Generator) DropLinuxResourcesNetworkPriorities(name string) {
g.initSpecLinuxResourcesNetwork()
if g.spec == nil || g.spec.Linux == nil || g.spec.Linux.Resources == nil || g.spec.Linux.Resources.Network == nil {
return
}

for i, netPriority := range g.spec.Linux.Resources.Network.Priorities {
if netPriority.Name == name {
g.spec.Linux.Resources.Network.Priorities = append(g.spec.Linux.Resources.Network.Priorities[:i], g.spec.Linux.Resources.Network.Priorities[i+1:]...)
Expand Down