Skip to content
This repository was archived by the owner on May 10, 2024. It is now read-only.

Support discriminated unions correctly #2

Merged
hickeng merged 1 commit into from
Oct 4, 2017
Merged

Support discriminated unions correctly #2

Show file tree
Hide file tree
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
24 changes: 16 additions & 8 deletions nfs/example/test/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,13 @@ func main() {
}
defer v.Close()

// discover any system files such as lost+found or .snapshot
dirs, err := ls(v, ".")
if err != nil {
log.Fatalf("ls: %s", err.Error())
}
baseDirCount := len(dirs)

if _, err = v.Mkdir(dir, 0775); err != nil {
log.Fatalf("mkdir error: %v", err)
}
Expand All @@ -65,14 +72,14 @@ func main() {
log.Fatalf("mkdir error: %v", err)
}

dirs, err := ls(v, ".")
dirs, err = ls(v, ".")
if err != nil {
log.Fatalf("ls: %s", err.Error())
}

// check the length. There should only be 1 entry in the target (aside from . and ..)
if len(dirs) != 3 {
log.Fatalf("expected 3 dirs, got %d", len(dirs))
// check the length. There should only be 1 entry in the target (aside from . and .., et al)
if len(dirs) != 1+baseDirCount {
log.Fatalf("expected %d dirs, got %d", 1+baseDirCount, len(dirs))
}

// 10 MB file
Expand Down Expand Up @@ -131,15 +138,16 @@ func main() {
log.Fatalf("ls: %s", err.Error())
}

if len(outDirs) != 2 {
log.Fatalf("directory shoudl be empty!")
if len(outDirs) != baseDirCount {
log.Fatalf("directory shoudl be empty of our created files!")
}

if err = mount.Unmount(); err != nil {
log.Fatalf("unable to umount target: %v", err)
}

mount.Close()
util.Infof("Completed tests")
}

func testFileRW(v *nfs.Target, name string, filesize uint64) error {
Expand All @@ -162,9 +170,9 @@ func testFileRW(v *nfs.Target, name string, filesize uint64) error {
t := io.TeeReader(f, h)

// Copy filesize
_, err = io.CopyN(wr, t, int64(filesize))
n, err := io.CopyN(wr, t, int64(filesize))
if err != nil {
util.Errorf("error copying: %s", err.Error())
util.Errorf("error copying: n=%d, %s", n, err.Error())
return err
}
expectedSum := h.Sum(nil)
Expand Down
81 changes: 50 additions & 31 deletions nfs/file.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,18 +34,15 @@ func (f *File) Read(p []byte) (int, error) {
}

type ReadRes struct {
Follows uint32
Attrs struct {
Attrs Fattr
}
Attr PostOpAttr
Copy link
Copy Markdown

@fdawg4l fdawg4l Oct 4, 2017

Choose a reason for hiding this comment

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

Curious. What happens the the Follows word? I don't see it defined in PostOpAttr.

Copy link
Copy Markdown

@mdubya66 mdubya66 Oct 4, 2017

Choose a reason for hiding this comment

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

fdawg! great to hear from you :)

Copy link
Copy Markdown
Author

@hickeng hickeng Oct 4, 2017

Choose a reason for hiding this comment

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

@fdawg4l Welcome! This is sort of like SAX vs DOM parsing for XML. Use of follows determines whether the gated data is even present in the XDR stream and that broke us - seems Linux had always been supplying the data, but when NetApp didn't we had misalignment in the rest of the structure.
To answer the specific query, Follows has been replaced with the xdr:"union" annotation which is in the xdr2 package specifically to support discriminated unions.

Copy link
Copy Markdown

@fdawg4l fdawg4l Oct 4, 2017

Choose a reason for hiding this comment

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

OOOHH!!! ok cool. Wow. That must've sucked. Good find.

Copy link
Copy Markdown
Author

@hickeng hickeng Oct 4, 2017

Choose a reason for hiding this comment

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

tcpdump + wireshark 4TW. Wireshark has awesome XDR/NFS protocol analytics built in.

Copy link
Copy Markdown

@fdawg4l fdawg4l Oct 4, 2017

Choose a reason for hiding this comment

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

It was the only way i made any progress was with wireshark. Agreed. Invaluable tool. And it lets you go up and down the stack; tcp, rpc, nfs.

Count uint32
EOF uint32
Data struct {
Length uint32
}
}

readSize := uint32(min(uint64(f.fsinfo.RTPref), uint64(len(p))))
readSize := min(f.fsinfo.RTPref, uint32(len(p)))
util.Debugf("read(%x) len=%d offset=%d", f.fh, readSize, f.curr)

r, err := f.call(&ReadArgs{
Expand Down Expand Up @@ -97,36 +94,58 @@ func (f *File) Write(p []byte) (int, error) {
Contents []byte
}

totalToWrite := len(p)
writeSize := uint64(min(uint64(f.fsinfo.WTPref), uint64(totalToWrite)))
type WriteRes struct {
Wcc WccData
Count uint32
How uint32
WriteVerf uint64
}

_, err := f.call(&WriteArgs{
Header: rpc.Header{
Rpcvers: 2,
Prog: Nfs3Prog,
Vers: Nfs3Vers,
Proc: NFSProc3Write,
Cred: f.auth,
Verf: rpc.AuthNull,
},
FH: f.fh,
Offset: f.curr,
Count: uint32(writeSize),
How: 2,
Contents: p[:writeSize],
})
totalToWrite := uint32(len(p))
written := uint32(0)

for written = 0; written < totalToWrite; {
writeSize := min(f.fsinfo.WTPref, totalToWrite-written)

res, err := f.call(&WriteArgs{
Header: rpc.Header{
Rpcvers: 2,
Prog: Nfs3Prog,
Vers: Nfs3Vers,
Proc: NFSProc3Write,
Cred: f.auth,
Verf: rpc.AuthNull,
},
FH: f.fh,
Offset: f.curr,
Count: writeSize,
How: 2,
Contents: p[written : written+writeSize],
})

if err != nil {
util.Errorf("write(%x): %s", f.fh, err.Error())
return int(written), err
}

if err != nil {
util.Debugf("write(%x): %s", f.fh, err.Error())
return int(writeSize), err
}
writeres := &WriteRes{}
if err = xdr.Read(res, writeres); err != nil {
util.Errorf("write(%x) failed to parse result: %s", f.fh, err.Error())
util.Debugf("write(%x) partial result: %+v", f.fh, writeres)
return int(written), err
}

if writeres.Count != writeSize {
util.Debugf("write(%x) did not write full data payload: sent: %d, written: %d", writeSize, writeres.Count)
}

util.Debugf("write(%x) len=%d offset=%d written=%d total=%d",
f.fh, writeSize, f.curr, writeSize, totalToWrite)
f.curr += uint64(writeres.Count)
written += writeres.Count

f.curr = f.curr + writeSize
util.Debugf("write(%x) len=%d new_offset=%d written=%d total=%d", f.fh, totalToWrite, f.curr, writeres.Count, written)
}

return int(writeSize), nil
return int(written), nil
}

// Close commits the file
Expand Down Expand Up @@ -197,7 +216,7 @@ func (v *Target) Open(path string) (io.ReadCloser, error) {
return f, nil
}

func min(x, y uint64) uint64 {
func min(x, y uint32) uint32 {
if x > y {
return y
}
Expand Down
91 changes: 69 additions & 22 deletions nfs/nfs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,10 @@ const (
NFSProc3FSInfo = 19
NFSProc3Commit = 21

// The size in bytes of the opaque cookie verifier passed by
// READDIR and READDIRPLUS.
NFS3_COOKIEVERFSIZE = 8

// file types
NF3Reg = 1
NF3Dir = 2
Expand All @@ -51,19 +55,37 @@ type Sattr3 struct {
Mode SetMode
UID SetUID
GID SetUID
Size uint64
Atime NFS3Time
Mtime NFS3Time
Size SetSize
Atime SetTime
Mtime SetTime
}

type SetMode struct {
Set uint32
Mode uint32
SetIt bool `xdr:"union"`
Mode uint32 `xdr:"unioncase=1"`
}
Copy link
Copy Markdown

@rasky rasky Feb 11, 2018

Choose a reason for hiding this comment

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

Sorry for being late to the game, but all sequences of "isset bool + value" are described in the RFC as optional data type. This type supported using xdr:"optional" and a pointer type, so basically you can replace Mode SetMode with Mode *uint32 `xdr:"optional"` and then check Mode != nil to find out when Mode is set.

Copy link
Copy Markdown
Author

@hickeng hickeng Feb 12, 2018

Choose a reason for hiding this comment

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

@rasky Thanks for the review at all! This was a very targeted PR done with the NFS RFC in one hand, wireshark trace in the other. Does the same xdr:optional and pointer values apply for the multi-value unions as well, e.g. this struct?

@matthewavery If you know wireshark/delve can you pick this up and use it to teach those tools to @AngieCris and/or @anchal-agrawal. If not I'll run that with the three of you next time I'm in Austin.

Copy link
Copy Markdown

@rasky rasky Feb 13, 2018

Choose a reason for hiding this comment

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

@hickeng the PR is technically correct, I'm just suggesting a better usage of the Go XDR library. If you read RFC4506, there are two different data types: discriminated unions (§4.15) and optional types (§4.19).

Optional types are defined as a special case of discriminated unions, where the discriminator is a boolean type, and the FALSE case is empty; so it does make sense that you can use a discriminated union for those structures, it's just that the optional type is more easy to reason, lets you write less code, and more readable.

NFS RFC has many optional types, but it's true that they're described as XDR unions in the spec. I'm not sure why, must be some historical reason, but the two are really equivalent.

As for applying optional values to multi-field structs, it's sufficient to use the correct level of indirection:

type WccData struct {
	Before *struct {
		Size  uint64
		MTime NFS3Time
		CTime NFS3Time
	} `xdr:"optional"`
	After PostOpAttr
}

Notice that you need to change Before to be a pointer, in addition to adding the optional tag.


type SetUID struct {
Set uint32
UID uint32
SetIt bool `xdr:"union"`
UID uint32 `xdr:"unioncase=1"`
}

type SetSize struct {
SetIt bool `xdr:"union"`
Size uint64 `xdr:"unioncase=1"`
}

type TimeHow int

const (
DontChange TimeHow = iota
SetToServerTime
SetToClientTime
)

type SetTime struct {
SetIt TimeHow `xdr:"union"`
Time NFS3Time `xdr:"unioncase=2"` //SetToClientTime
}

type NFS3Time struct {
Expand Down Expand Up @@ -109,65 +131,81 @@ func (f *Fattr) Sys() interface{} {
return nil
}

type PostOpFH3 struct {
IsSet bool `xdr:"union"`
FH []byte `xdr:"unioncase=1"`
}

type PostOpAttr struct {
IsSet bool `xdr:"union"`
Attr Fattr `xdr:"unioncase=1"`
}

type EntryPlus struct {
FileId uint64
FileName string
Cookie uint64
Attr struct {
Follows uint32
Attr Fattr
}
FHSet uint32
FH []byte
ValueFollows uint32
Attr PostOpAttr
Handle PostOpFH3
// NextEntry *EntryPlus
}

func (e *EntryPlus) Name() string {
return e.FileName
}

func (e *EntryPlus) Size() int64 {
if e.Attr.Follows == 0 {
if !e.Attr.IsSet {
return 0
}

return e.Attr.Attr.Size()
}

func (e *EntryPlus) Mode() os.FileMode {
if e.Attr.Follows == 0 {
if !e.Attr.IsSet {
return 0
}

return e.Attr.Attr.Mode()
}

func (e *EntryPlus) ModTime() time.Time {
if e.Attr.Follows == 0 {
if !e.Attr.IsSet {
return time.Time{}
}

return e.Attr.Attr.ModTime()
}

func (e *EntryPlus) IsDir() bool {
if e.Attr.Follows == 0 {
if !e.Attr.IsSet {
return false
}

return e.Attr.Attr.IsDir()
}

func (e *EntryPlus) Sys() interface{} {
if e.Attr.Follows == 0 {
if !e.Attr.IsSet {
return 0
}

return e.FileId
}

type WccData struct {
Before struct {
IsSet bool `xdr:"union"`
Size uint64 `xdr:"unioncase=1"`
MTime NFS3Time `xdr:"unioncase=1"`
CTime NFS3Time `xdr:"unioncase=1"`
}
After PostOpAttr
}

type FSInfo struct {
Follows uint32
Attr PostOpAttr
RTMax uint32
RTPref uint32
RTMult uint32
Expand All @@ -184,6 +222,7 @@ type FSInfo struct {
func DialService(addr string, prog rpc.Mapping) (*rpc.Client, error) {
pm, err := rpc.DialPortmapper("tcp", addr)
if err != nil {
util.Errorf("Failed to connect to portmapper: %s", err)
return nil, err
}
defer pm.Close()
Expand All @@ -194,6 +233,9 @@ func DialService(addr string, prog rpc.Mapping) (*rpc.Client, error) {
}

client, err := dialService(addr, port)
if err != nil {
Copy link
Copy Markdown

@fdawg4l fdawg4l Oct 4, 2017

Choose a reason for hiding this comment

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

Wow, that's was pretty big bug... :-)

return nil, err
}

return client, nil
}
Expand Down Expand Up @@ -222,7 +264,10 @@ func dialService(addr string, port int) (*rpc.Client, error) {
Port: p,
}

client, err = rpc.DialTCP("tcp", ldr, fmt.Sprintf("%s:%d", addr, port))
raddr := fmt.Sprintf("%s:%d", addr, port)
util.Debugf("Connecting to %s", raddr)

client, err = rpc.DialTCP("tcp", ldr, raddr)
if err == nil {
break
}
Expand All @@ -236,8 +281,10 @@ func dialService(addr string, port int) (*rpc.Client, error) {

util.Debugf("using random port %d -> %d", p, port)
} else {
raddr := fmt.Sprintf("%s:%d", addr, port)
util.Debugf("Connecting to %s from unprivileged port", raddr)

client, err = rpc.DialTCP("tcp", ldr, fmt.Sprintf("%s:%d", addr, port))
client, err = rpc.DialTCP("tcp", ldr, raddr)
if err != nil {
return nil, err
}
Expand Down
Loading