Code review.
authorChris Jones <chris@cjones.org>
Wed, 28 Dec 2011 12:39:24 -0700
changeset 17 d269d9c0fc8e
parent 16 b839e37b3f29
child 18 9f4af32925bf
Code review.
Makefile
stream.go
structs.go
xmpp.go
--- a/Makefile	Wed Dec 28 11:57:29 2011 -0700
+++ b/Makefile	Wed Dec 28 12:39:24 2011 -0700
@@ -10,4 +10,6 @@
 	stream.go \
 	structs.go \
 
+# TODO Add a target to build examples.
+
 include $(GOROOT)/src/Make.pkg
--- a/stream.go	Wed Dec 28 11:57:29 2011 -0700
+++ b/stream.go	Wed Dec 28 12:39:24 2011 -0700
@@ -5,7 +5,8 @@
 // This file contains the three layers of processing for the
 // communication with the server: transport (where TLS happens), XML
 // (where strings are converted to go structures), and Stream (where
-// we respond to XMPP events on behalf of the library client).
+// we respond to XMPP events on behalf of the library client), or send
+// those events to the client.
 
 package xmpp
 
@@ -26,11 +27,16 @@
 	"xml"
 )
 
+// Callback to handle a stanza with a particular id.
 type stanzaHandler struct {
 	id string
+	// Return true means pass this to the application
 	f func(Stanza) bool
 }
 
+// TODO Review all these *Client receiver methods. They should
+// probably either all be receivers, or none.
+
 func (cl *Client) readTransport(w io.Writer) {
 	defer tryClose(cl.socket, w)
 	cl.socket.SetReadTimeout(1e8)
@@ -138,6 +144,10 @@
 			break
 		}
 
+		// TODO If it's a Stanza, use reflection to search for
+		// any Unrecognized elements and fill in their
+		// attributes.
+
 		// Put it on the channel.
 		ch <- obj
 	}
@@ -160,6 +170,8 @@
 	}
 }
 
+// TODO This should go away. We shouldn't allow writing of
+// unstructured data.
 func writeText(w io.Writer, ch <-chan *string) {
 	if debug {
 		pr, pw := io.Pipe()
@@ -214,6 +226,9 @@
 	}
 }
 
+// TODO Disable this loop until resource binding is
+// complete. Otherwise the app might inject something weird into our
+// negotiation stream.
 func writeStream(srvOut chan<- interface{}, cliIn <-chan interface{}) {
 	defer tryClose(srvOut, cliIn)
 
@@ -240,6 +255,7 @@
 
 	if fe.Bind != nil {
 		cl.bind(fe.Bind)
+		return
 	}
 }
 
@@ -291,6 +307,7 @@
 	cl.socketSync.Done()
 }
 
+// TODO Implement TLS/SASL EXTERNAL.
 func (cl *Client) chooseSasl(fe *Features) {
 	var digestMd5 bool
 	for _, m := range(fe.Mechanisms.Mechanism) {
@@ -436,6 +453,7 @@
 	return m
 }
 
+// Inverse of parseSasl().
 func packSasl(m map[string]string) string {
 	var terms []string
 	for key, value := range(m) {
@@ -447,6 +465,7 @@
 	return strings.Join(terms, ",")
 }
 
+// Computes the response string for digest authentication.
 func saslDigestResponse(username, realm, passwd, nonce, cnonceStr,
 	authenticate, digestUri, nonceCountStr string) string {
 	h := func(text string) []byte {
@@ -470,6 +489,7 @@
 	return response
 }
 
+// Send a request to bind a resource. RFC 3920, section 7.
 func (cl *Client) bind(bind *Unrecognized) {
 	res := cl.Jid.Resource
 	msg := &Iq{Type: "set", Id: cl.NextId(), Any:
@@ -512,6 +532,10 @@
 	cl.xmlOut <- msg
 }
 
+// Register a callback to handle the next XMPP stanza (iq, message, or
+// presence) with a given id. The provided function will not be called
+// more than once. If it returns false, the stanza will not be made
+// available on the normal Client.In channel.
 func (cl *Client) HandleStanza(id string, f func(Stanza) bool) {
 	h := &stanzaHandler{id: id, f: f}
 	cl.handlers <- h
--- a/structs.go	Wed Dec 28 11:57:29 2011 -0700
+++ b/structs.go	Wed Dec 28 12:39:24 2011 -0700
@@ -30,6 +30,7 @@
 var _ flag.Value = &JID{}
 
 // XMPP's <stream:stream> XML element
+// TODO Hide this.
 type Stream struct {
 	To string `xml:"attr"`
 	From string `xml:"attr"`
@@ -41,12 +42,14 @@
 var _ fmt.Stringer = &Stream{}
 
 // <stream:error>
+// TODO Can this be consolidated with Error?
 type StreamError struct {
 	Any definedCondition
 	Text *errText
 }
 var _ xml.Marshaler = &StreamError{}
 
+// TODO Replace this with Unrecognized.
 type definedCondition struct {
 	// Must always be in namespace nsStreams
 	XMLName xml.Name
@@ -59,6 +62,7 @@
 }
 var _ xml.Marshaler = &errText{}
 
+// TODO Store this in Client and make it available to the app.
 type Features struct {
 	Starttls *starttls
 	Mechanisms mechs
@@ -67,7 +71,7 @@
 
 type starttls struct {
 	XMLName xml.Name
-	required *string
+	Required *string
 }
 
 type mechs struct {
@@ -81,17 +85,28 @@
 	Any *Unrecognized
 }
 
+// One of the three core XMPP stanza types: iq, message, presence. See
+// RFC3920, section 9.
 type Stanza interface {
+	// Returns "iq", "message", or "presence".
 	XName() string
+	// The to attribute.
 	XTo() string
+	// The from attribute.
 	XFrom() string
+	// The id attribute.
 	XId() string
+	// The type attribute.
 	XType() string
+	// The xml:lang attribute.
 	XLang() string
+	// A nested error element, if any.
 	XError() *Error
+	// A (non-error) nested element, if any.
 	XChild() *Unrecognized
 }
 
+// message stanza
 type Message struct {
 	To string `xml:"attr"`
 	From string `xml:"attr"`
@@ -104,6 +119,7 @@
 var _ xml.Marshaler = &Message{}
 var _ Stanza = &Message{}
 
+// presence stanza
 type Presence struct {
 	To string `xml:"attr"`
 	From string `xml:"attr"`
@@ -116,6 +132,7 @@
 var _ xml.Marshaler = &Presence{}
 var _ Stanza = &Presence{}
 
+// iq stanza
 type Iq struct {
 	To string `xml:"attr"`
 	From string `xml:"attr"`
@@ -128,12 +145,16 @@
 var _ xml.Marshaler = &Iq{}
 var _ Stanza = &Iq{}
 
+// Describes an XMPP stanza error. See RFC 3920, Section 9.3.
 type Error struct {
+	// The error type attribute.
 	Type string `xml:"attr"`
+	// Any nested element, if present.
 	Any *Unrecognized
 }
 var _ xml.Marshaler = &Error{}
 
+// Holds an XML element not described by the more specific types.
 // TODO Rename this to something like Generic.
 type Unrecognized struct {
 	XMLName xml.Name
--- a/xmpp.go	Wed Dec 28 11:57:29 2011 -0700
+++ b/xmpp.go	Wed Dec 28 12:39:24 2011 -0700
@@ -6,6 +6,9 @@
 // and 3921, plus the various XEPs at http://xmpp.org/protocols/.
 package xmpp
 
+// TODO Figure out why the library doesn't exit when the server closes
+// its stream to us.
+
 import (
 	"bytes"
 	"fmt"
@@ -30,11 +33,19 @@
 	serverSrv = "xmpp-server"
 	clientSrv = "xmpp-client"
 
+	// TODO Make this a parameter to NewClient, not a
+	// constant. We should have both a log level and a
+	// syslog.Writer, so the app can control how much time we
+	// spend generating log messages, as well as where they go.
 	debug = true
 )
 
 // The client in a client-server XMPP connection.
 type Client struct {
+	// This client's JID. This will be updated asynchronously when
+	// resource binding completes; at that time an iq stanza will
+	// be published on the In channel:
+	// <iq><bind><jid>jid</jid></bind></iq>
 	Jid JID
 	password string
 	socket net.Conn
@@ -44,15 +55,26 @@
 	idMutex sync.Mutex
 	nextId int64
 	handlers chan *stanzaHandler
+	// Incoming XMPP stanzas from the server will be published on
+	// this channel. Information which is only used by this
+	// library to set up the XMPP stream will not appear here.
+	// TODO Make these channels of type Stanza.
 	In <-chan interface{}
+	// Outgoing XMPP stanzas to the server should be sent to this
+	// channel.
 	Out chan<- interface{}
 	xmlOut chan<- interface{}
+	// TODO Remove this. Make a Stanza parser method available for
+	// use by interact.go and similar applications.
 	TextOut chan<- *string
 }
 var _ io.Closer = &Client{}
 
 // Connect to the appropriate server and authenticate as the given JID
-// with the given password.
+// with the given password. This function will return as soon as a TCP
+// connection has been established, but before XMPP stream negotiation
+// has completed. The negotiation will occur asynchronously, and sends
+// to Client.Out will block until negotiation is complete.
 func NewClient(jid *JID, password string) (*Client, os.Error) {
 	// Resolve the domain in the JID.
 	_, srvs, err := net.LookupSRV(clientSrv, "tcp", jid.Domain)
@@ -104,8 +126,6 @@
 	hsOut := &Stream{To: jid.Domain, Version: Version}
 	cl.xmlOut <- hsOut
 
-	// TODO Wait for initialization to finish.
-
 	cl.In = clIn
 	cl.Out = clOut
 	cl.TextOut = textOut
@@ -207,6 +227,8 @@
 	}
 }
 
+// This convenience function may be used to generate a unique id for
+// use in the Id fields of iq, message, and presence stanzas.
 func (cl *Client) NextId() string {
 	cl.idMutex.Lock()
 	defer cl.idMutex.Unlock()