win32: fix short reads on serial timeout - #3560
Open
Marcelo-PX wants to merge 1 commit into
Open
Conversation
|
A ZIP file with standard source tarball and another tarball with pre-built docs for commit d730b34 is temporarily available: NUT-tarballs-PR-3560.zip. |
jimklimov
approved these changes
Aug 5, 2026
jimklimov
left a comment
Member
There was a problem hiding this comment.
LGTM, can you look at errno for tot==0 case, and add an entry in NEWS.adoc please?
Signed-off-by: Marcelo Pacheco <marcello.mpacheco@gmail.com>
Marcelo-PX
force-pushed
the
win32-serial-timeout-posix
branch
from
August 5, 2026 18:13
e8a79ed to
d730b34
Compare
|
✅ Build nut 2.8.5.5030-master completed (commit e928ac69db by @Marcelo-PX)
|
23 tasks
|
✅ Build nut 2.8.5.5031-master completed (commit dd57052335 by @Marcelo-PX)
|
|
✅ Build nut 2.8.5.5032-master completed (commit fd145a835c by @Marcelo-PX)
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This is a revised follow-up to #3559.
Fix the Win32 serial read wrapper so that bytes already transferred before a
timeout are returned as a successful short read instead of being discarded as
an I/O error.
When
w32_serial_read()times out while waiting for additional serial data,valid bytes may already have been copied into the caller's buffer. The previous
Win32 code path could discard those bytes by falling through to the generic
error handling.
The revised implementation:
WaitCommEvent()operation;ERROR_OPERATION_ABORTEDas the expected cancellation result;-1witherrno = EIO;transferred, following POSIX-style short-read semantics.
The comments for
CreateEvent(..., TRUE, ...)were also corrected to identifythe events as manual-reset events.
Fixes #3452.
Rationale
A read operation which has already transferred one or more bytes should return
the byte count as a successful short read.
Returning a positive byte count together with an error in
errnowould notprovide useful portable semantics, since callers normally inspect
errnoonlywhen the primary return value indicates failure.
The caller remains responsible for validating protocol framing, terminators and
expected response lengths.
The cancellation path also waits for the pending overlapped operation to
complete before resetting and reusing the associated
OVERLAPPEDstructure andevent.
Testing
Tested on Windows x86_64 with:
nutdrv_qx0.53;COM4.The revised driver and shared libraries were built locally and executed through
libtool --mode=executeto ensure that the newly compiled libraries were used.Observed result:
A subsequent poll produced another successful 47-byte short read, confirming
that the behavior was repeatable.
The affected driver and shared libraries compiled successfully.
The source change also passed:
AI disclosure
ChatGPT was used to assist with diagnosing the Win32 timeout behavior,
reviewing the overlapped cancellation and short-read semantics.
The code was manually reviewed, compiled and tested against real hardware by
the contributor.
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.
Use of coding helper tools and AI should be disclosed in the commit
or PR comments (it is interesting to know which ones do a decent job).
As with other contributions, a human is responsible and thanked for the
quality and content of the change, and is presumed to have the right to
post that code to be published further under the project's license terms.
Especially with involvement of AI, including modern IDE coding aid,
please be sure to revise that proposed code and documentation changes
follow NUT code style guide -- this helps portability across the decades
worth of supported systems. Notably, avoid Unicode characters where ASCII
text is expected (C sources and headers, manual pages and other
acsiidocinputs). Particularly AI is keen on adding
mdashcharacters instead ofplain ASCII double-dash (which renders into the long dash where applicable).
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 othercase.
Did not extend obsoleted drivers with new hardware support features
(notably
blazerand other single-device family drivers for Qx protocols,except the new
nutdrv_qxwhich should cover them all).For updated existing device drivers, bumped the
DRIVER_VERSIONmacroor 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.hwdbfileProposed NUT data mapping is aligned with existing
docs/nut-names.txtfile. If the device exposes useful data points not listed in the file, the
experimental.*namespace can be used as documented there, and discussionshould be raised on the NUT Developers mailing list to standardize the new
concept.
Updated
data/driver.list.inif 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
intwherelanguage or libraries dictate the use of
size_t(orssize_tsometimes).Progress and errors are handled with
upsdebugx(),upslogx(),fatalx()and related methods, not with directprintf()orexit().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.txtfile.For newly added files, the
Makefile.amrecipes were updated and themake distchecktarget passes.General documentation updates
Added a bullet point into
NEWS.adoc, possibly alsoUPGRADING.adocif there is something packagers or custom-build users should take into
account (new driver categories, configuration options, dependencies...)
Updated
docs/acknowledgements.txt(for vendor-backed device support)Added or updated manual page information in
docs/man/*.txtfilesand corresponding recipe lists in
docs/man/Makefile.amfor new pagesPassed
make spellcheck, updated spell-checking dictionary in thedocs/nut.dictfile if needed (did not remove any words -- themakerule 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.