Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Mon, 6 Nov 2017 08:33:36 +0000
From:   Andy Whitcroft <apw@canonical.com>
To:     "Tobin C. Harding" <me@tobin.cc>
Cc:     Joe Perches <joe@perches.com>, linux-kernel@vger.kernel.org
Subject: Re: checkpatch potential false positive
Message-ID: <20171106083336.ccz5ht77iegqrd6d@brain>
References: <20171106041914.GB18478@eros>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20171106041914.GB18478@eros>
User-Agent: NeoMutt/20170609 (1.8.3)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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

From 1583288982307073975@xxx Mon Nov 06 04:20:37 +0000 2017
X-GM-THRID: 1583288982307073975
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread