Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

netlink: add AttributeEncoder type #95

Merged
merged 1 commit into from Sep 7, 2018

Conversation

Projects
None yet
4 participants
@terinjokes
Copy link
Contributor

commented Sep 5, 2018

The AttributeDecoder type makes it easy to quickly and safely decode
fields from netlink messages. Users, however, were left to use the
low-level functions to encode messages.

This CL introduces the AttributeEncoder type, which parallels the
AttributeDecoder API, making it just as easy to encode netlink messages,
including nested attributes.

@mdlayher
Copy link
Owner

left a comment

Looking great overall, just a few nits.

/cc @stapelberg

Show resolved Hide resolved attribute.go Outdated
Show resolved Hide resolved example_test.go Outdated
Show resolved Hide resolved example_test.go Outdated
Show resolved Hide resolved example_test.go Outdated
Show resolved Hide resolved attribute.go Outdated
//
// Errors from intermediate encoding steps are returned in the call to
// MarshalBinary.
type AttributeEncoder struct {

This comment has been minimized.

Copy link
@mdlayher

mdlayher Sep 5, 2018

Owner

Do you think it makes sense to expose func (ae *AttributeEncoder) Attributes() ([]netlink.Attribute, error) to get the attribute stack out, in addition to or instead of returning bytes directly?

This comment has been minimized.

Copy link
@terinjokes

terinjokes Sep 5, 2018

Author Contributor

I have no preference. I have no need for this, so didn't include it.

This comment has been minimized.

Copy link
@terinjokes

terinjokes Sep 5, 2018

Author Contributor

I still haven't included this. Is there a specific case somewhere that needs this?

@stapelberg

This comment has been minimized.

Copy link
Contributor

commented Sep 5, 2018

Let me know if you want me to give this a go over in the google/nftables package once you’ve reviewed it.

@mdlayher

This comment has been minimized.

Copy link
Owner

commented Sep 5, 2018

I made a couple of changes to this patch and decided to give it a go in wireguardctrl:

WireGuard/wgctrl-go@4a8c868

So far so good! It all seems to come together nicely. I made a few tweaks that I think aid in the ergonomics of the type, and did a few doc updates:

https://gist.github.com/mdlayher/e911bfca231e4c8c339263b480aa6f6e

Curious to hear what you both think. It seems useful to me to be able to easily embed a byte slice in attributes (common for things like IPs), and I also had a use case where I wanted the Go slice directly instead of the encoded attributes.

@terinjokes

This comment has been minimized.

Copy link
Contributor Author

commented Sep 5, 2018

@mdlayher What I had at community day also had a Bytes type. I was hesitant in including here, since it's a trivial adapter for ae.Do, and there's a naming conflict with nlenc.Bytes

netlink: add AttributeEncoder type
The AttributeDecoder type makes it easy to quickly and safely decode
fields from netlink messages. Users, however, were left to use the
low-level functions to encode messages.

This CL introduces the AttributeEncoder type, which parallels the
AttributeDecoder API, making it just as easy to encode netlink messages,
including nested attributes.

Bug: #91
@mdlayher

This comment has been minimized.

Copy link
Owner

commented Sep 5, 2018

Since this code lives in netlink and not nlenc, it ought to be just fine. I agree that you can do it easily with a small adapter, but it seems reasonable to me to offer the method directly, because otherwise I'll probably write the same helper in my code repeatedly for use with Do. It's nice and concise with a Bytes method, and general enough to make sense for many callers IMO.

@terinjokes terinjokes force-pushed the terinjokes:patches/attribute-encoder branch 3 times, most recently from cc41aaf to 99e7067 Sep 5, 2018

@terinjokes terinjokes force-pushed the terinjokes:patches/attribute-encoder branch from 99e7067 to 4487591 Sep 5, 2018

@terinjokes

This comment has been minimized.

Copy link
Contributor Author

commented Sep 5, 2018

It's nice and concise with a Bytes method, and general enough to make sense for many callers IMO.

I'd argue the same for AttributesDecoder, as many of my ad.Do calls are to a function to copy bytes. I can open an issue.

Show resolved Hide resolved attribute.go Outdated
Show resolved Hide resolved attribute.go Outdated
Show resolved Hide resolved attribute.go Outdated
Show resolved Hide resolved attribute.go Outdated
// Do is a general purpose function to encode arbitrary data into a field.
//
// Do is especially helpful in encoding nested attributes, attribute arrays,
// or encoding arbitrary types which don't fit cleanly into an unsigned integer

This comment has been minimized.

Copy link
@mdlayher

mdlayher Sep 5, 2018

Owner

I liked the "arbitrary types (such as C structures)" disclaimer I had on the decoder too, since it's a fairly common use case for netlink. Mind adding that statement?

Show resolved Hide resolved attribute.go Outdated
Show resolved Hide resolved example_attributeencoder_test.go Outdated
@mdlayher

This comment has been minimized.

Copy link
Owner

commented Sep 5, 2018

Yeah the bytes thing is tricky for decoding. I suppose I'm not entirely against making a function that makes a copy of the bytes and returns it to the caller on the decoder. Please do file an issue if you agree.

@terinjokes terinjokes force-pushed the terinjokes:patches/attribute-encoder branch 3 times, most recently from e10f3fa to cde95b2 Sep 6, 2018

terinjokes added a commit to terinjokes/netlink that referenced this pull request Sep 6, 2018

netlink: add AttributeEncoder type
The AttributeDecoder type makes it easy to quickly and safely decode
fields from netlink messages. Users, however, were left to use the
low-level functions to encode messages.

This CL introduces the AttributeEncoder type, which parallels the
AttributeDecoder API, making it just as easy to encode netlink messages,
including nested attributes.

Bug: mdlayher#95

@terinjokes terinjokes force-pushed the terinjokes:patches/attribute-encoder branch from cde95b2 to 8d9c801 Sep 6, 2018

@mdlayher

This comment has been minimized.

Copy link
Owner

commented Sep 7, 2018

LGTM thanks!

@mdlayher mdlayher merged commit b428678 into mdlayher:master Sep 7, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@elliotmr

This comment has been minimized.

Copy link

commented Sep 7, 2018

Absolute first impression on using the library: could we use "type" and "value" as the argument names?

The usage is clear inside the function, but for documentation and auto-completion it would be better.

@terinjokes terinjokes referenced this pull request Sep 7, 2018

Closed

AttributeDecoder.Bytes #103

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.