Skip to content

Conversation

@twoeater
Copy link

but upsdrvctl.exe located in sbin.
So, change bin to sbin.

General points

  • Described the changes in the PR submission or a separate issue, e.g.
    known published or discovered protocols, applicable hardware (expected
    compatible and actually tested/developed against), limitations, etc.

  • There may be multiple commits in the PR, aligned and commented with
    a functional change. Notably, coding style changes better belong in a
    separate PR, but certainly in a dedicated commit to simplify reviews
    of "real" changes in the other commits. Similarly for typo fixes in
    comments or text documents.

  • Please star NUT on GitHub, this helps with sponsorships! ;)

Frequent "underwater rocks" for driver addition/update PRs

  • Revised existing driver families and added a sub-driver if applicable
    (nutdrv_qx, usbhid-ups...) or added a brand new driver in the other
    case.

  • Did not extend obsoleted drivers with new hardware support features
    (notably blazer and other single-device family drivers for Qx protocols,
    except the new nutdrv_qx which should cover them all).

  • For updated existing device drivers, bumped the DRIVER_VERSION macro
    or its equivalent.

  • For USB devices (HID or not), revised that the driver uses unique
    VID/PID combinations, or raised discussions when this is not the case
    (several vendors do use same interface chips for unrelated protocols).

  • For new USB devices, built and committed the changes for the
    scripts/upower/95-upower-hid.hwdb file

  • Proposed NUT data mapping is aligned with existing docs/nut-names.txt
    file. If the device exposes useful data points not listed in the file, the
    experimental.* namespace can be used as documented there, and discussion
    should be raised on the NUT Developers mailing list to standardize the new
    concept.

  • Updated data/driver.list.in if applicable (new tested device info)

Frequent "underwater rocks" for general C code PRs

  • Did not "blindly assume" default integer type sizes and value ranges,
    structure layout and alignment in memory, endianness (layout of bytes and
    bits in memory for multi-byte numeric types), or use of generic int where
    language or libraries dictate the use of size_t (or ssize_t sometimes).
  • Progress and errors are handled with upsdebugx(), upslogx(),
    fatalx() and related methods, not with direct printf() or exit().
    Similarly, NUT helpers are used for error-checked memory allocation and
    string operations (except where customized error handling is needed,
    such as unlocking device ports, etc.)

  • Coding style (including whitespace for indentations) follows precedent
    in the code of the file, and examples/guide in docs/developers.txt file.

  • For newly added files, the Makefile.am recipes were updated and the
    make distcheck target passes.

General documentation updates

  • Updated docs/acknowledgements.txt (for vendor-backed device support)

  • Added or updated manual page information in docs/man/*.txt files
    and corresponding recipe lists in docs/man/Makefile.am for new pages

  • Passed make spellcheck, updated spell-checking dictionary in the
    docs/nut.dict file if needed (did not remove any words -- the make
    rule printout in case of changes suggests how to maintain it).

Additional work may be needed after posting this PR

  • Propose a PR for NUT DDL with detailed device data dumps from tests
    against real hardware (the more models, the better).

  • Address NUT CI farm build failures for the PR: testing on numerous
    platforms and toolkits can expose issues not seen on just one system.

  • Revise suggestions from LGTM.COM analysis about "new issues" with
    the changed codebase.

but upsdrvctl.exe located in sbin.
So, change bin to sbin.
@twoeater
Copy link
Author

twoeater commented Aug 21, 2025

#3063 (comment)
Sorry to bother you.
I can't Set up a build environment. I don't have mingw experience.
Please approval test build. then i trying to test.

most drivers are located in "bin".
but referred Relative running path, it is "sbin".
So changing to "NULL(running path)" to "bin"
@twoeater twoeater marked this pull request as ready for review August 21, 2025 02:39
@twoeater
Copy link
Author

PS C:\NUT-for-Windows-x86_64-SNAPSHOT-2.8.4.443-master\mingw64\bin> .\nut.exe -N
Running in non-service mode

EventLog : Starting

EventLog : Starting drivers

EventLog : Starting upsd

Network UPS Tools upsdrvctl.exe - UPS driver controller 2.8.4.8.3-11+g53a17cf87 (development iteration after 2.8.4)
Network UPS Tools upsd 2.8.4.8.3-11+g53a17cf87 (development iteration after 2.8.4)
listening on 0.0.0.0 port 3493
Found 1 UPS defined in ups.conf
Network UPS Tools 2.8.4.8.3-11+g53a17cf87 (development iteration after 2.8.4) - Generic HID driver 0.67
USB communication driver (libusb 1.0) 0.50
Warning: report descriptor too short (expected 745, got 33)
Please check your Windows Device Manager: perhaps the UPS was recognized by default OS
driver such as HID UPS Battery (hidbatt.sys, hidusb.sys or similar). It could have been
"restored" by Windows Update. You can try https://zadig.akeo.ie/ to handle it with
either WinUSB, libusb0.sys or libusbK.sys.
HIDParse: LogMax is less than LogMin. Vendor HID report descriptor may be incorrect; interpreting LogMax -1 as 255 in ReportID: 0x00
HIDParse: LogMax is less than LogMin. Vendor HID report descriptor may be incorrect; interpreting LogMax -1 as 255 in ReportID: 0x00
Using subdriver: APC HID 0.100
Defaulting lbrb_log_delay_sec=3 for American Power Conversion model Back-UPS BX1600MI; consider also setting the lbrb_log_delay_without_calibrating and/or onlinedischarge_calibration and/or onlinedischarge_log_throttle_sec flag(s) in your configuration
Listening on named pipe \\.\pipe\usbhid-ups-apc-bx1600
EventLog : Starting upsmon

EventLog : upsd - listening on 0.0.0.0 port 3493

Network UPS Tools upsmon 2.8.4.8.3-11+g53a17cf87 (development iteration after 2.8.4)
UPS: [email protected] (primary) (power value 1)
Using power down flag file C:\killpower
Warning: no custom notification command defined, just so you know
EventLog : upsmon - Startup successful

EventLog : upsmon - upsnotify: failed to notify about state NOTIFY_STATE_READY_WITH_PID: no notification tech defined, will not spam more about it

EventLog : upsd - User [email protected] logged into UPS [apc-bx1600]

It's working.
Not need to driverpath in ups.conf.
just CLI clean run.

@jimklimov
Copy link
Member

jimklimov commented Aug 22, 2025

Odd, i think upsdrvctl(.exe) should be in same dir as drivers - is it also in Windows builds? (drvpath which may be binpath per configuration of that build).

  • UPDATE: no it is not, details in later posts below

NUT for Windows env setup (semi-native with MSYS2 or cross with Linux+mingw) is documented in docs/config-prereqs.txt and scripts/Windows/README.adoc - you may have to pre-build some prerequisites on this or that platform. Also see appveyor.yml which sets it up from scratch for CI builds.

@jimklimov
Copy link
Member

Also, I'm away from computers till September, so can only look at this in detail in a couple of weeks. Just in case, hoping someone else might chime in sooner...

driverpath = xstrdup(DRVPATH); /* set default */
#else /* WIN32 */
driverpath = getfullpath(NULL); /* Relative path in WIN32 */
driverpath = getfullpath(PATH_BIN); /* Relative path in WIN32 */
Copy link
Member

@jimklimov jimklimov Sep 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, the method (in common.c) actually has a possible NULL deref problem if there is no backslash. And also looks only for that one, ignoring forward slashes (that recent Windows do accept... at least if not mixing two styles in same string - not sure yet what happens then).

  • UPDATE: Checked locally in cmd with dir "c:/temp\tools" - that worked okay in Win11 (as long as quotes were used, otherwise forward slash was assumed to start an invalid CLI argument)

This problem probably should never happen since we parse the output of WIN32 API method GetModuleFileName() but still that bit does not seem clean now that I looked at it.

driverpath = xstrdup(DRVPATH); /* set default */
#else /* WIN32 */
driverpath = getfullpath(NULL); /* Relative path in WIN32 */
driverpath = getfullpath(PATH_BIN); /* Relative path in WIN32 */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the whole PR is flawed here by assuming the drivers would be in PATH_BIN. This is a coincidence due to configure script arguments involved (or defaulted) in a particular build; the pedantically correct one would be the DRVPATH (or a relative path to that, starting from location of upsdrvctl.exe).

I believe here the root-cause problem is rather derived from assuming in this code spot that the tool is in same location as drivers (NULL relative path, compared to program module location), despite this drivers/Makefile.am piece:

# always build upsdrvctl
sbin_PROGRAMS = upsdrvctl

(because drivers are often hidden in some /lib/nut or under $libexec, and the tool to generally manage them should be in a more visible location).

The puzzle to solve here is in fact the relative path to drivers, I think, because the absolute paths built into the code may be flawed (in the default build config for Win32 in scripts/Windows/build-mingw-nut.sh script, the --prefix=/ and a shifted make install DESTDIR=$INSTALL_DIR is used).

char *path;

path = getfullpath(PATH_BIN);
path = getfullpath(PATH_SBIN);
Copy link
Member

@jimklimov jimklimov Sep 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here the change to PATH_SBIN is correct (matching the install location of upsdrvctl(.exe) per drivers/Makefile.am).

Copy link
Member

@jimklimov jimklimov Sep 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, it seems the relative path to another directory should be added with slash and dot-dot like "/../lib" (in one example); but "thanks" to --prefix=/ in those builds, I see paths actually almost ready to use with this approach - e.g.

$ grep PATH scripts/Windows/nut_build_x86_64-w64-mingw32/include/config.h
#define CONFIG_FLAGS "--host=x86_64-w64-mingw32 --build=x86_64-linux-gnu --prefix=/ --enable-keep_nut_report_feature PKG_CONFIG_PATH=/usr/x86_64-w64-mingw32/lib/pkgconfig --with-all=auto --with-doc='man=auto html-single=auto html-chunked=skip pdf=skip' --without-systemdsystemunitdir --with-pynut=app --with-augeas-lenses-dir=/augeas-lenses --enable-Werror"

#define ALTPIDPATH "/var/state/ups"
#define CGIPATH "//cgi-bin"
#define CONFPATH "//etc"
#define DRVPATH "//bin"
#define HTMLPATH "//html"
#define PIDPATH "/run"
#define STATEPATH "/var/state/ups"

...but lacking the /../ part - so now I am surprised it did find anything (whether binaries or configs).

Needs some more debugging I guess.

Also, the getfullpath() is used inconsistently I think, as it currently uses xstrdup() to return what it finds and needs the caller to free() the returned string, while methods that return PID, config, etc. locations generally return pointers to someone else's strings that must not be freed (env, built-in defaults). So I guess these calls can leak when repetitively called on Win32 builds.

@jimklimov jimklimov added service/daemon start/stop General subject for starting and stopping NUT daemons (drivers, server, monitor); also BG/FG/Debug Windows-not-on-par-with-POSIX Aspect of Windows builds known to be dysfunctional compared to POSIX builds; fix needed to be on par labels Sep 5, 2025
@jimklimov jimklimov marked this pull request as draft September 6, 2025 22:35
jimklimov added a commit that referenced this pull request Sep 6, 2025
Started from review of PR #3065 for issue #3063.
* The method had a potential NULL dereference
* Only backslashes were considered
* Issues above should never happen with real Windows API,
  but who knows what we get in cross-builds?..

Signed-off-by: Jim Klimov <[email protected]>
jimklimov added a commit to jimklimov/nut that referenced this pull request Sep 6, 2025
…ls#3065]

Started from review of PR networkupstools#3065 for issue networkupstools#3063.
* The method had a potential NULL dereference
* Only backslashes were considered
* Issues above should never happen with real Windows API,
  but who knows what we get in cross-builds?..

Signed-off-by: Jim Klimov <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

service/daemon start/stop General subject for starting and stopping NUT daemons (drivers, server, monitor); also BG/FG/Debug Windows-not-on-par-with-POSIX Aspect of Windows builds known to be dysfunctional compared to POSIX builds; fix needed to be on par

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants