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