On Tue, 17 Apr 2012 09:27:02 +0200, Marek Szyprowski <m.szyprowski@xxxxxxxxxxx> wrote:
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_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;
Also, just noticed, that this check must be before the line above, not here.
> + }
> + }
> + 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.
Problem is that it already implements a state machine as it adds functions.
This also means that it's not that atomic (sure, adding is atomic, but so
what?).
If this function was to replace the list of functions, I'm all with you on
this one, but it adds them.
Also, if user tries to add too many functions, this just breaks at one point
rather then returning some error user can (try) and handle.
Without the whole comma handling thing, the whole function would look as
follows:
struct android_dev *dev = _android_dev;
ssize_t ret = count;
char *name;
buff = skip_space(buff);
if (!*buff)
return -EINVAL;
name = kstrdup(buff);
if (!name)
return -ENOMEM;
name = strim(name);
mutex_lock(&dev->mutex);
if (dev->enabled) {
ret = -EBUSY;
} else if (dev->max_func_num == GFS_MAX_DEVS) {
ret = -ENOSPC;
} else if (functionfs_find_dev(dev, name)) {
ret = -EEXIST;
} else {
dev->ffs_tab[dev->max_func_num++].name = name;
name = NULL;
}
mutex_unlock(&dev->mutex);
kfree(name);
return ret;
which is much cleaner.
--
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
..o | Computer Science, Michał “mina86” Nazarewicz (o o)
ooo +----<email/xmpp: mpn@xxxxxxxxxx>--------------ooO--(_)--Ooo--
--
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
