Support discriminated unions correctly #2

hickeng · 2017-10-04T16:15:21Z

This makes use of the discriminated union support in the xdr2 package,
and uses that to correct several protocol errors in the previous code.

This client is still woefully incomplete and likely does not handle
various corner cases correctly.

Basic testing performed against Linux kernel NFS server and NetApp
simulator.

fdawg4l · 2017-10-04T16:26:21Z

nfs/file.go

-		Attrs   struct {
-			Attrs Fattr
-		}
+		Attr  PostOpAttr


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

fdawg! great to hear from you :)

@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.

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

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

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.

fdawg4l · 2017-10-04T16:27:52Z

nfs/nfs.go

@@ -194,6 +233,9 @@ func DialService(addr string, prog rpc.Mapping) (*rpc.Client, error) {
 	}

 	client, err := dialService(addr, port)
+	if err != nil {


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

fdawg4l · 2017-10-04T16:31:57Z

nfs/target.go

+			},
+			FH:         fh,
+			Cookie:     cookie,
+			CookieVerf: cookieVerf,


So was Cookie and CookieVerf the issue with the NTAP box? I don't see how the union handling change wouldve fixed the issue.

Cookie/CookieVerf were preventing us from getting more than 1 directory entry back. I'm not sure why but the first return always seems to be for . and then you get the rest. Could be related to the FSInfo.dtpref value, but I couldn't find any expansion on what the preferred multipliers were intended for so left it as is.

The union values were causing most of the problems - for example FSInfo was getting populated with incorrect data because we didn't have the optional FAttr structure - that led to the wtpref size being 0 so we wrote 0 bytes at a time, got a short write and then died.

Gah. Good point - I'll need to add handling to Write to deal with a wtmax being lower than the data to write.

fdawg4l · 2017-10-04T16:37:56Z

nfs/rpc/client.go

@@ -68,11 +72,14 @@ type message struct {
 }

 func (c *Client) Call(call interface{}) (io.ReadSeeker, error) {
+	retries := 1


This can get hairy. It's semantically correct, but I was afraid we'd retry atomic operations when really the unmarshaller was off by a few bytes or something nonsensical like that. Anyway, just providing color.

Yeah - I don't really like it but if I don't then we won't work with certain IBM storage appliances - the actual docs for that appliance contain more details, but relies on the retry.
Saying "we're not using the Linux client" gets awkward as a configuration input.

fdawg4l

Cool find. The union handling I suspect wasnt the issue and adds a lot of churn, but the cleanup looks useful. Probably good to call out the cookie handling (i suspect that's the cause) as the bug.

Yes, "woefully" incomplete. I'm planning on using this project in the future and will add some methods as they are needed. Probably good to add issues for the ones that are missing.

hickeng · 2017-10-04T18:46:36Z

@fdawg4l At some point we'll need to add enough support to this to allow for docker cp to/from nfs volumes - as such we'll need most of the rest of the capabilities (definitely symlinks) and a filewalker style interface that's shared with os.File, et al, so we can share the diff'ing logic between remote and local use.

fdawg4l · 2017-10-04T19:40:54Z

@hickeng

docker cp [OPTIONS] CONTAINER:SRC_PATH DEST_PATH|-
docker cp [OPTIONS] SRC_PATH|- CONTAINER:DEST_PATH

To implement docker cp, you might as well use guest tools for that rather than the VCH so theres only one code path for any filesystem type.

This makes use of the discriminated union support in the xdr2 package, and uses that to correct several protocol errors in the previous code. This client is still woefully incomplete and likely does not handle various corner cases correctly. Basic testing performed against Linux kernel NFS server and NetApp simulator.

hickeng · 2017-10-04T20:15:59Z

@fdawg4l We do when we're copying to a running container, however when copying to a stopped container we have to do it on the appliance where the same VFS issues apply as for the volume store prep. For now we've punted and said that NFS volumes are just not processed for offline containers.
Also, updated the file.go:Write method for correct handling of chunking and force pushed.

fdawg4l · 2017-10-04T20:21:21Z

Write looks fine. Good work!

andersk · 2018-01-19T02:50:19Z

This doesn’t work with the declared go-xdr dependency because it requires an unmerged pull request (davecgh/go-xdr#6). See #6.

rasky · 2018-02-11T12:12:14Z

nfs/nfs.go

-	Set  uint32
-	Mode uint32
+	SetIt bool   `xdr:"union"`
+	Mode  uint32 `xdr:"unioncase=1"`
 }


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.

@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.

@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.

cache all entries of a directory

mdubya66 approved these changes Oct 4, 2017

View reviewed changes

hickeng mentioned this pull request Oct 4, 2017

Incorrect marshalling of XDR data for NFS mkdir and create file operations vmware/vic#6500

Closed

fdawg4l reviewed Oct 4, 2017

View reviewed changes

fdawg4l approved these changes Oct 4, 2017

View reviewed changes

hickeng force-pushed the discriminated-union-fixes branch from 964a7a4 to 5173af6 Compare October 4, 2017 20:11

hickeng merged commit 7592fd7 into vmware-archive:master Oct 4, 2017

rasky reviewed Feb 11, 2018

View reviewed changes

willscott referenced this pull request in willscott/go-nfs-client Jan 3, 2024

Merge pull request #2 from juicedata/wip-cache-dir

b997a81

cache all entries of a directory

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support discriminated unions correctly #2

Support discriminated unions correctly #2

hickeng commented Oct 4, 2017

fdawg4l Oct 4, 2017

mdubya66 Oct 4, 2017

hickeng Oct 4, 2017

fdawg4l Oct 4, 2017

hickeng Oct 4, 2017

fdawg4l Oct 4, 2017

fdawg4l Oct 4, 2017

fdawg4l Oct 4, 2017

hickeng Oct 4, 2017

hickeng Oct 4, 2017

fdawg4l Oct 4, 2017

hickeng Oct 4, 2017

fdawg4l left a comment

hickeng commented Oct 4, 2017

fdawg4l commented Oct 4, 2017

hickeng commented Oct 4, 2017 •

edited

Loading

fdawg4l commented Oct 4, 2017

andersk commented Jan 19, 2018 •

edited

Loading

rasky Feb 11, 2018

hickeng Feb 12, 2018

rasky Feb 13, 2018

Support discriminated unions correctly #2

Support discriminated unions correctly #2

Conversation

hickeng commented Oct 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fdawg4l left a comment

Choose a reason for hiding this comment

hickeng commented Oct 4, 2017

fdawg4l commented Oct 4, 2017

hickeng commented Oct 4, 2017 • edited Loading

fdawg4l commented Oct 4, 2017

andersk commented Jan 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hickeng commented Oct 4, 2017 •

edited

Loading

andersk commented Jan 19, 2018 •

edited

Loading