> On 22 Feb 2023, at 23:55, Maxim Dounin <mdounin@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@mdounin.ru> wrote:
>>>
>>> [..]
>>> # HG changeset patch
>>> # User Maxim Dounin <mdounin@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@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@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.

--
Sergey Kandaurov
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel