From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751540AbdKFIdk (ORCPT ); Mon, 6 Nov 2017 03:33:40 -0500 Received: from youngberry.canonical.com ([91.189.89.112]:39746 "EHLO youngberry.canonical.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750739AbdKFIdj (ORCPT ); Mon, 6 Nov 2017 03:33:39 -0500 Date: Mon, 6 Nov 2017 08:33:36 +0000 From: Andy Whitcroft To: "Tobin C. Harding" Cc: Joe Perches , 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 List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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