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

Jump to bottom

vcsim: Synchronize calls to OptionsManager internal data structures #2907

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

vcsim: Synchronize calls to OptionsManager internal data structures #2907

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
12 changes: 12 additions & 0 deletions simulator/option_manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package simulator

import (
"strings"
"sync"

"github.com/vmware/govmomi/object"
"github.com/vmware/govmomi/simulator/esx"
Expand All @@ -30,6 +31,7 @@ import (

type OptionManager struct {
mo.OptionManager
lock sync.Mutex
Copy link
Member

Choose a reason for hiding this comment

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

Rewrite this as:

type OptionManager struct {
    sync.Mutex
    mo.OptionManager
}

That way:

func (m *OptionManager) init(r *Registry) {
    m.Lock()
    defer m.Unlock()

Copy link
Member

Choose a reason for hiding this comment

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

Also, having read the rest of the change, please rewrite further:

type OptionManager struct {
    sync.RWMutex
    mo.OptionManager
}

There's zero reason this should be an exclusive write lock. It's far more performant to rely on shared locks for read-only operations.

}

func NewOptionManager(ref *types.ManagedObjectReference, setting []types.BaseOptionValue) object.Reference {
Expand All @@ -42,6 +44,9 @@ func NewOptionManager(ref *types.ManagedObjectReference, setting []types.BaseOpt
}

func (m *OptionManager) init(r *Registry) {
m.lock.Lock()
defer m.lock.Unlock()

if len(m.Setting) == 0 {
if r.IsVPX() {
m.Setting = vpx.Setting
Expand All @@ -55,6 +60,9 @@ func (m *OptionManager) QueryOptions(req *types.QueryOptions) soap.HasFault {
body := &methods.QueryOptionsBody{}
res := &types.QueryOptionsResponse{}

m.lock.Lock()
Copy link
Member

Choose a reason for hiding this comment

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

  1. Move the lock calls the top of this function. The only thing currently above the calls are non-expensive variable declarations, so it is well-worth moving the lock entrance to the top of the function where it's more evident.
  2. Rewrite to use the RLock/RUnlock methods: 
    m.RLock()
defer m.RUnlock()

defer m.lock.Unlock()

for _, opt := range m.Setting {
if strings.HasPrefix(opt.GetOptionValue().Key, req.Name) {
res.Returnval = append(res.Returnval, opt)
Expand All @@ -70,6 +78,7 @@ func (m *OptionManager) QueryOptions(req *types.QueryOptions) soap.HasFault {
return body
}

// find requires holding the lock.
Copy link
Member

Choose a reason for hiding this comment

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

Either:

  • Update the GoDoc to be more clear: 
    // find is not thread-safe and requires callers to already hold either
// a shared or exclusive lock from the OptionsManager

or

  • Move the function into UpdateOptions so only it can invoke find, ex: 
    func (m *OptionManager) UpdateOptions(req *types.UpdateOptions) soap.HasFault {
	m.Lock()
	defer m.Unlock()

	find := func(key string) *types.OptionValue {
		for _, opt := range m.Setting {
			setting := opt.GetOptionValue()
			if setting.Key == key {
				return setting
			}
		}
		return nil
	}

	// ...

func (m *OptionManager) find(key string) *types.OptionValue {
for _, opt := range m.Setting {
setting := opt.GetOptionValue()
Expand All @@ -83,6 +92,9 @@ func (m *OptionManager) find(key string) *types.OptionValue {
func (m *OptionManager) UpdateOptions(req *types.UpdateOptions) soap.HasFault {
body := new(methods.UpdateOptionsBody)

m.lock.Lock()
defer m.lock.Unlock()

for _, change := range req.ChangedValue {
setting := change.GetOptionValue()

Expand Down