code review 157121: 9P2000 client and server support.

9P2000 client and server support.
The srv package can be used to write custom file servers.
The clnt package can be used to connect to a 9P file server and
perform I/O operations on its files.

[lucho, 2009-11-21 06:34:16]

Hello r, rsc,

I'd like you to review the following change.

[rsc, 2009-12-14 03:54:36]

I think you're right to split things into three
packages - core protocol, server, and client -
but some things seem to be gratuitously different
in the core protocol.

The client seems to have twice as many entry
points as it needs.  For example, if I have
a *Fid I can call clnt.FRemove and fid.Remove.
Why both?

I haven't look as closely at the client and server
as I did at the core.

http://codereview.appspot.com/157121/diff/68/69
File src/pkg/Makefile (right):

http://codereview.appspot.com/157121/diff/68/69#newcode81
src/pkg/Makefile:81: plan9/p\
Sorry for the long delay getting back to you.

This code is probably too special-use to include
in the standard Go distribution, as much as it 
pains me to say that.  I encourage you to put
it on Google Code so that others can check it out,
though.  As I mentioned over email, I'd like to 
write acme clients in Go.

http://codereview.appspot.com/157121/diff/68/87
File src/pkg/plan9/p/osusers.go (right):

http://codereview.appspot.com/157121/diff/68/87#newcode10
src/pkg/plan9/p/osusers.go:10: type osUser struct {
It doesn't feel like this belongs in the core protocol package.
This is a detail for servers and clients, not for the protocol 
itself.

http://codereview.appspot.com/157121/diff/68/88
File src/pkg/plan9/p/p9.go (right):

http://codereview.appspot.com/157121/diff/68/88#newcode16
src/pkg/plan9/p/p9.go:16: Tfirst, Tversion	uint8	= 100 + iota, 100 + iota;
I don't think Tfirst and Tlast belong here.
Not sure about the uint8 type.  If anything
it should be its own new type name,
but having no type at all is probably okay too.

http://codereview.appspot.com/157121/diff/68/88#newcode48
src/pkg/plan9/p/p9.go:48: MSize	= 8216;	// default message size (8192+IOHdrSz)
s/8216/8192+IOHdrSz/ and then you can drop the comment

http://codereview.appspot.com/157121/diff/68/88#newcode55
src/pkg/plan9/p/p9.go:55: QTFILE		= 1 << iota;	// regular file
QTFILE is 0, not 1.
I think you'd be better off giving the explicit
constants here.  iota implies that these can
be changed, and they can't.

Alternately,

const (
    QTDIR = 0x80>>iota;
    QTAPPEND;
    QTEXCL;
    QTMOUNT;
    QTAUTH;
    QTTMP;
    QTSYMLINK;
    QTFILE = 0;
)

http://codereview.appspot.com/157121/diff/68/88#newcode97
src/pkg/plan9/p/p9.go:97: Notag	uint16	= 0xFFFF;	// no tag specified
The others are all caps; might as well make these
NOTAG, NOFID, NOUID.

http://codereview.appspot.com/157121/diff/68/88#newcode105
src/pkg/plan9/p/p9.go:105: Nerror	os.Errno;	// numeric representation of the
error (9P2000.u)
Should match the C definition: Errornum int;
There's no guarantee it matches the local os.Errno.

It could be ErrorNum but I think if you start
camelcasing things you'll go nuts trying to
be consistent.

http://codereview.appspot.com/157121/diff/68/88#newcode112
src/pkg/plan9/p/p9.go:112: Path	uint64;	// server's unique identification of the
file
Why is this upside down from struct Qid?

http://codereview.appspot.com/157121/diff/68/88#newcode115
src/pkg/plan9/p/p9.go:115: // Stat describes a file
Why is this Stat and not Dir?

http://codereview.appspot.com/157121/diff/68/88#newcode117
src/pkg/plan9/p/p9.go:117: Size	uint16;	// size-2 of the Stat on the wire
Why is this here?  It's not in the C struct Dir,
and as soon as one of the strings below gets
changed it will be wrong.

http://codereview.appspot.com/157121/diff/68/88#newcode120
src/pkg/plan9/p/p9.go:120: Sqid	Qid;	// file's Qid
s/Sqid/Qid/

http://codereview.appspot.com/157121/diff/68/88#newcode132
src/pkg/plan9/p/p9.go:132: Nuid	uint32;	// owner ID
Should match C: UidNum, GidNum, MuidNum.

http://codereview.appspot.com/157121/diff/68/88#newcode139
src/pkg/plan9/p/p9.go:139: size	uint32;	// size of the message
Shouldn't be here.

http://codereview.appspot.com/157121/diff/68/88#newcode140
src/pkg/plan9/p/p9.go:140: Id	uint8;	// message type
s/Id/Type/

http://codereview.appspot.com/157121/diff/68/88#newcode143
src/pkg/plan9/p/p9.go:143: Fid	uint32;		// file identifier
These should be in the same order as the C struct.

http://codereview.appspot.com/157121/diff/68/88#newcode153
src/pkg/plan9/p/p9.go:153: Wnames	[]string;	// list of names to walk (used by
Twalk)
No need for s on the end.
Wname []string
Wqid []Qid

http://codereview.appspot.com/157121/diff/68/88#newcode165
src/pkg/plan9/p/p9.go:165: Nerror	uint32;	// error code, 9P2000.u only (used by
Rerror)
Errornum

http://codereview.appspot.com/157121/diff/68/88#newcode167
src/pkg/plan9/p/p9.go:167: Nuname	uint32;	// user ID, 9P2000.u only (used by
Tauth, Tattach)
Uidnum

http://codereview.appspot.com/157121/diff/68/88#newcode169
src/pkg/plan9/p/p9.go:169: Pkt	[]uint8;	// raw packet data
Doesn't belong here.

http://codereview.appspot.com/157121/diff/68/88#newcode173
src/pkg/plan9/p/p9.go:173: type Users interface {
Doesn't belong in this package.

http://codereview.appspot.com/157121/diff/68/88#newcode181
src/pkg/plan9/p/p9.go:181: type User interface {
Doesn't belong in this package.

http://codereview.appspot.com/157121/diff/68/88#newcode188
src/pkg/plan9/p/p9.go:188: type Group interface {
Doesn't belong in this package.

http://codereview.appspot.com/157121/diff/68/88#newcode312
src/pkg/plan9/p/p9.go:312: if buf == nil {
Since you already handle buf == nil in gstr, seems
like this is unnecessary.

http://codereview.appspot.com/157121/diff/68/88#newcode394
src/pkg/plan9/p/p9.go:394: func ppint64(val uint64, buf []byte, pval *uint64)
[]byte {
This is an odd function.
I think the call site would be clearer
if you just did *pval = val separately
from the call.

http://codereview.appspot.com/157121/diff/68/89
File src/pkg/plan9/p/packr.go (right):

http://codereview.appspot.com/157121/diff/68/89#newcode8
src/pkg/plan9/p/packr.go:8: func PackRversion(fc *Fcall, msize uint32, version
string) *Error {
I don't think these should be separate entry points.
If there's one Fcall, there should be one Pack.
And it can be a method on Fcall.

http://codereview.appspot.com/157121/diff/68/89#newcode154
src/pkg/plan9/p/packr.go:154: for i := 0; i < len(data); i++ {
This is pretty inefficient.
The C code would have shared the buffer:

fc.Data = data

why not do the same here?

[lucho, 2009-12-15 15:05:48]

The client implements two, a bit overlapping APIs. One is for developers that
want to access the 9P files the same way they access OS files, and the other is
for developers that need lower-level access to the 9P (sending 9P messages).

http://codereview.appspot.com/157121/diff/68/69
File src/pkg/Makefile (right):

http://codereview.appspot.com/157121/diff/68/69#newcode81
src/pkg/Makefile:81: plan9/p\
It's your call, but it may be beneficial for all Go developers if there is an
easy way to represent resources as file trees.

http://codereview.appspot.com/157121/diff/68/87
File src/pkg/plan9/p/osusers.go (right):

http://codereview.appspot.com/157121/diff/68/87#newcode10
src/pkg/plan9/p/osusers.go:10: type osUser struct {
You are correct. But both the client and the server need a way to resolve users
and groups, the core protocol directory looks like a good place. I can create a
common/ directory for it if you think it fits better.

http://codereview.appspot.com/157121/diff/68/88
File src/pkg/plan9/p/p9.go (right):

http://codereview.appspot.com/157121/diff/68/88#newcode115
src/pkg/plan9/p/p9.go:115: // Stat describes a file
Already fixed in the separate repository.

http://codereview.appspot.com/157121/diff/68/88#newcode117
src/pkg/plan9/p/p9.go:117: Size	uint16;	// size-2 of the Stat on the wire
But the value is part of the on-the-wire representation. Currently it is used
only to figure out how much data was consumed by UnpackDir.

http://codereview.appspot.com/157121/diff/68/88#newcode312
src/pkg/plan9/p/p9.go:312: if buf == nil {
gstr returns nil if the slice is smaller than the string size.

http://codereview.appspot.com/157121/diff/68/88#newcode394
src/pkg/plan9/p/p9.go:394: func ppint64(val uint64, buf []byte, pval *uint64)
[]byte {
Removed.

http://codereview.appspot.com/157121/diff/68/89
File src/pkg/plan9/p/packr.go (right):

http://codereview.appspot.com/157121/diff/68/89#newcode8
src/pkg/plan9/p/packr.go:8: func PackRversion(fc *Fcall, msize uint32, version
string) *Error {
Separate functions make it clearer which fields of the Fcall struct are actually
used by the particular message. I got bitten few times by forgetting to set a
field that was required by a 9P message.

http://codereview.appspot.com/157121/diff/68/89#newcode154
src/pkg/plan9/p/packr.go:154: for i := 0; i < len(data); i++ {
We need to copy the data from the user's buffer to the one that contains the
on-the-wire message. bytes.Copy disappeared at some point and I didn't find
copy() before I submitted the patch.

On 2009/12/14 03:54:36, rsc wrote:
> This is pretty inefficient.
> The C code would have shared the buffer:
> 
> fc.Data = data
> 
> why not do the same here?
> 
>