Re: Improving psql's \password command

Thanks for the expeditious review.

On 11/15/21, 10:13 AM, "Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Hm. It's not as bad as I thought it might be, but I still dislike
> importing <setjmp.h> into common/string.h --- that seems like a mighty
> ugly dependency to have there. I guess one idea to avoid that is to
> declare SigintInterruptContext.jmpbuf as "void *". Alternatively we
> could push those function declarations into some specialized header.

I used "void *" for v2.

> * API spec for SigintInterruptContext needs to be a bit more detailed.
> Maybe "... via an existing SIGINT signal handler that will longjmp to
> the specified place, but only when *enabled is true".

Done.

> * I don't believe that this bit is necessary, or if it is,
> the comment fails to justify it:
>
> - initStringInfo(&buf);
> + /* make sure buf is palloc'd so we don't lose changes after a longjmp */
> + buf = makeStringInfo();

My main worry was that buf->data might get repalloc'd via
enlargeStringInfo(), which could cause problems after a longjmp. We
could also declare it as volatile, but I think you'd unfortunately
have to unvolatize() a bunch.

> * You're failing to re-enable sigint_ctx->enabled when looping
> around for another fgets call.

Oops, fixed.

> * Personally I'd write those assignments like
>
> *(sigint_ctx->enabled) = true;
>
> as that seems clearer.

Done.

Nathan