Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace zabbix_log() macro provided by Zabbix #142

Merged
merged 1 commit into from
Feb 18, 2022
Merged

Replace zabbix_log() macro provided by Zabbix #142

merged 1 commit into from
Feb 18, 2022

Conversation

i-ky
Copy link
Contributor

@i-ky i-ky commented May 1, 2019

Our own macro will ensure log level check across all versions of Zabbix.

Some background story. zabbix_log() has always been a wrapper macro for __zbx_zabbix_log() function. It takes the severity level of a log message, format string and variable number of arguments. Depending on the severity of the log message and logging level currently in action, log message may or may not end up in the actual Zabbix log. In older versions of Zabbix, log level check was performed by the __zbx_zabbix_log() function, but after ZBX-10889 the situation has changed. Depending on how Zabbix was compiled, the check will be performed by the __zbx_zabbix_log() function like before or by the zabbix_log() macro itself. What this effectively means is that loadable module compiled with older zabbix_log() macro definition and loaded into newer Zabbix binary may end up with no log level check whatsoever when calling __zbx_zabbix_log() function.

In order to provide a guarantee that module can be compiled with any version of Zabbix headers and then used with any other version of Zabbix, I decided to implement an independent zabbix_log() macro which will check log level on its own. Again, runtime linking is used to get access to current log level settings, because API has changed in ZBX-10889. Before runtime linking is complete zabbix_log() cannot be used, therefore I've replaced it with printf() and fprintf(stderr, ...). Zabbix redirects stdout and stderr to the log file anyway.

For a test, I've compiled the module with good old 3.2 and latest trunk and then successfully loaded it into oldish 3.4 and trunk in all possible combinations. However, I'm not using this module or PostgreSQL in my everyday life, so some more in-depth testing may be required.

Hopefully, this will be enough to craft a single binary package compatible with all more or less current versions of Zabbix (despite binary compatibility not being officially supported).

…h will ensure log level check across all versions of Zabbix
@biertie
Copy link

biertie commented Sep 12, 2019

Hello,

Can someone check and approve this patch? :-)
We are waiting for it. Sadly enough, not enough C experience here.

Bert

@i-ky
Copy link
Contributor Author

i-ky commented Sep 14, 2019

@biertie, you don't need to be an experienced C programmer. If you are using this module and can test this branch with various versions of Zabbix, you can provide quite useful feedback.

@OrangeDog
Copy link
Collaborator

@cavaliercoder can this be merged? And then ideally can you sort out being able to release it?

@dodok1
Copy link

dodok1 commented Jan 25, 2020

I had to merge this PR, because after using compiled master with ZBX 4.4.4, I'd end up with very verbose module filling log partition quickly.

@i-ky
Copy link
Contributor Author

i-ky commented Jul 17, 2021

There is another ABI break on the horizon: zabbix-tools/zabbix-module-sockets#7 (comment)

Is anyone interested in me updating this pull request?

@OrangeDog
Copy link
Collaborator

@i-ky I'd be interested. Even better if you can sort out the release build and either get control from @cavaliercoder or fork it.
I tried a while ago, but what's in this repo is not what produced the last releases.

@i-ky
Copy link
Contributor Author

i-ky commented Jul 17, 2021

I've not seen @cavaliercoder responding to mentions on GitHub for some time now. Can someone contact him on Twitter or LinkedIn? I use neither of them.

I don't think I'm the best person to "get control", because I don't use Zabbix myself.

@i-ky
Copy link
Contributor Author

i-ky commented Jul 17, 2021

And by the way, if you haven't already, please consider voting for ZBXNEXT-5189.

@cavaliercoder
Copy link
Collaborator

Hey friends. I apologise for not maintaining this project. It's one of things where I intend to give it a few days of focus every now and then, but life keeps getting in the way. I haven't worked with Zabbix in years, so the relevance for me has long faded. I get so much email about my OSS projects, it's hard to pay attention to and sometimes pings like this get missed.

I will gladly hand over control to someone. The beauty of open source is, you can just take it. Please feel free to fork this, publish it around the place, etc. with the appropriate license attribution.

@i-ky
Copy link
Contributor Author

i-ky commented Jul 19, 2021

@cavaliercoder, happy to hear you!

Forking is not ideal in my opinion. I bet there are lots of links to this repo all over the Internet and it will take forever to update them all. Forks will cause confusion.

I am a Collaborator in a couple of repos and it is sufficient most of the time, but I still have to ask repository's owner to intervene if repo's settings need adjustment (for CI and whatnot).

I have not used this feature, but maybe creating an Organization and transferring all of your Zabbix legacy (in a good sense!) there will be the best way for you to let it go.

@OrangeDog
Copy link
Collaborator

I'll happily hold some keys in the absence of anyone else, but I'm probably not of much use at the moment. At least I do actually use this project.

@i-ky
Copy link
Contributor Author

i-ky commented Aug 18, 2021

@cavaliercoder, can you make @OrangeDog a collaborator?

@i-ky
Copy link
Contributor Author

i-ky commented Feb 18, 2022

@OrangeDog, as I can see, you are a collaborator now. Can you merge this PR?

@OrangeDog OrangeDog merged commit 2e93bc1 into zabbix-tools:master Feb 18, 2022
@i-ky i-ky deleted the compatibility-hack-2 branch February 18, 2022 20:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants