On 3/18/24 8:33 AM, Janne Grunau wrote:
+static int usb_blocklist_parse_error(const char *blocklist, size_t pos) +{
- printf("usb_blocklist parse error at char %zu in "%s"\n", pos,
blocklist);- return 0;
This could be static void without return 0 at the end.
the return is there to break out of the while loop on parsing errors in a single statement. This probably won't be necessary after using strsep and sscanf in the parsing function but see below.
Ahh, now I see it. But then, shouldn't this return -ENODEV here already ?
It returns 0 so that parsing errors in the blocklist do not result in blocking every USB device. That looked to me like the less bad error behavior to me.
In unix , 0 is considered success and non-zero failure .
How about this:
- Return -EINVAL here (parsing failed) - Instead of 'return 1' in usb_device_is_blocked() return -ENODEV - In usb_select_config(), check if usb_device_is_blocked returned 0, no device blocked OK if usb_device_is_blocked returned -ENODEV, device blocked, return -ENODEV if usb_device_is_blocked returned any other error, parsing error return that error
What do you think ?
+}
+static int usb_device_is_blocked(u16 id_vendor, u16 id_product) +{
- ulong vid, pid;
- char *end;
- const char *block_str = env_get("usb_blocklist");
- const char *cur = block_str;
- /* parse "usb_blocklist" strictly */
- while (cur && cur[0] != '\0') {
Have a look at strsep() , namely strsep(block_str, ","); This will split the string up for you at "," delimiters.
Example is in drivers/dfu/dfu.c dfu_config_interfaces() .
strsep() is probably a good idea even if it alone won't make the code that much simpler for strict parsing.
And then, on each token, you can try and run sscanf(token, "%04x:%04x", vid, pid);, that will parse the token format for you. See e.g. test/lib/sscanf.c for further examples.
That should simplify the parsing a lot.
It would but sscanf() is optional and is only selected by CONFIG_XEN so I assumed there would be concerns over binary size increase if USB_HOST would require sscanf.
Good point, lets postpone sscanf() . Also, looking at it, sscanf would work for numbers, but not for the special characters. So ... do you want to try tweaking this further with strsep() or shall we go with this implementation ?
I started converting it to strsep. It makes the intent clearer but it doesn't simplify the implementation much. strsep() has the disadvantage that it can't work in place and needs to copy the string. If we go with strsep I would look into parsing the list once at USB init time and use a list of numeric vendor, product ID pairs at device probe time. If have a slight preference for the current implementation (with ignore or deny instead of blocklist) as long as the list remains short.
OK, we can always improve this later, I would also like this functionality in soon-ish.