Hello Andy, On Fri, Feb 21, 2020 at 10:42:29AM +0200, Andy Shevchenko wrote: > On Thu, Feb 20, 2020 at 4:01 PM Uwe Kleine-König > <u.kleine-koenig@xxxxxxxxxxxxxx> wrote: > > > > Also I don't understand yet, what you want me to do. > > > > > > I have issues with kstrto() not playing with simple and single numbers > > > (boolean is a special case, but still a number at the end). > > > > A dev_t is also a number in the end. > > My point (when I added single) is 1:1 map. dev_t is not. I don't agree. The mapping is quite similar, the only difference is that you cannot write 47:11 in C source code to get an instance of dev_t. (But you can write MKDEV(47, 11) which is close but IMHO unsuitable for the sysfs interface.) Other than that it's just that kstrtoul does "49283083" -> 10111100000000000000001011 while kstrtodev_t does "47:11" -> 10111100000000000000001011. (nitpick: it's both not 1:1 as "0x2f0000b" maps to the same as "49283083", but ...) > > > In b) case, add to the commit message how many potential _existing_ > > > users may be converted to this. > > > > <sarcasm>Will use 9f6158946987a5ce3f16da097d18f240a89db417 as a good > > example how to do that.</sarcasm> > > I didn't get it. There are _existing_ users in the kernel for that > functionality, At least two are using it right now. Yeah, this was just me being grumpy about "add to the commit message how many potential _existing_ users may be converted". See the output of git grep '\<int_pow\>' 9f6158946987a5ce3f16da097d18f240a89db417^ . > > [...] > > I think what is needed here to satisfy us both is a set of functions like: > > > > int buftoul(const char *buf, char **endp, unsigned long *result) > > > > which does proper range checking (by not consuming chars that are too > > much) and still provides an endp pointer that allows to make use of this > > function to parse types that are represented by more than a plain > > integer. > > Yeah, https://xkcd.com/927/. With the difference that if we introduce a new standard we can effectively kill the older ones. And that you now work towards undeprecating simple_str* seems to confirm that we don't have the one standard to rule them all yet. ===== So, I'm trying to summarize the things we agree about and our differences to maybe help finding an agreement. Trying to be objective until the ==== below. I think we agree about: - The dev_t specification provided by a user via sysfs should be parsed in a strict way. - A helper that takes a string as argument and yields a dev_t or an error is wanted. The points we don't agree about yet are: a) naming of the function Uwe: It fits well into kstrto*(), so kstrtodev_t() Andy: It doesn't fit and feels like a layer violation b) Where to put the function Uwe: Put it into a global place for others to find Andy: Put it near the (for now) single user. (not sure this is really your position here) c) Helpers used to implement the str-to-dev_t helper Uwe: calling the already existing _parse_integer twice is fine Andy: let's create a helper that parses two integers with a given separator ==== I don't feel very strong about b), and could live with putting it near the led trigger until a new user appears. Concerning a) I still think it should have a name that should be obvious enough that a potential new user finds it. And given that kstrto* already contains functions converting strings to a given type this feels right for me. Andy didn't suggest a definitive name, only string_* namespace. This is quite crowded, the best representatives are probably the ones declared in include/linux/string_helpers.h. I looked a bit around for potential users of str-to-dev_t and parse-two-integers. I found none for str-to-dev_t and only dname_to_vma_addr() for the parse-two-integers helper. (But for parse-two-integers the problem might be that I missed some as I don't have a good idea how to grep for these.) dname_to_vma_addr() takes a string in format "%lx-%lx", interprets the numbers as base16 without 0x prefix and leading zeros and doesn't accept a trailing \n. Sounds like a quest for someone being really motivated to cover both (i.e. dname_to_vma_addr() and str-to-dev_t) in a single versatile function. Another way out would be to not take a dev_t from user-space to determine the tty, but a name and use tty_dev_name_to_number() to get the dev_t. (Which would add a second user to this globally exported function. (The only other user is in staging.) :-) I don't know how we can find an agreement, maybe we'd need some input by someone else? There are quite some people who get this mail, maybe someone read 'til here and cares enough to comment? Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
