On Thu, Aug 26, 2021 at 9:13 AM Jacob Champion <pchamp...@vmware.com> wrote:
> On Wed, 2021-08-25 at 15:25 -0700, Zhihong Yu wrote: > > > > Hi, > > For v2-0001-common-jsonapi-support-FRONTEND-clients.patch : > > > > + /* Clean up. */ > > + termJsonLexContext(&lex); > > > > At the end of termJsonLexContext(), empty is copied to lex. For stack > > based JsonLexContext, the copy seems unnecessary. > > Maybe introduce a boolean parameter for termJsonLexContext() to > > signal that the copy can be omitted ? > > Do you mean heap-based? i.e. destroyJsonLexContext() does an > unnecessary copy before free? Yeah, in that case it's not super useful, > but I think I'd want some evidence that the performance hit matters > before optimizing it. > > Are there any other internal APIs that take a boolean parameter like > that? If not, I think we'd probably just want to remove the copy > entirely if it's a problem. > > > +#ifdef FRONTEND > > + /* make sure initialization succeeded */ > > + if (lex->strval == NULL) > > + return JSON_OUT_OF_MEMORY; > > > > Should PQExpBufferBroken(lex->strval) be used for the check ? > > It should be okay to continue if the strval is broken but non-NULL, > since it's about to be reset. That has the fringe benefit of allowing > the function to go as far as possible without failing, though that's > probably a pretty weak justification. > > In practice, do you think that the probability of success is low enough > that we should just short-circuit and be done with it? > > On Wed, 2021-08-25 at 16:24 -0700, Zhihong Yu wrote: > > > > For v2-0002-libpq-add-OAUTHBEARER-SASL-mechanism.patch : > > > > + i_init_session(&session); > > + > > + if (!conn->oauth_client_id) > > + { > > + /* We can't talk to a server without a client identifier. */ > > + appendPQExpBufferStr(&conn->errorMessage, > > + libpq_gettext("no oauth_client_id is set > for the connection")); > > + goto cleanup; > > > > Can conn->oauth_client_id check be performed ahead > > of i_init_session() ? That way, ```goto cleanup``` can be replaced > > with return. > > Yeah, I think that makes sense. FYI, this is probably one of the > functions that will be rewritten completely once iddawc is removed. > > > + if (!error_code || (strcmp(error_code, "authorization_pending") > > + && strcmp(error_code, "slow_down"))) > > > > What if, in the future, there is error code different from the above > > two which doesn't represent "OAuth token retrieval failed" scenario ? > > We'd have to update our code; that would be a breaking change to the > Device Authorization spec. Here's what it says today [1]: > > The "authorization_pending" and "slow_down" error codes define > particularly unique behavior, as they indicate that the OAuth client > should continue to poll the token endpoint by repeating the token > request (implementing the precise behavior defined above). If the > client receives an error response with any other error code, it MUST > stop polling and SHOULD react accordingly, for example, by displaying > an error to the user. > > > For client_initial_response(), > > > > + token_buf = createPQExpBuffer(); > > + if (!token_buf) > > + goto cleanup; > > > > If token_buf is NULL, there doesn't seem to be anything to free. We > > can return directly. > > That's true today, but implementations have a habit of changing. I > personally prefer not to introduce too many exit points from a function > that's already using goto. In my experience, that makes future > maintenance harder. > > Thanks for the reviews! Have you been able to give the patchset a try > with an OAuth deployment? > > --Jacob > > [1] https://datatracker.ietf.org/doc/html/rfc8628#section-3.5 Hi, bq. destroyJsonLexContext() does an unnecessary copy before free? Yeah, in that case it's not super useful, but I think I'd want some evidence that the performance hit matters before optimizing it. Yes I agree. bq. In practice, do you think that the probability of success is low enough that we should just short-circuit and be done with it? Haven't had a chance to try your patches out yet. I will leave this to people who are more familiar with OAuth implementation(s). bq. I personally prefer not to introduce too many exit points from a function that's already using goto. Fair enough. Cheers