-
Notifications
You must be signed in to change notification settings - Fork 50
snagrecover: improve logging on USB access errors #75
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
f804fb6
to
4cd9975
Compare
When an USB Device can't be accessed, snagrecover prints a generic error message stating to check presence and access right. Improve this by testing if the USB error is a permission error. If so log an error message and some hints on how to solve it. Signed-off-by: François Foltete <[email protected]>
4cd9975
to
9f72262
Compare
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.
Thanks for the contribution! I've requested a few changes.
except usb.USBError as e: | ||
if e.errno == errno.EACCES: | ||
logger.error( | ||
f"USB Device has been found at address {pretty_addr} but can't be accessed because of permission issue." |
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.
s/has been found/was found/
s/of permission issue/of a device file access rights issue/
"Please check your udev config (refer to README.md#Installation on Linux)." | ||
) | ||
logger.error( | ||
"The following udev rule allow access to the USB 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.
s/allow access/grants access/
) | ||
elif sys.platform == "win32": | ||
logger.error( | ||
"Please check your installation (refer to README.md#Installation on Windows 10 or 11)." |
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 needs to be more specific, a USB access rights issue on Windows would likely be due to no appropriate libusb driver being bound to the device.
So:
"Please check that the 'libusb-win32' driver is bound to this USB device ID: {dev.idProduct:04x}:{dev.idVendor:04x}"
"This is usually done with the Zadig tool, please check the Snagboot installation guide for more information."
"The following udev rule allow access to the USB device:" | ||
) | ||
logger.error( | ||
f'SUBSYSTEM=="usb", ATTRS{{idVendor}}=="{dev.idProduct:x}", ATTRS{{idProduct}}=="{dev.idVendor:x}", MODE="0660", TAG+="uaccess"' |
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.
USB vendor/device ID pairs should be formatted with 4 hexadecimal digits and leading zeros, so: {...:04x}
.
) | ||
|
||
if error_on_fail: | ||
# Don't retry if it is a permission issue |
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 will still allow retrying if error_on_fail
is false though.
Moreover, I'm wondering if not retrying is correct or not. After all, the udev rule could take some time to kick in... Therefore, I think we should only run the access rights check on the last retry.
When an USB Device can't be accessed, snagrecover prints a generic error message stating to check presence
and access right.
Improve this by testing if the USB error is a permission error. If so log an error message and some hints
on how to solve it.