[PATCH 1 of 8] QUIC: treat qc->error == -1 as a missing error

[Roman Arutyunyan]

On Tue, Aug 02, 2022 at 05:48:56PM +0400, Sergey Kandaurov wrote:
> On Thu, Jun 23, 2022 at 07:58:39PM +0400, Roman Arutyunyan wrote:
> > # HG changeset patch
> > # User Roman Arutyunyan <arut at nginx.com>
> > # Date 1655889315 -14400
> > #      Wed Jun 22 13:15:15 2022 +0400
> > # Branch quic
> > # Node ID 951d7116f37dc39d9eba20ceae49434592ce4677
> > # Parent  5b1011b5702b5c5db2ba3d392a4da25596183cc2
> > QUIC: treat qc->error == -1 as a missing error.
> > 
> > Previously, zero was used for this purpose.  However, NGX_QUIC_ERR_NO_ERROR is
> > zero too.  As a result, NGX_QUIC_ERR_NO_ERROR was changed to
> > NGX_QUIC_ERR_INTERNAL_ERROR when closing a QUIC connection.
> > 
> > diff --git a/src/event/quic/ngx_event_quic.c b/src/event/quic/ngx_event_quic.c
> > --- a/src/event/quic/ngx_event_quic.c
> > +++ b/src/event/quic/ngx_event_quic.c
> > @@ -317,6 +317,8 @@ ngx_quic_new_connection(ngx_connection_t
> >      qc->congestion.ssthresh = (size_t) -1;
> >      qc->congestion.recovery_start = ngx_current_msec;
> >  
> > +    qc->error = (ngx_uint_t) -1;
> > +
> >      if (pkt->validated && pkt->retried) {
> >          qc->tp.retry_scid.len = pkt->dcid.len;
> >          qc->tp.retry_scid.data = ngx_pstrdup(c->pool, &pkt->dcid);
> > @@ -523,7 +525,7 @@ ngx_quic_close_connection(ngx_connection
> >                  qc->error = NGX_QUIC_ERR_NO_ERROR;
> >  
> >              } else {
> > -                if (qc->error == 0 && !qc->error_app) {
> > +                if (qc->error == (ngx_uint_t) -1 && !qc->error_app) {
> >                      qc->error = NGX_QUIC_ERR_INTERNAL_ERROR;
> >                  }
> >  
> 
> A former initialization is in ngx_quic_handle_payload().
> Leaving it there makes connection close on the error path, for example,
> on ngx_quic_set_path() failure, with a wrong NO_ERROR code.
> 
> I'd just leave (a new value of) initialization in place:
> ngx_quic_new_connection() gives an established quic connection or nothing,
> it cannot fail with qc initialized. ngx_quic_handle_payload() is called
> right after, and is a point of packet decryption, which implies qc->error
> reset.  This makes it a suitable place.

You're right, let's leave the initializaion where it is now, but with a new
value.