Hi
On Monday, April 16, 2012 4:27 PM Michał Nazarewicz wrote:
> On Tue, 27 Mar 2012 12:48:55 +0200, Andrzej Pietrasiewicz <andrzej.p@xxxxxxxxxxx> wrote:
>
> > +static ssize_t functionfs_user_functions_show(struct device *_dev,
> > + struct device_attribute *attr,
> > + char *buf)
> > +{
> > + struct android_dev *dev = _android_dev;
> > + char *buff = buf;
> > + int i;
> > +
> > + mutex_lock(&dev->mutex);
> > +
> > + for (i = 0; i < dev->max_func_num; i++)
> > + buff += sprintf(buff, "%s,", dev->ffs_tab[i].name);
>
> buff += snprintf(buff, buf + PAGE_SIZE - buff, "%s,", dev->ffs_tab[i].name);
>
> > +
> > + mutex_unlock(&dev->mutex);
> > +
> > + if (buff != buf)
> > + *(buff - 1) = '\n';
> > + return buff - buf;
> > +}
> > +
> > +static ssize_t functionfs_user_functions_store(struct device *_dev,
> > + struct device_attribute *attr,
> > + const char *buff, size_t size)
> > +{
> > + struct android_dev *dev = _android_dev;
> > + char *name, *b;
> > +
> > + strncpy(func_names_buf, buff, sizeof(func_names_buf));
>
> This dose not necessarily include terminating NUL byte. You should use
> strlcpy().
>
> > + b = strim(func_names_buf);
>
> Here you are overwriting existing names with (possibly) garbage. The
> func_names_buf should be dynamically allocated rather then a static
> array used to store names.
>
> > +
> > + mutex_lock(&dev->mutex);
> > +
> > + if (dev->enabled) {
> > + mutex_unlock(&dev->mutex);
> > + return -EBUSY;
> > + }
> > +
> > + while (b) {
> > + name = strsep(&b, ",");
> > + if (name) {
>
> strsep() always returns non-NULL (if b is not NULL).
>
> > + dev->ffs_tab[dev->max_func_num++].name = name;
>
> kstrdup().
>
> > + if (dev->max_func_num == GFS_MAX_DEVS)
> > + break;
> > + }
> > + }
> > + mutex_unlock(&dev->mutex);
> > + return size;
>
> Since this adds new functions, how about just letting user specify a single
> function at a time? Ie. instead of:
>
> echo foo,bar >/sys/...
>
> one would have to do:
>
> echo foo >/sys/...; echo bar >/sys/...
>
> This would make this function simpler.
I'm not sure if creating a state machine for the user interface is a good idea. Multiple
writes are not the most intuitive way of configuring the device. Setting configuration
should be done atomically with a single write call.
Best regards
--
Marek Szyprowski
Samsung Poland R&D Center
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
