-
Notifications
You must be signed in to change notification settings - Fork 38
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
Simplest code triggers assertion failure #344
Comments
If I use
which ruined my program And the ASAN complains
Full code // gcc test.c -o test -lddcutil -g -fsanitize=address
#include <ddcutil_c_api.h>
int main()
{
ddca_init("--disable-watch-displays", DDCA_SYSLOG_NOT_SET, DDCA_INIT_OPTIONS_DISABLE_CONFIG_FILE);
} |
Ref: #330 (comment) |
I have pushed a fix for the assert() failure to branch 2.0.2-dev. Running your sample program under valgrind shows some leaks. I will investigate. Re the options messages, I see your problem. The messages exist in order to avoid confusion about unexpected settings, but they should be optional in some way. In retrospect ddca_init() should have had an additional argument at which a null-terminated list of benign initialization messages are returned. I will have to think through how to control those messages without a SONAME bump. In the meantime, wrapping ddca_init() in ddca_start_capture() and ddca_end_capture() should address the problem, though it is kludgy. Re the reference to issue #330, are you saying the dlopen() segfault problem still exists? Does it require that driver i2c-dev not be loaded? Finally, I see that you're running your program in a VM using the VMWare virtualized video driver. Last I looked at the question some time ago, virtualized video drivers do not support I2C communication. The only way to access the /dev/i2c devices from a VM is to use a non-virtual video driver using passthru to an actual physical video card. |
Thanks for the fix.
What about a new flag
I metioned the issue because it was the source that
I knew it. The Linux distro I use (Fedora) haven't upgrade ddcutil to 2.0 yet, so I tested it in a VM with a different distro. AFAIK most distros still stuck with ddcutil 1.4, so my program has to support both 1.4 and 2.0. I have to say that it was not a good experience. Because of the API imcompatibility, my code ends up with #if DDCUTIL_VMAJOR >= 2
double ddca_set_default_sleep_multiplier(double multiplier);
#else
void ddca_init(const char* libopts, int syslog_level_arg, int opts);
#endif
void pseudoCode()
{
void* libddcutil = dlopen("libddcutil.so");
__typeof__(&ddca_init) ffddca_init = dlsym(libddcutil, "ddca_init");
if (ffddca_init)
{
// use ffddca_init
}
else
{
__typeof__(&ddca_set_default_sleep_multiplier) ffddca_set_default_sleep_multiplier = dlsym(libddcutil, "ddca_set_default_sleep_multiplier");
if (ffddca_set_default_sleep_multiplier)
{
// use ffddca_set_default_sleep_multiplier
}
}
dlclose(libddcutil);
} In addition, |
I have added option flag DDCA_INIT_OPTIONS_ENABLE_INIT_MSGS to DDCA_Init_Options in branch 2.0.2-dev. The informational messages regarding how the option string is assembled will only be output if this flag is set. (These messages are still written to the system log, if syslog is enabled.) As I previously noted, a better solution would be to add a msg collector argument to ddca_init() but that would have required a SONAME bump, and adding a new ddca_init2() function with that argument would I think be needlessly confusing. Why is ddca_init() defined the way it is? The number of ddcutil options is vast. Most are useful only for various special cases. Having rarely used API calls for each option bloated the API, and the problem was only going to get worse. Plus the options passed from the client program must be merged in some way from those obtained from the configuration file, and some settings are applicable only before initialization has occurred. So the general solution is to assemble an option string from the configuration file and the libopts string argument to ddca_init() and pass that assembled string to the parser. However, some things must be known before the parser is called. which is why ddca_init() has the syslog_level and opts arguments. In particular, that is why the informational messages about how the options string has been assembled are controlled by a flag in opts instead of the ultimate option string passed to the parser. I have added documentation about option --disable-watch-displays to page Shared Library Options. The default may change to --enable-watch-displays in a future release, so for safety I'd suggest just continuing to pass this option as dynamic display detection is irrelevant for your application. I apologize for the complexity release 2.0 adds to your application. Given the need for a SONAME bump, this was the time to bite the bullet on a large number of needed changes. I am aware that it will be some time before ddcutil 2.0 will make it into most distributions. I maintain ddcutil in the official Debian repositories, and my plan is to continue to distribute libddcuti.so.4 as well as the new libddcutil.so.5. I would hope that other binary based distributions do the same. This should enable existing applications that do not use dynamic linking to work unchanged. (As the process for acceptance of packages into Debian is complex, I do not plan to submit ddcutil 2.0 until at least the first patch release. It should then become part of Ubuntu long term support release 24.04.) Regarding memory leaks, the ASAN output gives no hint as to the code that is causing the leak. More useful would be to build libddcutil with CFLAGS=-g and run your program under valgrind with option --leak-check=full. If you'd prefer, direct me to your source code I will do this myself. Finally, thank you for your precise input. You're using libddcutil in a way I hadn't anticipated, so it's very helpful and I want to be responsive to the problems you encounter. |
You don't need my code to reproduce the leaks. Just call
Some distro / package managers have adopted ddcutil 2.0. If your program has incompatibility issues against their existing packages, it's your fault and it's your duty to resolve the issues, despite you did nothing wrong. Anyway, My code is here if you interest: https://github.com/fastfetch-cli/fastfetch/blob/dev/src/detection/brightness/brightness_linux.c#L93-L148 (Usage of My requirement is simple. It fetches brightness values of all external displays as fast as possible. And if the user doesn't have libddcutil installed, the function will be ignored. |
|
The most recent memory leaks you have reported should now be addressed in branch 2.0.2-dev. Re:
No. The SONAME change from libddcutil.4 to libddcutil.5 declares that the new library is not backwards compatible with the old. Applications written to the earlier library should require that library. I will try to make it clearer to distributions that both libraries need to be installed if they have applications built to the older library, and simpler to do so, but that is their responsibility. |
Hopefully a new release soon |
Results in
The text was updated successfully, but these errors were encountered: