launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/juju-core/mpv-fwreade-19-and-3 into lp:~maas-maintainers/juju-core/maas-provider-skeleton.

Commit message:
Fixes as per fwreade's review: protect all access to maasClientUnlocked (comment #19) and fix "Mutext" typo (comment #3).

Requested reviews:
  MAAS Maintainers (maas-maintainers)

For more details, see:
https://code.launchpad.net/~jtv/juju-core/mpv-fwreade-19-and-3/+merge/158280

This addresses William's review notes of the MAAS provider feature branch: https://code.launchpad.net/~maas-maintainers/juju-core/maas-provider-skeleton/+merge/157025 (this branch is being proposed for merging into that feature branch).

William's notes were:

«3) s/ecfgMutext/ecfgMutex/»

and

«19) client := environ.maasClientUnlocked.GetSubObject("nodes/") [FIX]

«The intended model is that Environ methods be safe from any goroutine. Use a maasClient() helper throughout to get a client safely. (see `// To properly observe e.storageUnlocked `)

«It may very well be the case that there's actually no point making Environs goroutine-safe, but I'm not going to analyse that now.»

We thought we'd be able to read the maasClientUnlocked pointer atomically, but looking at the Go memory model definition, that relies on a bunch of undefined terms so we can't be sure: http://golang.org/ref/mem

As far as I can see, that text does not specify that a pointer is a single machine word, which is exactly the sort of thing that this precise document would specify, so best not take chances.


Jeroen
-- 
https://code.launchpad.net/~jtv/juju-core/mpv-fwreade-19-and-3/+merge/158280
Your team MAAS Maintainers is requested to review the proposed merge of lp:~jtv/juju-core/mpv-fwreade-19-and-3 into lp:~maas-maintainers/juju-core/maas-provider-skeleton.

=== modified file 'environs/maas/environ.go'
--- environs/maas/environ.go	2013-04-11 04:42:00 +0000
+++ environs/maas/environ.go	2013-04-11 06:35:25 +0000
@@ -39,7 +39,7 @@
 type maasEnviron struct {
 	name string
 
-	// ecfgMutext protects the *Unlocked fields below.
+	// ecfgMutex protects the *Unlocked fields below.
 	ecfgMutex sync.Mutex
 
 	ecfgUnlocked       *maasEnvironConfig
@@ -291,6 +291,15 @@
 	return nil
 }
 
+// getMAASClient returns a MAAS client object to use for a request, in a
+// lock-protected fashioon.
+func (env *maasEnviron) getMAASClient() *gomaasapi.MAASObject {
+	env.ecfgMutex.Lock()
+	defer env.ecfgMutex.Unlock()
+
+	return env.maasClientUnlocked
+}
+
 // acquireNode allocates a node from the MAAS.
 func (environ *maasEnviron) acquireNode() (gomaasapi.MAASObject, error) {
 	retry := trivial.AttemptStrategy{
@@ -300,7 +309,7 @@
 	var result gomaasapi.JSONObject
 	var err error
 	for a := retry.Start(); a.Next(); {
-		client := environ.maasClientUnlocked.GetSubObject("nodes/")
+		client := environ.getMAASClient().GetSubObject("nodes/")
 		result, err = client.CallPost("acquire", nil)
 		if err == nil {
 			break
@@ -445,7 +454,7 @@
 // that could be found plus the error environs.ErrPartialInstances in the error
 // return.
 func (environ *maasEnviron) instances(ids []state.InstanceId) ([]environs.Instance, error) {
-	nodeListing := environ.maasClientUnlocked.GetSubObject("nodes")
+	nodeListing := environ.getMAASClient().GetSubObject("nodes")
 	filter := getSystemIdValues(ids)
 	listNodeObjects, err := nodeListing.CallGet("list", filter)
 	if err != nil {

=== modified file 'environs/maas/storage.go'
--- environs/maas/storage.go	2013-03-14 16:10:10 +0000
+++ environs/maas/storage.go	2013-04-11 06:35:25 +0000
@@ -27,13 +27,9 @@
 var _ environs.Storage = (*maasStorage)(nil)
 
 func NewStorage(env *maasEnviron) environs.Storage {
-	env.ecfgMutex.Lock()
-	defer env.ecfgMutex.Unlock()
-
-	filesClient := env.maasClientUnlocked.GetSubObject("files")
 	storage := new(maasStorage)
 	storage.environUnlocked = env
-	storage.maasClientUnlocked = filesClient
+	storage.maasClientUnlocked = env.getMAASClient().GetSubObject("files")
 	return storage
 }