On Sun, May 18, 2003 at 08:14:06PM +0400, Alexey Mahotkin wrote:
>
> Two comments on firmware_class_hotplug(), maybe both are irrelevant.
>
> +int firmware_class_hotplug(struct class_device *class_dev, char **envp,
> + int num_envp, char *buffer, int buffer_size)
> +{
> + struct firmware_priv *fw_priv = class_get_devdata(class_dev);
> + int i=0;
> + char *scratch=buffer;
> +
> + if (buffer_size < (FIRMWARE_NAME_MAX+10))
> + return -ENOMEM;
> +
> + envp [i++] = scratch;
> + scratch += sprintf(scratch, "FIRMWARE=%s", fw_priv->fw_id) + 1;
> + return 0;
> +}
> +
>
> First, I do not understand how the environment is handled here.
You can take a look at lib/kobject.c:kset_hotplug()
> You're just setting first element of provided environment to
> "FIRMWARE=%s", possibly overwriting the existing value.
envp points to the first empty slot of the environment being
constructed, so no, I am not overwriting anything.
> Then why are you incrementing `i'? Why are you using `i' at all? Why
> are you incrementing `scratch'?
The code is ready to add a new environment variable if needed. If we
really don't need more variables, it could be simplified, but I don't
think it is so important.
> Ah, it seems like you should be using num_envp somehow, and you're not.
OK, I just added:
if (num_envp < 1)
return -ENOMEM;
> Also, environment pointer list must be terminated with a NULL pointer. Is
> it not done or is that handled somewhere else?
The envp array we get is reset to zero, so anything we don't explicitly
set is already NULL.
> The machine I have 2.5.69 sources is not reachable now so I cannot
> check it. Sorry if I am wrong.
The num_envp issue was real, and anyway, I appreciate a little
feedback, it seams like people don't hack the kernel on Sunday :-P
Thanks
Manuel
--
--- Manuel Estrada Sainz <ranty@debian.org>
<ranty@bigfoot.com>
<ranty@users.sourceforge.net>
------------------------ <manuel.estrada@hispalinux.es> -------------------
Let us have the serenity to accept the things we cannot change, courage to
change the things we can, and wisdom to know the difference.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
This archive was generated by hypermail 2b29 : Fri May 23 2003 - 22:00:31 EST