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.
