Vladimir Homutov via nginx-devel
October 26, 2023 10:30AM
On Thu, Oct 26, 2023 at 03:08:55AM +0400, Sergey Kandaurov wrote:
> On Wed, Oct 11, 2023 at 04:58:47PM +0300, Vladimir Homutov via nginx-devel wrote:
[..]

> > diff --git a/src/event/quic/ngx_event_quic_output.c b/src/event/quic/ngx_event_quic_output.c
> > --- a/src/event/quic/ngx_event_quic_output.c
> > +++ b/src/event/quic/ngx_event_quic_output.c
> > @@ -563,8 +563,6 @@ ngx_quic_output_packet(ngx_connection_t
> > pkt.need_ack = 1;
> > }
> >
> > - ngx_quic_log_frame(c->log, f, 1);
> > -
> > flen = ngx_quic_create_frame(p, f);
> > if (flen == -1) {
> > return NGX_ERROR;
> > @@ -578,6 +576,8 @@ ngx_quic_output_packet(ngx_connection_t
> > f->last = now;
> > f->plen = 0;
> >
> > + ngx_quic_log_frame(c->log, f, 1);
> > +
> > nframes++;
>
> I'd rather move setting frame fields before calling
> ngx_quic_log_frame()/ngx_quic_create_frame()
> to preserve consistency with other places, i.e.:
> - set fields
> - log frame
> - create frame
>
> To look as follows:
>
> : f->pnum = ctx->pnum;
> : f->first = now;
> : f->last = now;
> : f->plen = 0;
> :
> : ngx_quic_log_frame(c->log, f, 1);
> :
> : flen = ngx_quic_create_frame(p, f);
> :

agreed

> > }
> >
> > @@ -925,6 +925,13 @@ ngx_quic_send_early_cc(ngx_connection_t
> >
> > res.data = dst;
> >
> > + ngx_log_debug7(NGX_LOG_DEBUG_EVENT, c->log, 0,
> > + "quic packet tx %s bytes:%ui need_ack:%d"
> > + " number:%L encoded nl:%d trunc:0x%xD frame:%ui]",
>
> typo: closing square bracket

thanks, removed

> Not sure we need logging for a (particular) frame in packet logging,
> not to say that it looks like a layering violation.
> Anyway, it is logged nearby, for example:
>
> quic frame tx init:0 CONNECTION_CLOSE err:11 invalid address validation token ft:0
> quic packet tx init bytes:36 need_ack:0 number:0 encoded nl:1 trunc:0x0
>
> So I'd remove this part.

agreed, frame logging removed

> > + ngx_quic_level_name(pkt.level), pkt.payload.len,
> > + pkt.need_ack, pkt.number, pkt.num_len, pkt.trunc,
> > + frame->type);
> > +
>
> BTW, it would make sense to get a new macro / inline function
> for packet tx logging, similar to ngx_quic_log_frame(),
> since we will have three places with identical ngx_log_debug7().

actually, four (we have also retry), so having a macro is a good idea

updated patch attached
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel
Subject Author Views Posted

[patch] quic PTO counter fixes

Vladimir Homutov via nginx-devel 381 October 11, 2023 10:00AM

Re: [patch] quic PTO counter fixes

Thorvaldur Thorvaldsson 82 October 15, 2023 03:50PM

Re: [patch] quic PTO counter fixes

Sergey Kandaurov 63 October 25, 2023 07:10PM

Re: [patch] quic PTO counter fixes

Vladimir Homutov via nginx-devel 59 October 26, 2023 10:30AM

Re: [patch] quic PTO counter fixes

Sergey Kandaurov 58 October 26, 2023 04:28PM

Re: [patch] quic PTO counter fixes

Vladimir Homutov via nginx-devel 51 October 26, 2023 04:38PM

Re: [patch] quic PTO counter fixes

Sergey Kandaurov 77 October 26, 2023 05:34PM

Re: [patch] quic PTO counter fixes

Vladimir Homutov via nginx-devel 57 November 09, 2023 10:16AM

Re: [patch] quic PTO counter fixes

Sergey Kandaurov 66 November 14, 2023 08:12AM



Sorry, you do not have permission to post/reply in this forum.

Online Users

Guests: 71
Record Number of Users: 8 on April 13, 2023
Record Number of Guests: 421 on December 02, 2018
Powered by nginx      Powered by FreeBSD      PHP Powered      Powered by MariaDB      ipv6 ready