-
Notifications
You must be signed in to change notification settings - Fork 142
Proxy fixes and logging clarity improvements #162
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
It would be great to split these changes into separate commits with separate descriptions of what each change does. But I'll leave reviewing this to the Facedancer developers. |
antoinevg
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great submission, I have a few questions in my review comments but mostly it's just nit-picks. Thank you!
| from .logging import log | ||
| from .request import USBControlRequest | ||
| from .types import USB | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please remove any code format changes that are not directly related to behavioral changes. e.g. whitespace, new-lines, small-fixes, spelling etc.
They make it harder to see what's going on and can open up discussions best had in another context. 🥹
One way to strip them all out can be found here: https://gist.github.com/hello-josh/dbabb56e1a01ac293b83154a6f4a208e
| class USBProxyDevice(USBBaseDevice): | ||
| """ USB Proxy Device """ | ||
| """USB Proxy Device""" | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whitespace
| if len(usb_devices) <= index: | ||
| raise DeviceNotFoundError(f"Could not find device to proxy.") | ||
| raise DeviceNotFoundError("Could not find device to proxy.") | ||
| device = usb_devices[index] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unrelated change
| interface.number: interface | ||
| for interface in configuration.get_interfaces() | ||
| if interface.alternate == 0 | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whitespace
| endpoint = self.configuration.get_endpoint(ep_num, USBDirection.IN) | ||
| if endpoint is None: | ||
| return | ||
| Typically, this method will delegate the request to the appropriate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason you removed this method?
| """libusb1 recommends always using this instead of sending control packets.""" | ||
| log.info(f"LibUSB1Device setInterface {interface} alt {alt}") | ||
| cls._handle().setInterfaceAltSetting(interface, alt) | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
| cls._claim() | ||
|
|
||
| cls._handle().setAutoDetachKernelDriver(1) | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
| from .filters.logging import USBProxyPrettyPrintFilter | ||
| from .filters.standard import USBProxySetupFilters | ||
| from .filters.logging import USBProxyPrettyPrintFilter | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whitespace
| async def configure_logging(): | ||
| import logging | ||
|
|
||
| logging.getLogger("facedancer").setLevel(logging.INFO) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whitespace
| logging.getLogger("facedancer").setLevel(logging.INFO) | ||
|
|
||
| from facedancer import main | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whitespace
While working on the raw_gadget backend I was mostly using the proxy device. However there were several issues with it that prevented it from working. This fixes those.
It also fixes a few things that were not correct according to the libusb docs, like forwarding SET_INTERFACE to the real device - it should be handled specially.
There are also some suggestions for logging to make it more clear and have a level where it's less verbose.