[PATCH 1 of 2] Core: connect() error log message made more verbose

[Maxim Dounin]

Hello!

On Thu, Mar 09, 2023 at 08:07:03PM +0400, Sergey Kandaurov wrote:

> > On 22 Feb 2023, at 23:55, Maxim Dounin <mdounin at mdounin.ru> wrote:
> > 
> > Hello!
> > 
> > On Wed, Feb 22, 2023 at 03:37:51PM +0400, Sergey Kandaurov wrote:
> > 
> >>> On 9 Feb 2023, at 12:11, Maxim Dounin <mdounin at mdounin.ru> wrote:
> >>> 
> >>> [..]
> >>> # HG changeset patch
> >>> # User Maxim Dounin <mdounin at mdounin.ru>
> >>> # Date 1675929813 -10800
> >>> #      Thu Feb 09 11:03:33 2023 +0300
> >>> # Node ID 6b662855bf77c678a3954939aefe3fd4b4af4c70
> >>> # Parent  cffaf3f2eec8fd33605c2a37814f5ffc30371989
> >>> Syslog: introduced error log handler.
> >>> 
> >>> This ensures that errors which happen during logging to syslog are logged
> >>> with proper context, such as "while logging to syslog" and the server name.
> >>> 
> >>> Prodded by Safar Safarly.
> >>> 
> >>> diff --git a/src/core/ngx_syslog.c b/src/core/ngx_syslog.c
> >>> --- a/src/core/ngx_syslog.c
> >>> +++ b/src/core/ngx_syslog.c
> >>> @@ -18,6 +18,7 @@
> >>> static char *ngx_syslog_parse_args(ngx_conf_t *cf, ngx_syslog_peer_t *peer);
> >>> static ngx_int_t ngx_syslog_init_peer(ngx_syslog_peer_t *peer);
> >>> static void ngx_syslog_cleanup(void *data);
> >>> +static u_char *ngx_syslog_log_error(ngx_log_t *log, u_char *buf, size_t len);
> >>> 
> >>> 
> >>> static char  *facilities[] = {
> >>> @@ -66,6 +67,8 @@ ngx_syslog_process_conf(ngx_conf_t *cf, 
> >>>        ngx_str_set(&peer->tag, "nginx");
> >>>    }
> >>> 
> >>> +    peer->logp = &cf->cycle->new_log;
> >>> +
> >> 
> >> You may want to reflect this change in the description.
> >> That's, this now follows other error logging by using log from
> >> the configuration that is going to be applied (cycle->new_log):
> >> 
> >> src/core/ngx_log.c:    dummy = &cf->cycle->new_log;
> >> src/mail/ngx_mail_core_module.c:    conf->error_log = &cf->cycle->new_log;
> >> src/stream/ngx_stream_core_module.c:    conf->error_log = &cf->cycle->new_log;
> >> src/http/ngx_http_core_module.c:    conf->error_log = &cf->cycle->new_log;
> >> src/core/ngx_resolver.c:    r->log = &cf->cycle->new_log;
> >> src/core/ngx_cycle.c:    cycle->log = &cycle->new_log;
> >> 
> >> Previously, before configuration was read, it used init_cycle configuration,
> >> that's builtin error_log on the NGX_LOG_NOTICE level, which means that its
> >> own ngx_send debug appeared only after the configuration was read, and other
> >> syslog error logging was limited to the builtin error log.
> > 
> > The main goal of introduction of peer->logp is to avoid 
> > re-initializing peer->log on each ngx_syslog_send() call.  
> > Previous code used to initialize peer->conn.log on each call, 
> > though now there are more things to initialize, and doing this on 
> > each call makes the issue obvious.
> > 
> > But yes, the resulting code is consistent with other uses and 
> > matches how logging is expected to be used: when something is used 
> > in a context of a configuration, it uses logging from the 
> > configuration.
> > 
> > A side note: it looks like ngx_syslog_add_header() uses 
> > ngx_cycle->hostname.  During initial startup this will use 
> > init_cycle->hostname, which is empty.
> > 
> > Looking at all this again I tend to think it might be a good idea 
> > to ditch ngx_cycle usage in a separate patch (both for 
> > ngx_cycle->log and ngx_cycle->hostname), and implement proper 
> > logging context on top of it.  Patches below.
> > 
> >>>    peer->conn.fd = (ngx_socket_t) -1;
> >>> 
> >>>    peer->conn.read = &ngx_syslog_dummy_event;
> >>> @@ -286,15 +289,19 @@ ngx_syslog_send(ngx_syslog_peer_t *peer,
> >>> {
> >>>    ssize_t  n;
> >>> 
> >>> +    if (peer->log.handler == NULL) {
> >>> +        peer->log = *peer->logp;
> >>> +        peer->log.handler = ngx_syslog_log_error;
> >>> +        peer->log.data = peer;
> >>> +        peer->log.action = "logging to syslog";
> >>> +    }
> >>> +
> >>>    if (peer->conn.fd == (ngx_socket_t) -1) {
> >>>        if (ngx_syslog_init_peer(peer) != NGX_OK) {
> >>>            return NGX_ERROR;
> >>>        }
> >>>    }
> >>> 
> >>> -    /* log syslog socket events with valid log */
> >>> -    peer->conn.log = ngx_cycle->log;
> >>> -
> >>>    if (ngx_send) {
> >>>        n = ngx_send(&peer->conn, buf, len);
> >>> 
> >> 
> >> One conversion to &peer->log is missing in the ngx_send error handling:
> >> 
> >> diff --git a/src/core/ngx_syslog.c b/src/core/ngx_syslog.c
> >> --- a/src/core/ngx_syslog.c
> >> +++ b/src/core/ngx_syslog.c
> >> @@ -313,7 +313,7 @@ ngx_syslog_send(ngx_syslog_peer_t *peer,
> >>     if (n == NGX_ERROR) {
> >> 
> >>         if (ngx_close_socket(peer->conn.fd) == -1) {
> >> -            ngx_log_error(NGX_LOG_ALERT, ngx_cycle->log, ngx_socket_errno,
> >> +            ngx_log_error(NGX_LOG_ALERT, &peer->log, ngx_socket_errno,
> >>                           ngx_close_socket_n " failed");
> >>         }
> >> 
> >> 
> >> Other than that it looks good.
> > 
> > Applied, thanks.
> > 
> > Updated patches below.
> > 
> > # HG changeset patch
> > # User Maxim Dounin <mdounin at mdounin.ru>
> > # Date 1677095349 -10800
> > #      Wed Feb 22 22:49:09 2023 +0300
> > # Node ID a964a013031dabbdd05fb0637de496640070c416
> > # Parent  cffaf3f2eec8fd33605c2a37814f5ffc30371989
> > Syslog: removed usage of ngx_cycle->log and ngx_cycle->hostname.
> > [..]
> > 
> > # HG changeset patch
> > # User Maxim Dounin <mdounin at mdounin.ru>
> > # Date 1677095351 -10800
> > #      Wed Feb 22 22:49:11 2023 +0300
> > # Node ID 8f7c464c54e0b18bdb4d505866755cd600fac9fb
> > # Parent  a964a013031dabbdd05fb0637de496640070c416
> > Syslog: introduced error log handler.
> > [..]
> 
> Both patches are good for me.

Thanks, pushed to http://mdounin.ru/hg/nginx.

-- 
Maxim Dounin
http://mdounin.ru/