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 _______________________________________________ nginx-devel mailing list nginx-devel@nginx.org http://mailman.nginx.org/mailman/listinfo/nginx-devel