win32: return partial serial data on timeout

[Marcelo-PX]

Summary

Fix the Win32 serial read wrapper so that data already accumulated before a
timeout is returned to the caller instead of being discarded as an I/O error.

When nutdrv_qx receives a complete Q1 response over a Windows COM port, the
subsequent timeout currently falls through to the generic error path even
though valid bytes were already received.

This change cancels the pending overlapped operation, resets its state, clears
the Win32 error status and returns the accumulated byte count when the timeout
occurs after receiving data.

Fixes #3452.

Testing

Tested on Windows x86_64 with a UPSBrasil 3 kVA UPS using:

• nutdrv_qx
• Q1 protocol
• USB-to-serial adapter on COM4
• NUT 2.8.5 plus the current development branch

Verified:

• the complete Q1 response is accepted without the previous EIO failure;
• continuous polling remains stable;
• upsd, upsc and upsmon receive the UPS data;
• the Windows NUT service starts automatically after reboot;
• communication recovers after disconnecting and reconnecting the USB serial
  adapter without restarting the driver process;
• UPS status changes between OL and OL BYPASS are reported correctly.

The broader ci_build.sh run stopped on an unrelated GCC 16 -Werror warning
in nut-scanner, after the affected driver and libraries had compiled.

AI disclosure

ChatGPT was used to assist with diagnosing the Win32 timeout behavior,
reviewing the proposed change, and drafting the issue and pull request text.

The submitted change 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.

• 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.

• 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.

• Updated data/driver.list.in if applicable.

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, or use of generic
  int where language or libraries dictate the use of size_t or
  ssize_t.

• Progress and errors are handled with upsdebugx(), upslogx(),
  fatalx() and related methods, not with direct printf() or exit().

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

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

General documentation updates

• Added a bullet point into NEWS.adoc, possibly also UPGRADING.adoc
  if there is something packagers or custom-build users should take into
  account.

• Updated docs/acknowledgements.txt.

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

• Passed make spellcheck and updated docs/nut.dict if needed.

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.

• Address NUT CI farm build failures for the PR.

• Revise suggestions from LGTM.COM analysis about new issues.

[github-actions]

A ZIP file with standard source tarball and another tarball with pre-built docs for commit d255dfa is temporarily available: NUT-tarballs-PR-3559.zip.

[AppVeyorBot]

✅ Build nut 2.8.5.5029-master completed (commit ced2ce8edf by @Marcelo-PX)

• artifacts
• A 7-zip archive with built NUT for Windows debug binaries is temporarily available: NUT-for-Windows-x86_64-SNAPSHOT-2.8.5.5029-master.7z

[jimklimov]

I wonder if it is correct to return partial data, without even a hint like setting errno? In case of nutdrv_qx, ASCII \r ending the line (or not) can be a good clue; also expected-length checks, to parse or discard a really short read. I hope other drivers are similarly protected (or should become if not).

[Marcelo-PX]

I wonder if it is correct to return partial data, without even a hint like setting errno? In case of nutdrv_qx, ASCII \r ending the line (or not) can be a good clue; also expected-length checks, to parse or discard a really short read. I hope other drivers are similarly protected (or should become if not).

That makes sense.

In my test with nutdrv_qx, the UPS returned the complete Q1 response including
the final \r, and the driver parsed it correctly. My goal was only to avoid
discarding valid bytes that had already been received on Windows.

But I agree that w32_serial_read() is shared by other serial drivers, so
returning partial data with errno cleared could hide a truncated response.

Would it be better to return the byte count but keep errno = ETIMEDOUT and
SetLastError(WAIT_TIMEOUT), so the caller can detect that the read ended by
timeout? Or do you think this validation should be handled specifically by
nutdrv_qx, checking the final \r or the expected response length?

[jimklimov]

TL;DR: Ultimately, probably what you did is in fact the correct way to do it for the case where tot > 0 (and code already handled if (!tot) just above your addition, although maybe setting non-zero errno there would be right).

I thought returning the byte count and setting the errno (and/or equivalent) would be the most honest way forward. Having said that, I am not sure OTOH how the POSIX systems go about this (if there is some sort of standard on that, possibly with select or similar methods), so I asked AI for a second opinion.

A quick "Chat with GPT" confirmed it also does not know about any precedents directly like what I first suggested either. Portable readers just loop while read() > 0 if they expect more data than a single buffer's worth (or expect to collect a specific packet size), so stopping at EOF or error cases that either way filled zero bytes in the buffer. Quoting from it directly:

POSIX does not define a mechanism whereby read() both returns a positive byte count and reports an error via errno. In fact, the POSIX API is intentionally designed so that a successful return (>= 0) means errno is unspecified and should be ignored.

Specifically, read() returns:

• > 0: that many bytes were read successfully.
• 0: EOF.
• -1: failure, with errno indicating the reason.

POSIX explicitly treats errno as meaningful only if the function indicates failure.

Partial reads are not errors

A partial read is already a normal, expected outcome for many kinds of file descriptors:

• pipes
• terminals
• sockets
• some devices
• interrupted reads
• nonblocking descriptors

The standard already allows

ssize_t n = read(fd, buf, 4096);

to return

127

without implying anything is wrong.

The application is expected to call read() again if it wants more data.

Timeouts

POSIX generally treats timeouts as one of two things:

1. No data transferred

If no bytes have been transferred before the timeout expires, the operation fails:

read() -> -1
errno = EAGAIN

or

ETIMEDOUT

depending on the API and object.

For example, sockets with SO_RCVTIMEO commonly behave this way.

2. Some data transferred

If some bytes have already been copied into the user's buffer, virtually every implementation considers the operation successful and simply returns the byte count.

The timeout is effectively forgotten.

This mirrors how EINTR works.

For example, POSIX says (paraphrasing):

• interrupted before reading anything → return -1, errno=EINTR
• interrupted after reading some bytes → return the bytes read

Timeouts are commonly implemented analogously.

Why not both?

Suppose

read(fd, buf, 4096)

copies

1500 bytes

and then times out.

If it returned

1500
errno = ETIMEDOUT

what should portable code do?

Typical code is

n = read(fd, buf, size);
if (n < 0) {
    perror("read");
}

It never even looks at errno after success.

Changing that convention would break decades of software.

Existing precedents

I can't think of a POSIX interface that returns both

• successful primary result, and
• mandatory secondary error via errno.

There are other APIs that return additional status separately, e.g. recvmsg() returns

• byte count
• flags

where flags may contain

MSG_TRUNC
MSG_CTRUNC
MSG_EOR

These are not reported through errno but extra OUT-args.

If a datagram is larger than the supplied buffer, it returns buffer_size and sets MSG_TRUNC instead of failing with errno=EMSGSIZE.

Similarly,

poll()
select()
epoll_wait()
kevent()

return readiness information separately from errors.

Some APIs have out-parameters for auxiliary status. But POSIX generally avoids "success plus errno".

[jimklimov]

Huh, why closed?

UPDATE: refactored via #3560