On Mon, Nov 06, 2017 at 03:19:14PM +1100, Tobin C. Harding wrote:
> Hi,
>
> When parsing drivers/staging/unisys/visorbus/visorchipset.c in Greg's
> staging tree checkpatch emits
>
> --------------
> visorchipset.c
> --------------
> WARNING: char * array declaration might be better as static const
> #1050: FILE: visorchipset.c:1050:
> + char *envp[] = { env_cmd, env_id, env_state, env_bus, env_dev,
>
> WARNING: char * array declaration might be better as static const
> #1140: FILE: visorchipset.c:1140:
> + char *envp[] = { env_selftest, NULL };
>
> total: 0 errors, 2 warnings, 1694 lines checked
>
> I may be wrong but I think the code in question is clean and
> correct. Since checkpatch is saying this _might_ be better ... perhaps
> checkpatch could emit CHECK instead of WARNING for this?
>
> Here is the complete function to save finding the file.
>
> /*
> * parahotplug_request_kickoff() - initiate parahotplug request
> * @req: the request to initiate
> *
> * Cause uevent to run the user level script to do the disable/enable
> specified
> * in the parahotplug_request.
> */
> static int parahotplug_request_kickoff(struct parahotplug_request *req)
> {
> struct controlvm_message_packet *cmd = &req->msg.cmd;
> char env_cmd[40], env_id[40], env_state[40], env_bus[40], env_dev[40],
> env_func[40];
> char *envp[] = { env_cmd, env_id, env_state, env_bus, env_dev,
> env_func, NULL
> };
>
> sprintf(env_cmd, "VISOR_PARAHOTPLUG=1");
> sprintf(env_id, "VISOR_PARAHOTPLUG_ID=%d", req->id);
> sprintf(env_state, "VISOR_PARAHOTPLUG_STATE=%d",
> cmd->device_change_state.state.active);
> sprintf(env_bus, "VISOR_PARAHOTPLUG_BUS=%d",
> cmd->device_change_state.bus_no);
> sprintf(env_dev, "VISOR_PARAHOTPLUG_DEVICE=%d",
> cmd->device_change_state.dev_no >> 3);
> sprintf(env_func, "VISOR_PARAHOTPLUG_FUNCTION=%d",
> cmd->device_change_state.dev_no & 0x7);
> return kobject_uevent_env(&chipset_dev->acpi_device->dev.kobj,
> KOBJ_CHANGE, envp);
> }
>
> If this is not the sort of bug report you like, could you please (if you
> have time) school me on how to better report such things in the future.
>
> thanks,
> Tobin.
I'll let Joe talk to whether this ought to be a CHECK.
The function there looks to never modify the envp pointer so some kind
of const might well be reasonable as you do not update the pointers.
Right?
-apw
