Messages in this thread | | | Date | Mon, 19 Jun 2017 09:09:26 +0100 | From | Okash Khawaja <> | Subject | Re: [patch v2 1/3] tty: add function to convert device name to number |
| |
On Sun, Jun 18, 2017 at 04:27:42PM +0300, Andy Shevchenko wrote: > This doesn't have actual parameter name. > Btw, I would drop dev_ suffix completely from parameter (you have it > in function name). Good call, thanks.
> > + * Locking: this acquires tty_mutex > > ...and releases. > > Perhaps it makes sense to describe what it protects (like it's done in > some functions around). Yes, will add the description
> > + */ > > +int tty_dev_name_to_number(char *dev_name, dev_t *dev_no) > > const char *name, right? Yes :)
> > + int rv, index, prefix_length = 0; > > I would keep returned variable on a separate line and name it like > other functions do in this file, i.e. ret. > > int ret; Sure
> > + while (!isdigit(*(dev_name + prefix_length)) && prefix_length < > > + strlen(dev_name) ) > > + prefix_length++; > > + > > + if (prefix_length == strlen(dev_name)) > > + return -EINVAL; > > Basically, what you need is to get tailing digits, right? > > Moreover, there is quite similar piece of code in > tty_find_polling_driver() you may share. tty_find_polling_driver does something slightly different. It looks for digits embedded in a string, so something like kgdboc=ttyS0,115200 where the digit being sought is 0 after ttyS. So the sanity checks aren't the same. It also looks for a comma in addition to looking for a number, which doesn't apply here. Little bit of functionality may be factored out but that will be too trivial.
While skimming through tty_find_polling_driver, I also noticed that, in a strict sense, the following check may not be sufficient when name is prefix of p->name.
if (strncmp(name, p->name, len) != 0)
> > + if (rv) { > > > + mutex_unlock(&tty_mutex); > > + return rv; > > I would go with goto style in this function (since it has locking involved). Sure > > > + } > > All together kstrtoint() is invariant here as far as I can see and can > be done out of locking. Also see above comment how to get line index. Good call, thanks. Think I changed the code afterwards but didn't notice that kstrtoint no longer needs to be inside the loop and lock.
Best regards, Okash
| |