Re: Patch proposal: New hooks in the connection path

Hi,

On 8/14/22 7:52 AM, Gurjeet Singh wrote:
> 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.

Thanks for the feedback!

Yeah, good point. I agree that it would be better to make the distinction.

> 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.

That looks like a good idea to me. I'm tempted to rewrite the patch that 
way (and addressing the first comment in the same time).

Curious to hear about others hackers thoughts too.

> 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.

I can see why this is not safe in the EXEC_BACKEND case, but it's not 
clear to me why it would not be safe in the non EXEC_BACKEND case.

>> ...
>> *     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.

yeah.

Regards,

-- 

Bertrand Drouvot
Amazon Web Services: https://aws.amazon.com