Re: Improving psql's \password command

Thanks for the expeditious review.

On 11/15/21, 10:13 AM, "Tom Lane" <tgl@sss.pgh.pa.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