[Home]

Summary:ASTERISK-28435: cdr_pgsql: Unix socket doesn't work
Reporter:Dmitry Svyatogorov (PnD)Labels:
Date Opened:2019-06-03 09:12:38Date Closed:2019-06-03 16:00:00
Priority:MajorRegression?Yes
Status:Closed/CompleteComponents:CDR/cdr_pgsql
Versions:16.3.0 Frequency of
Occurrence		Constant
Related
Issues:
Environment:Attachments:
Description:After refactoring, function pgsql_reconnect() does "ast_str_set()" with statically composed string:
{code}
ast_str_set(&conn_info, 0, "host=%s port=%s dbname=%s user=%s"
{code}
It results in error while trying to connect to unix socket (empty host and port in config), so it's a regression.

I propose the simple check:

*<edit>*
Editing to remove inline patch.

Apologies, unfortunately inline patches are not allowed. Please see the patch contribution wiki [1] to learn more about contributing a patch to Asterisk, or attaching one here

[1] https://wiki.asterisk.org/wiki/display/AST/Patch+Contribution+Process

*<end edit>*

This patch works for me. _But m.b. there is more clear way?_
Comments:By: Asterisk Team (asteriskteam) 2019-06-03 09:12:39.038-0500

Thanks for creating a report! The issue has entered the triage process. That means the issue will wait in this status until a Bug Marshal has an opportunity to review the issue. Once the issue has been reviewed you will receive comments regarding the next steps towards resolution.

A good first step is for you to review the [Asterisk Issue Guidelines|https://wiki.asterisk.org/wiki/display/AST/Asterisk+Issue+Guidelines] if you haven't already. The guidelines detail what is expected from an Asterisk issue report.

Then, if you are submitting a patch, please review the [Patch Contribution Process|https://wiki.asterisk.org/wiki/display/AST/Patch+Contribution+Process].

Please note that once your issue enters an open state it has been accepted. As Asterisk is an open source project there is no guarantee or timeframe on when your issue will be looked into. If you need expedient resolution you will need to find and pay a suitable developer. Asking for an update on your issue will not yield any progress on it and will not result in a response. All updates are posted to the issue when they occur.
By: Chris Savinovich (csavinovich) 2019-06-03 16:00:01.003-0500

Hi Dmitry, thank you very much for your collaboration.

The following code will do a better job, since it also checks for the values of each parameter, using internal asterisk function ast_strlen_zero() which is actually used a couple of lines later in the same function.

I am going to proceed to submit the code to Gerrit for review, unless you prefer to do it yourself, in which case I will look for your reply back. otherwise if I don't hear from you I will take it you want me to submit it.

Thank you and best regards

<edit to remove inline patch>
By: Kevin Harwell (kharwell) 2019-06-03 18:04:27.631-0500

Just a note, but we unfortunately we are unable to accept inline patches. A patch can either be submitted to gerrit directly, or attached here after signing the contributor's license agreement. For more information please see the following:

https://wiki.asterisk.org/wiki/display/AST/Patch+Contribution+Process
By: Dmitry Svyatogorov (PnD) 2019-06-04 03:32:44.462-0500

Hi all, thanks a lot for rapid reply!
Feel free to push the correct patch from your own. (And I'll take this process as an example).
I'm just interested in asterisk well-functioning.
By: Chris Savinovich (csavinovich) 2019-06-10 17:39:08.836-0500

This ticket is waiting for Gerrit review.

By: Friendly Automation (friendly-automation) 2019-06-11 08:02:34.354-0500

Change 11442 merged by Friendly Automation:
cdr_pgsql: fix error in connection string

[https://gerrit.asterisk.org/c/asterisk/+/11442|https://gerrit.asterisk.org/c/asterisk/+/11442]
By: Friendly Automation (friendly-automation) 2019-06-11 08:05:20.287-0500

Change 11443 merged by Friendly Automation:
cdr_pgsql: fix error in connection string

[https://gerrit.asterisk.org/c/asterisk/+/11443|https://gerrit.asterisk.org/c/asterisk/+/11443]
By: Friendly Automation (friendly-automation) 2019-06-11 08:05:34.310-0500

Change 11441 merged by Joshua Colp:
cdr_pgsql: fix error in connection string

[https://gerrit.asterisk.org/c/asterisk/+/11441|https://gerrit.asterisk.org/c/asterisk/+/11441]
By: Dmitry Svyatogorov (PnD) 2019-06-11 08:41:05.875-0500

(!) Please, recheck this part of resulting patch:
{code}
if (!ast_strlen_zero(pghostname)) {
ast_str_append(&conn_info, 0, "host=%s ", pghostname);
}
if (!ast_strlen_zero(pgdbport)) {
ast_str_append(&conn_info, 0, "port=%s ", pgdbport);
}
{code}
As far as I view the rest of "cdr_pgsql.c", pgdbport is always set (at least to default 5432).
At my point of view (no host → no port), so the check for pgdbport must be placed inside one for pghostname.
By: Asterisk Team (asteriskteam) 2019-06-11 08:41:06.400-0500

This issue has been reopened as a result of your commenting on it as the reporter. It will be triaged once again as applicable.
By: Chris Savinovich (csavinovich) 2019-06-11 11:13:58.039-0500

Hello Dmitry, sorry but the patch was merged to the release this morning.  While you are totally right when you say "no host no port" the end result of the current code is worst case scenario the connection string is syntactically valid. I would go back and make it better as per your suggestion but in the interest of responding to the other tickets I am going to leave as is. You are welcome to submit your own patch to Gerrit for review.  I want to suggest also, once code is up for review in Gerrit the best way to suggest a change to it is to participate in the review process, and add your positive or negative review to it. Works like a charm since it invites the opinion of all reviewers.
Thanks
Chris