client Fixes #27
client Fixes #27
Conversation
client.c: In function 'get_fan_status':
client.c:301:11: warning: format '%d' expects argument of type 'int', but argument 8 has type 'nxjson_s64 {aka const long int}' [-Wformat=]
"Fan Display Name : %s\n"
instead of allocating a fixed-size buffer.
additionally, even if malloc returns a usable allocation, it will not be zero-initialized.
1e13 is 10000000000000, which doesn't even fit into an int.
instead, use a stack buffer of 128 kbyte which should be plenty,
then strdup the actual string on return.
note that this entire function is horribly inefficient as the
string has to be scanned over and over.
an efficient implementation would use
len = snprintf(buf, sizeof buf "[%s]", s)
instead of
strcat(s, "["), strcat(s, t), strcat(s, "]")
, and then add len to the start pointer.
while at it, also rename the confusing IF_... macro which suggests an
if block starts, and use a function-like macro.
recursive make considered harmful: https://embeddedartistry.com/blog/2017/04/10/recursive-make-considered-harmful/ the debug and release targets were removed. debug/release builds being provided by the makefile is CMake-think, a dangerous virus infecting the minds of the innocent. the canonical way to make a release build is: make CFLAGS=-O3 LDFLAGS-s and a debug build: make CFLAGS="-O0 -g3" for someone who's too lazy to type that out these oneliners can be put into debug.sh and release.sh, respectively.
not everyone uses systemd. this allows to selectively use the targets one is interested in.
|
i also took the liberty to improve the makefile according to the guide i've written:
|
|
Thanks for this! I've submitted my own PR to your branch which resolves a couple of issues that I found with the new /usr/local prefix in its current implementation. I have also adjusted the Makefile so that the default behaviour is in line with how it was before, while hopefully still allowing for the customization improvements that you have made. |
|
thanks. i've seen your stuff is in https://github.com/JoshIles/nbfc-linux/commits/fixes but didn't make it here. the prefix adjustements i made are in line with autoconf and other makefile-only build systems, where prefix always defaults to /usr/local. the intention is that a user compiling stuff from source wants it in /usr/local in order to not collide with distro-provided packages, and someone who knows what he's doing or a distro packager can just run make and install-systemd-files then gets a dependency on |
|
as for CMake-style build vs release targets, the conventional way to do this in makefile-based build system is to run |
- added -lm as a requirement. - added install-c target which installs nbfc-linux with the C client, so users no longer need to manually edit the Makefile. - changed uninstall target so that it force removes files, ensuring that it doesn't exit with Error in the slim (but real) chance that one of the files were manually removed. - added new targets to make the python client and systemd service files compatibile with the user-defined path PREFIX, and updated those files accordingly.
|
nicely done! |
|
Thanks, and thank you for the much more robust solution with regard to the prefix. I have chosen to leave the conditional to allow 'make BUILD=debug' for a debug build, simply because it preserves the previous defaults, and advanced users can override it if they wish, so it shouldn't be an issue. But, I by no means have the final say on this matter. I'd welcome some more input from anyone on this PR, since there is still currently no way of updating the documentation dynamically based on the user-definable prefix. Currently, @braph's argparse-tool is not part of the source tree, which was used to generate some of this stuff, as can be seen in the Makefile. In addition, completion/ec_probe.py, completion/nbfc_service.py, and nbfc.py.in would need to be dynamically edited, as they still contain hard-coded prefix paths and from which argparse-tool does its job. I also am yet to figure out how nbfc_service.help.h was generated. |
- nbfc config --set auto now works correctly. - couple of formatting improvements. Additionally - corrected a couple of typos. - new entries in .gitignore.
client.c
Note: I never knew there were issues with setting configs via the C client before because I always had a leftover config file from the Python client. The changes to the uninstall rule in the Makefile now removes this, which made these issues apparent. It's no wonder they were never caught before. Additionally
|
No description provided.