Messages in this thread | | | From | Kalle Valo <> | Subject | Re: [PATCH RFC] brcmfmac: sanitize DMI strings v2 | Date | Mon, 06 May 2019 15:29:21 +0300 |
| |
Victor Bravo <1905@spmblk.com> writes:
> On Mon, May 06, 2019 at 11:42:06AM +0300, Kalle Valo wrote: >> Hans de Goede <hdegoede@redhat.com> writes: >> >> >> @@ -99,6 +107,15 @@ static const struct dmi_system_id dmi_platform_data[] = { >> >> {} >> >> }; >> >> +void brcmf_dmi_sanitize(char *dst, const unsigned char *allowed, >> >> char safe) >> >> +{ >> >> + while (*dst) { >> >> + if ((*dst < 0) || !(allowed[*dst / 8] & (1 << (*dst % 8)))) >> > >> > At a first look I have no clue what this code is doing and I honestly do not feel >> > like figuring it out, this is clever, but IMHO not readable. >> > >> > Please just write this as if (*dst < 0x21 || (*dst > foo && < bar) || etc, >> > so that a human can actually see in one look what the code is doing. >> >> Is there an existing function for sanitising filenames so that we don't >> need to reinvent the wheel, maybe something like isalnum()? > > I would definitely prefer to use existing function, but I didn't find > any suitable one. Suggestions are welcome.
I didn't find anything either, but hopefully someone knows.
> As for implementation details, the one I posted was optimized for both > speed and size, and at least in my opinion this bit array driven > parametric implementation is exactly what is needed here (using a string > of allowed characters with strchr-style lookups would bring much worse > complexity, and checking the characters using series of hardcoded if > conditions could quickly grow to more than those 16 bytes used by the > array).
But is this really something which should be optimised? This is driver initialisation, not in some hot path, right? Can you even measure the difference?
-- Kalle Valo
| |