Re: Patch proposal: New hooks in the connection path

(reposting the same review, with many grammatical fixes)

On Mon, Aug 8, 2022 at 3:51 AM Drouvot, Bertrand <bdrouvot@amazon.com> wrote:
> Please find attached v2-0004-connection_hooks.patch

     /*
      * Stop here if it was bad or a cancel packet.  ProcessStartupPacket
      * already did any appropriate error reporting.
      */
     if (status != STATUS_OK)
+    {
+#ifndef EXEC_BACKEND
+        if (FailedConnection_hook)
+            (*FailedConnection_hook) (FCET_BAD_STARTUP_PACKET, port);
+#endif
         proc_exit(0);
+    }

Per the comment above the if condition, the `status != OK` may
represent a cancel packet, as well. Clearly, a cancel packet is not
the same as a _bad_ packet. So I think here you need to differentiate
between a cancel packet and a genuinely bad packet; I don't see
anything good coming out of us, or the hook-developer, lumping
those 2 cases together.

I think we can reduce the number of places the hook is called, if we
call the hook from proc_exit(), and at all the other places we simply set
a global variable to signify the reason for the failure. The case of
_exit(1) from the signal-handler cannot use such a mechanism, but I
think all the other cases of interest can simply register one of the
FCET_* values, and let the call from proc_exit() pass that value
to the hook.

If we can convince ourselves that we can use proc_exit(1) in
StartupPacketTimeoutHandler(), instead of calling _exit(1), I think we
cal replace all call sites for this hook with the
set-global-variable variant.

> ...
> *     This should be the only function to call exit().
> *     -cim 2/6/90
>...
> proc_exit(int code)

The comment on proc_exit() claims that it should be the only place
calling exit(), except that the add-on/extension hooks may ignore this.
So there must be a strong reason why the signal-handler uses _exit()
to bypass all callbacks.

Best regards,
Gurjeet
http://Gurje.et