gRFC Proposal : Addition of DebugString API in CallCredentials

[Moiz Haidry]

Hello all, 

I have posted a new gRFC at https://github.com/grpc/proposal/pull/171. This is an effort to add more debugging capability to the gRPC core surface API.  I'd appreciate any comments that you may have regarding this topic and how it might affect you.

Regards,

@mhaidrygoog

[Christopher Warrington - MSFT]

On Wednesday, February 19, 2020 at 9:40:14 AM UTC-8, Moiz Haidry wrote:

> This is an effort to add more debugging capability to the gRPC core
> surface API.

During the PR, Vijay Pai mentioned [1] that the addition of the debug_string
member to the grpc_metadata_credentials_plugin is an API breaking change.
It's likely to affect callers who fail to initialize their automatic storage
duration grpc_metadata_credentials_plugin instances.

    // Callers like this will be unhappy: .debug_string will have an indeterminate value
    grpc_metadata_credentials_plugin plugin;
    plugin.get_metadata = &gm;
    plugin.destroy = &d;
    plugin.state = 42;
    plugin.type = "my cool plugin";
    
    creds = grpc_metadata_credentials_create_from_plugin(plugin, ...);

    // Callers like this will be fine, as .debug_string will be initialized to a null pointer
    grpc_metadata_credentials_plugin plugin2 = {
      .get_metadata = &gm,
      .destroy = &d,
      .state = 42,
      .type = "my cool plugin2",
    };

    creds = grpc_metadata_credentials_create_from_plugin(plugin2, ...);

Does that need to be captured in the proposal anywhere?

[1] https://github.com/grpc/grpc/pull/21984#discussion_r384061238

--
Christopher Warrington
Microsoft Corp.

[Moiz Haidry]

Hi Christopher,

There is a note in the proposal under grpc plugin credentials mentioning that this a breaking API change. Please let me know if there needs to be more details provided.

Thanks,

Moiz

--
You received this message because you are subscribed to a topic in the Google Groups "grpc.io" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/grpc-io/eqA-G6eLE5Y/unsubscribe.
To unsubscribe from this group and all its topics, send an email to grpc-io+u...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/grpc-io/11b3c6c8-6999-4ed5-8a6d-31a847c46414%40googlegroups.com.

--

Thanks,

Moiz

[Christopher Warrington - MSFT]

On Tuesday, March 10, 2020 at 1:33:52 PM UTC-7, Moiz Haidry wrote:

MH> There is a note in the proposal under grpc plugin credentials mentioning
MH> that this a breaking API change. Please let me know if there needs to be
MH> more details provided.

The proposal has this:

> The caller has to initialize the method if they plan to use the
> debug_string method.

This note looks pretty good to me, though I'd reword this so that there’s no
implication that it's OK for the field to be uninitialized. How about this?

"The caller must initialize this field to either NULL or an implementation
of the debug_string callback."

While the current implementation may be fine with an uninitialized field, if
I remember by C undefined behavior rules correctly, I think that operations
like copying the struct may result in undefined behavior should debug_string
remain uninitialized.

[Moiz Haidry]

Sounds good. I'll follow up with another PR with those corrections. Thanks for the review.