Add support for discriminated unions and optional data #6

rasky · 2015-09-13T20:43:18Z

This PR adds support for:

Discriminated unions (RFC 4506, 4.15)
Optional data (RFC 4506, 4.19)

In both cases, the code relies on struct tags to convey the required metadata information. Please see the individual commits for more information.

davecgh · 2015-09-16T19:55:14Z

Thanks for the PR. I'll review it this weekend.

rasky · 2015-09-22T07:38:54Z

ping :)

rasky · 2015-10-24T11:00:17Z

Hi, any chance to get this reviewed? I need to release libraries based on this, and I would like to avoid a permanent fork if possible at all.

jhbe · 2015-11-28T02:55:40Z

Ping. :)

This is just what I need, but like rasky I'd rather use the main repo if possible.

rasky · 2016-08-14T10:58:04Z

Ping again on this :) I've been using my fork with success in production, I think the modifications in this PR are correct. If you're concerned with styling, let me know.

marcopeereboom · 2016-11-23T17:16:17Z

Can you redo this PR and I'll push for it to go in.

prashanthpai · 2017-01-04T09:18:18Z

Thanks @rasky for working on this. I'd love to see this PR land in the main repo. What is it that needs to be done here to get this in ?

Thanks.

The usage of absolute import paths in testsuite is not compatible with allowing a package to easily fork. One has to replace all global paths in test files. I tried to use relative import paths such as "../xdr2" but there appears to be a Go 1.5 bug for which the internal_test.go functions TstEncode/TstDecode are not exposed to tests.

Unions are represented by normal Go structs, using different struct tags to pinpoint the union discriminant, and to select the various cases. Since more struct tags were required, I introduced a new general tag called "xdr" with key/value options, and thus changed the previous "xdropaque" struct tag into the new canonical name form `xdr:"opaque=false"`, while retaining the previous form for backward compatibility.

In Go, they are expressed through indirection: a pointer to the actualy data, that can be nil to express that the field is missing. The field must be marked with the new tag option "optional", because otherwise it is automatically indirected as per default behaviour of marshallers.

rasky · 2017-01-24T16:48:17Z

Hi, I've rebased the PR and also completed coverage (100%).

I would appreciate if you could merge this PR @marcopeereboom @davecgh

marcopeereboom

I am mostly ok with this diff. Some minor nits.

@davecgh can you have a peek too?

marcopeereboom · 2017-02-03T15:44:09Z

xdr2/bench_test.go

@@ -14,14 +14,12 @@
 * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
 */

-package xdr_test
+package xdr


I am sure @davecgh has a reason to split test from package. Seems like a gratuitous change.

The point is that this style of testing makes forking impossible, because of the absolute import path used to import the xdr2 package. And unfortunately, you can't use relative imports in this context.

So for instance, the CI would be unable to test this pull request because it would use the upstream version of the xdr2 package, rather than the version in this branch.

marcopeereboom · 2017-02-03T15:46:32Z

xdr2/tag_test.go

@@ -0,0 +1,45 @@
+/*
+ * Copyright (c) 2012-2014 Dave Collins <[email protected]>


Pretty sure @davecgh didn't write this :-) Date is wrong to boot.

marcopeereboom · 2017-02-03T15:47:38Z

xdr2/tag.go

@@ -0,0 +1,73 @@
+/*
+ * Copyright (c) 2012-2014 Dave Collins <[email protected]>


Wrong author and date.

marcopeereboom · 2017-02-03T15:49:12Z

xdr2/tag_test.go

@@ -0,0 +1,45 @@
+/*
+ * Copyright (c) 2012-2014 Dave Collins <[email protected]>


Wrong author and date.

rasky · 2017-02-17T17:24:43Z

I've updated the copyrights.

dougm · 2017-02-21T20:18:53Z

👍 ran into the need for this recently, would be great to have it merged. cc @fdawg4l

marcopeereboom · 2017-02-21T20:52:51Z

I spoke to @davecgh about this and we want it if it becomes xdr3. xdr2 API is set in stone and we don't want to modify it.

rasky · 2017-02-21T21:18:48Z

Can you please clarify how the API is modified? AFAICT it's retro compatible in pure go 1 sense

vtolstov · 2017-04-21T15:52:22Z

any news? I need xdr3 package and i need to distinglish between string and string pointer.
Because I'm decode libvirt rpc data and:

 There are basically two types of strings when it comes to XDR. 
The first one is nonnull_string, which are encoded [length][string]. 
With just this it would be impossible to differentiate between NULL 
and an empty string "" as both would be encoded in the same way. 
Therefore, there is remote_string (libvirt terminology) or 
xdr_pointer (XDR terminology) which encodes a pointer as [bool][pointer] where 
bool is either 00 00 00 00 meaning the pointer is NULL or something else (e.g. 00 00 00 01) meaning the pointer has a non-NULL value.

marcopeereboom · 2017-04-26T13:04:27Z

I will look at this soon. Remind me again if I forget.

vtolstov · 2017-10-25T17:14:45Z

@marcopeereboom ping...

vtolstov · 2017-10-25T17:21:45Z

@davecgh can you merge this on create xdr3 ? I'm really need this pr and it sit in this repo 2 years...

rasky · 2017-10-25T17:55:40Z

I still don’t see why xdr3 is needed for a perfectly backward compatible PR.

vtolstov · 2017-10-26T15:30:58Z

ok @davecgh please merge it, or if you don't interests in this package add unmaintained info to readme .
And all who need new features can use rasky repo

Commit 7592fd7 makes use of the `xdr:"union"` and `xdr:"unioncase=N"` annotations that were submitted in a pull request that has not been merged upstream to davecgh/go-xdr: davecgh/go-xdr#6 Without support for these annotations, the code crashes at runtime. Signed-off-by: Anders Kaseorg <[email protected]>

Commit 7592fd7 makes use of the `xdr:"union"` and `xdr:"unioncase=N"` annotations that were submitted in a pull request that has not been merged upstream to davecgh/go-xdr: davecgh/go-xdr#6 Without support for these annotations, the code is unable to parse a valid FSINFO reply. Signed-off-by: Anders Kaseorg <[email protected]>

andrewrk · 2018-03-27T16:23:08Z

bumpity humpidy dump

rasky added 6 commits January 24, 2017 17:29

Add support for bool type in discriminant

19be307

Document optional-data

382fb42

Complete test coverage

1a41d1a

rasky force-pushed the improvements branch from 0c28d85 to 1a41d1a Compare January 24, 2017 16:30

Add test for tag (and remove spurious code)

d3abeac

marcopeereboom suggested changes Feb 3, 2017

View reviewed changes

Updated copyright

4930550

dougm mentioned this pull request Feb 28, 2017

toolbox: Add guest NicInfo RPC vmware/vic#4057

Merged

This was referenced Jan 19, 2018

Depend on a version of go-xdr with working discriminated unions vmware-archive/go-nfs-client#6

Merged

Support discriminated unions correctly vmware-archive/go-nfs-client#2

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for discriminated unions and optional data #6

Add support for discriminated unions and optional data #6

rasky commented Sep 13, 2015

davecgh commented Sep 16, 2015

rasky commented Sep 22, 2015

rasky commented Oct 24, 2015

jhbe commented Nov 28, 2015

rasky commented Aug 14, 2016

marcopeereboom commented Nov 23, 2016

prashanthpai commented Jan 4, 2017

rasky commented Jan 24, 2017

marcopeereboom left a comment

marcopeereboom Feb 3, 2017

rasky Feb 17, 2017

marcopeereboom Feb 3, 2017

marcopeereboom Feb 3, 2017

marcopeereboom Feb 3, 2017

rasky commented Feb 17, 2017

dougm commented Feb 21, 2017

marcopeereboom commented Feb 21, 2017

rasky commented Feb 21, 2017

vtolstov commented Apr 21, 2017 •

edited

Loading

marcopeereboom commented Apr 26, 2017

vtolstov commented Oct 25, 2017

vtolstov commented Oct 25, 2017

rasky commented Oct 25, 2017

vtolstov commented Oct 26, 2017

andrewrk commented Mar 27, 2018

		@@ -0,0 +1,45 @@
		/*
		* Copyright (c) 2012-2014 Dave Collins <[email protected]>

		@@ -0,0 +1,73 @@
		/*
		* Copyright (c) 2012-2014 Dave Collins <[email protected]>

Add support for discriminated unions and optional data #6

Are you sure you want to change the base?

Add support for discriminated unions and optional data #6

Conversation

rasky commented Sep 13, 2015

davecgh commented Sep 16, 2015

rasky commented Sep 22, 2015

rasky commented Oct 24, 2015

jhbe commented Nov 28, 2015

rasky commented Aug 14, 2016

marcopeereboom commented Nov 23, 2016

prashanthpai commented Jan 4, 2017

rasky commented Jan 24, 2017

marcopeereboom left a comment

Choose a reason for hiding this comment

marcopeereboom Feb 3, 2017

Choose a reason for hiding this comment

rasky Feb 17, 2017

Choose a reason for hiding this comment

marcopeereboom Feb 3, 2017

Choose a reason for hiding this comment

marcopeereboom Feb 3, 2017

Choose a reason for hiding this comment

marcopeereboom Feb 3, 2017

Choose a reason for hiding this comment

rasky commented Feb 17, 2017

dougm commented Feb 21, 2017

marcopeereboom commented Feb 21, 2017

rasky commented Feb 21, 2017

vtolstov commented Apr 21, 2017 • edited Loading

marcopeereboom commented Apr 26, 2017

vtolstov commented Oct 25, 2017

vtolstov commented Oct 25, 2017

rasky commented Oct 25, 2017

vtolstov commented Oct 26, 2017

andrewrk commented Mar 27, 2018

vtolstov commented Apr 21, 2017 •

edited

Loading