search

google / gousb

gousb provides low-level interface for accessing USB devices
Apache License 2.0
838 stars 124 forks source link

Zero-length writes are not allowed #71

Closed QuLogic closed 5 years ago

QuLogic commented 5 years ago

In the transfer function, a zero-length buffer is immediately returned without actually sending anything. However, there seem to be valid use cases for this, so the short-circuit breaks certain things.

I am attempting to port yubihsm-connector to gousb from go-libusb, but the lack of empty writes appears to be problematic.

As libusb even includes a flag LIBUSB_TRANSFER_ADD_ZERO_PACKET to automatically send empty writes itself (which is not exposed by gousb), I think there's no reason to disallow them from user code and skipping them from the Go layer is incorrect.

zagrodzki commented 5 years ago

Ok, so if I understand correctly, the only thing needed is the removal of the short-circuit? If an additional zero-length packet is needed after certain transfers (which is what LIBUSB_TRANSFER_ADD_ZERO_PACKET would have added automatically), the users will be able to request it manually by just initiating a zero-length write, so we don't need to worry about implementing some support for that transfer flag. Does that sound correct?

@kylelemons I think the code in question was inherited from the previous versions, https://github.com/google/gousb/blame/master/endpoint.go#L89. Can you think of any reason why we shouldn't allow zero-length transfers here?

@QuLogic FYI, in gousb Read/WriteTimeout is instead replaced with a context-aware read/write, see e.g. https://godoc.org/github.com/google/gousb#OutEndpoint.WriteContext

QuLogic commented 5 years ago

Yes, I believe that is correct. If zero-length writes are allowed, then there's no need to mess with transfer flags (other than convenience, I guess).

I'm aware of the new timeout implementation.

kylelemons commented 5 years ago

I don't think I had any particular reason; it's possible that I assumed nonzero somewhere and added it as a guard, but I'm not sure. Sorry I can't be more helpful :)

On Wed, Jul 31, 2019, 2:29 AM Sebastian Zagrodzki notifications@github.com wrote:

Ok, so if I understand correctly, the only thing needed is the removal of the short-circuit? If an additional zero-length packet is needed after certain transfers (which is what LIBUSB_TRANSFER_ADD_ZERO_PACKET would have added automatically), the users will be able to request it manually by just initiating a zero-length write, so we don't need to worry about implementing some support for that transfer flag. Does that sound correct?

@kylelemons https://github.com/kylelemons I think the code in question was inherited from the previous versions, https://github.com/google/gousb/blame/master/endpoint.go#L89. Can you think of any reason why we shouldn't allow zero-length transfers here?

@QuLogic https://github.com/QuLogic FYI, in gousb Read/WriteTimeout is instead replaced with a context-aware read/write, see e.g. https://godoc.org/github.com/google/gousb#OutEndpoint.WriteContext

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/google/gousb/issues/71?email_source=notifications&email_token=AACOVJITR5UGEYQNWWULNODQCFLPTA5CNFSM4IHW2NV2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD3GVJTI#issuecomment-516773069, or mute the thread https://github.com/notifications/unsubscribe-auth/AACOVJJYRP4UK4IYONIXZCDQCFLPTANCNFSM4IHW2NVQ .

zagrodzki commented 5 years ago

That works for me, I'll send a PR.