request_firmware() hotplug interface, third round and a halve
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're just setting first element of provided environment to "FIRMWARE=%s", possibly overwriting the existing value. Then why are you incrementing i'? Why are you using i' at all? Why are you incrementing `scratch'?
Ah, it seems like you should be using num_envp somehow, and you're not.
Also, environment pointer list must be terminated with a NULL pointer. Is it not done or is that handled somewhere else? The machine I have 2.5.69 sources is not reachable now so I cannot check it. Sorry if I am wrong.
--alexm
Quoted 19 lines · Alexey Mahotkin
Two comments on
firmware_class_hotplug(), maybe both are irrelevant.(quoted diff) +15
-0+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; +} +
Quoted 1 line
First, I do not understand how the environment is handled here.
You can take a look at lib/kobject.c:kset_hotplug()
Quoted 2 lines
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.
Quoted 2 lines
Then why are you incrementing
i'? Why are you usingi' 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.
Quoted 1 line
Ah, it seems like you should be using
num_envpsomehow, and you're not.
OK, I just added:
if (num_envp < 1) return -ENOMEM;
Quoted 2 lines
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.
Quoted 2 lines
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
Signature · 6 lines
--- Manuel Estrada Sainz <[email protected]> <[email protected]> <[email protected]> ------------------------ <[email protected]> ------------------- 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.