[Nut-upsuser] Windows 2.6.0-1 usbhid-ups driver infinite loop on USB disconnect

[David Bolen]

David Bolen <db3l.net at gmail.com> writes:

> Attached below is an excerpt of debugging output from usbhid-ups.  It
> would appear that the interrupt and query failures have reasonably
> appropriate errors, so maybe it's just a difference in how such errors
> reflect under Windows vs. *nix.

Following up on my own note, I think I have a theory on what is
happening, but am having trouble compiling to test a change as while I
have an pre-existing Mingw/MSYS toolchain, it doesn't have a full
autoconf toolchain yet.  If anyone might have a pre-existing
"configure" script for Win32, that would help jump start things as I
could configure from there.  

I think I've run into two items.  The first is that the errors from
checking the interrupt pipe in upsdrv_updateinfo aren't actually
checked for cases where they could identify a disconnected UPS, so it
just assumes no information and continues.  That's not Win32 specific.
I'm not quite sure why it loops so quickly, as opposed to the top
level polltime, but perhaps the state of the driver connection is such
that it immediately satisfies the WaitForMultipleObjects call in the
Win32 version of dstate_poll_fds.

Anyway, normally it looks like that would be short-lived anyway since
the periodic full poll would detect the disconnect a modest time
later.  Here I think I run into an assumption in the libhid code that
errno always identifies all errors, while in the case of a win32
failure inside of libusb-win32, it's only the result code of the
function calls that reflect the error, not errno.  So USB I/O level
errors while polling are getting hidden.

After shrinking the poll time and letting things run a bit after a
cable disconnect, I got a few other errors (from libusb_get_report)
interleaved such as:

   5.843750     Got 0 HID objects...
   5.843750     Quick update...
   5.859375     upsdrv_updateinfo...
   5.859375     libusb_get_interrupt: libusb0-dll:err [submit_async] submitting
request failed, win error: The device does not recognize the command.
   5.859375     Got 0 HID objects...
   5.859375     Quick update...
   5.859375     libusb_get_report: libusb0-dll:err [control_msg] sending control
 message failed, win error: The device does not recognize the command.
   5.859375     Can't retrieve Report 16: No error
   5.859375     libusb_get_report: libusb0-dll:err [control_msg] sending control
 message failed, win error: The device does not recognize the command.
   5.859375     Can't retrieve Report 15: No error
   5.859375     upsdrv_updateinfo...
   5.859375     libusb_get_interrupt: libusb0-dll:err [submit_async] submitting
request failed, win error: The device does not recognize the command.

>From looking at the libusb-win32 source, this would be the result of a
Win32 ERROR_BAD_COMMAND code which is turned into a -EIO result for
the usb_control_message call.  But in this case, it's only the result
that has the error, as it's a translation from the Windows internal
error and not from any original errno value.

It appears that the NUT libusb.c module passes the return code on
fine, but then it's suppressed with libhid.c (refresh_report_buffer),
and eventually HIDGetDataValue, who just gets a generic -1 code,
assumes it can pass along -errno to the usbhid-ups driver, which at
that point has no error (yielding the "No error" in the "Can't
retrieve Report" message.

It seems like adjusting functions along the path to all return the
function result code as opposed to just -1 would work but also feels
like a riskier change.  I'm wondering if perhaps just making the lower
level libusb entry points (probably just #if WIN32) assign the result
code of the libusb-win32 driver calls to errno to ensure it represents
the failures would be cleaner and help obscure the Windows differences?

-- David

PS: In reviewing the libusb-win32 source it also looks like in some
cases it can return either EINVAL or ENOMEM, neither of which are
checked for in the error checking path for usbhid-ups, so probably
could get added, if only to the WIN32 block.