[PATCH 03 of 14] HTTP/2: add debug logging of control frames

[Valentin V. Bartenev]

On Wednesday 05 July 2017 03:02:55 Piotr Sikora via nginx-devel wrote:
> Hey Valentin,
> comments below. I commented only on the first occurrence, but
> obviously those comments apply to all similar changes.
> 
> >          ngx_log_debug0(NGX_LOG_DEBUG_HTTP, h2c->connection->log, 0,
> > -                       "http2 SETTINGS frame ack:1");
> > +                       "http2 SETTINGS ACK frame");
> 
> I don't particularly like this change, since it makes it look like
> this is a "SETTINGS ACK" frame, not a "SETTINGS" frame with ACK flag
> set. This is even more confusing since QUIC drafts had "SETTINGS_ACK"
> frame at some point.
> 
> Maybe "http2 SETTINGS frame ACK" would be more acceptable?

Yes, "http2 SETTINGS frame ACK" looks good.



> 
> >      ngx_log_debug1(NGX_LOG_DEBUG_HTTP, h2c->connection->log, 0,
> > -                   "http2 SETTINGS frame params:%uz",
> > +                   "http2 SETTINGS frame with %uz params",
> >                     h2c->state.length / NGX_HTTP_V2_SETTINGS_PARAM_SIZE);
> 
> The extra "with" doesn't add any information, and it doesn't seem to
> be inline with other debug logs, but I don't mind this change too
> much.
> 
> >              ngx_log_debug1(NGX_LOG_DEBUG_HTTP, h2c->connection->log, 0,
> > -                           "http2 SETTINGS param HEADER_TABLE_SIZE:%ui "
> > +                           "http2 SETTINGS param HEADER_TABLE_SIZE: %ui "
> >                             "(ignored)", value);
> 
> This is fine with me, but again, the extra space doesn't seem to be
> inline with other debug logs, which usually don't include space
> between "key:" and "value".

This is similar to headers debug lines.

No space is usually used in cases when there are multiple small parameters.


> 
> >              ngx_log_debug2(NGX_LOG_DEBUG_HTTP, h2c->connection->log, 0,
> > -                           "http2 SETTINGS param 0x%Xi:%ui "
> > -                           "(ignored)", id, value);
> > +                           "http2 unknown SETTINGS param id:0x%Xi value:%ui",
> > +                           id, value);
> 
> I'd prefer if we could stick to the "http2 SETTINGS" prefix here,
> otherwise it won't look like part of the same frame, i.e.:
> 
>     http2 SETTINGS frame with 3 params
>     http2 SETTINGS param HEADER_TABLE_SIZE: 4096
>     http2 SETTINGS param INITIAL_WINDOW_SIZE: 65536
>     http2 unknown SETTINGS param id:0xAA value:1234
> 
> Also, " (ignored)" part was removed, which makes it inconsistent with
> other ignored parameters.
> 
> Maybe "http2 SETTINGS unknown param id:0x%Xi value:%ui (ignored)" or
[..]

This one looks good enough, but I think that "(ignored)" is excessive
information.  There's nothing else to do with unknown parameter anyway.

  wbr, Valentin V. Bartenev