credentials.TLSInfo: export State

[tamird]

See #111.

[iamqizhao]

I missed this in my previous PR. I actually tend to make state exported directly.

[tamird]

I think a getter makes more sense here; you don't want to allow mutation of this state.

[iamqizhao]

The getter does not prohibit mutation. It is the same thing compared to making it exported.

[tamird]

I'm pretty sure it does prohibit mutation; it returns a copy.

[iamqizhao]

On Tue, Sep 8, 2015 at 10:50 AM, Tamir Duberstein notifications@github.com
wrote:

In credentials/credentials.go
#328 (comment):

@@ -126,6 +126,10 @@ func (t TLSInfo) AuthType() string {
return "tls"
}

+func (t TLSInfo) ConnState() tls.ConnectionState {

I'm pretty sure it does prohibit mutation; it returns a copy.

Making it exported also makes a copy when users work on it (notice
tls.ConnectionState is a object not a pointer). Plus, making a copy does
NOT prohibit mutation because tls.ConnectionState contains a number of
pointer fields.

—
Reply to this email directly or view it on GitHub
https://github.com/grpc/grpc-go/pull/328/files#r38957015.

[iamqizhao]

On Tue, Sep 8, 2015 at 10:55 AM, Qi Zhao toqizhao@gmail.com wrote:

On Tue, Sep 8, 2015 at 10:50 AM, Tamir Duberstein <
notifications@github.com> wrote:

In credentials/credentials.go
#328 (comment):

@@ -126,6 +126,10 @@ func (t TLSInfo) AuthType() string {
return "tls"
}

+func (t TLSInfo) ConnState() tls.ConnectionState {

I'm pretty sure it does prohibit mutation; it returns a copy.

Making it exported also makes a copy when users work on it (notice
tls.ConnectionState is a object not a pointer). Plus, making a copy does
NOT prohibit mutation because tls.ConnectionState contains a number of
pointer fields.

Actually adding a getter might make the 2nd copy (depends on how the
compiler optimizes it) which is not needed.

—

Reply to this email directly or view it on GitHub
https://github.com/grpc/grpc-go/pull/328/files#r38957015.

[tamird]

Making it exported also makes a copy when users work on it (notice
tls.ConnectionState is a object not a pointer).

Doesn't matter that it's not a pointer. If it's exported I can overwrite it entirely: tlsInfo.State = myOtherTlsState.

Plus, making a copy does
NOT prohibit mutation because tls.ConnectionState contains a number of
pointer fields.

Yes, you could mess with some of the slices in tls.ConnectionState but it's still better than exporting this.

Actually adding a getter might make the 2nd copy (depends on how the
compiler optimizes it) which is not needed.

This doesn't matter - those copies are on the stack and the Go compiler does RVO.

[iamqizhao]

On Tue, Sep 8, 2015 at 11:00 AM, Tamir Duberstein notifications@github.com
wrote:

In credentials/credentials.go
#328 (comment):

@@ -126,6 +126,10 @@ func (t TLSInfo) AuthType() string {
return "tls"
}

+func (t TLSInfo) ConnState() tls.ConnectionState {

Making it exported also makes a copy when users work on it (notice
tls.ConnectionState is a object not a pointer).

Doesn't matter that it's not a pointer. If it's exported I can overwrite
it entirely: tlsInfo.State = myOtherTlsState.

I would say overwriting does not matter because you only overwrite your own
copy which is not the ConnectionState of the real tls connection (because
tls.ConnectionState returns a copy). I think that is the whole point why
tls.ConnectionState returns an object instead of a pointer. Keep in mind
that you are the service provider. I do not see any malicious operations
could happen here.

Plus, making a copy does
NOT prohibit mutation because tls.ConnectionState contains a number of
pointer fields.

Yes, you could mess with some of the slices in tls.ConnectionState
http://golang.org/pkg/crypto/tls/#ConnectionState but it's still better
than exporting this.

Slice elements are pointers.

Actually adding a getter might make the 2nd copy (depends on how the
compiler optimizes it) which is not needed.

This doesn't matter - those copies are on the stack and the Go compiler
does RVO.

—
Reply to this email directly or view it on GitHub
https://github.com/grpc/grpc-go/pull/328/files#r38958528.

[tamird]

Alright, I've exported it.

[iamqizhao]

LGTM, thanks for the fix.