checkpatch potential false positive

checkpatch potential false positive

[Tobin C. Harding]

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.

Re: checkpatch potential false positive

[Andy Whitcroft]

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

Re: checkpatch potential false positive

[Joe Perches]

On Mon, 2017-11-06 at 08:33 +0000, Andy Whitcroft wrote:
> On Mon, Nov 06, 2017 at 03:19:14PM +1100, Tobin C. Harding wrote:
> > Hi,

Hello.

> > 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?

CHECKs aren't enabled by default except for a few
directories and this warning is much more commonly
correct than incorrect.

checkpatch will always have both false positives and
false negatives.  It's stupid, people generally aren't.

Just ignore checkpatch bleats that aren't appropriate.

Re: checkpatch potential false positive

[Tobin C. Harding]

On Mon, Nov 06, 2017 at 07:29:18AM -0800, Joe Perches wrote:
> On Mon, 2017-11-06 at 08:33 +0000, Andy Whitcroft wrote:
> > On Mon, Nov 06, 2017 at 03:19:14PM +1100, Tobin C. Harding wrote:
> > > Hi,
> 
> Hello.
> 
> > > 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?
> 
> CHECKs aren't enabled by default except for a few
> directories and this warning is much more commonly
> correct than incorrect.

Ok, thanks.

> checkpatch will always have both false positives and
> false negatives.  It's stupid, people generally aren't.
> 
> Just ignore checkpatch bleats that aren't appropriate.

Got it, cheers Andy.