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