From: Markus Elfring <[email protected]>
Date: Sat, 20 Aug 2016 18:35:43 +0200
A few update suggestions were taken into account
from static source code analysis.
Markus Elfring (3):
Use memdup_user()
Delete an unnecessary jump label
Delete unnecessary variable initialisations
.../net/wireless/intersil/hostap/hostap_ioctl.c | 36 ++++++++--------------
1 file changed, 12 insertions(+), 24 deletions(-)
--
2.9.3
On Mon, 2016-08-22 at 22:50 +0200, SF Markus Elfring wrote:
> > @@ -2289,6 +2299,10 @@ sub process {
> > }
> >
> > if ($found_file) {
> > + if (is_maintained_obsolete($realfile)) {
> > + WARN("OBSOLETE",
> > + "$realfile is marked as 'obsolete' in the MAINTAINERS hierarchy. No unnecessary modifications please.\n");
> > + }
> How do you think about to avoid a double negation in such a warning message?
>
> Would a wording like "… Only really necessary modifications please.\n"
> be more useful here?
No, probably not.
On 20-08-16 18:43, SF Markus Elfring wrote:
> From: Markus Elfring <[email protected]>
> Date: Sat, 20 Aug 2016 18:35:43 +0200
>
> A few update suggestions were taken into account
> from static source code analysis.
Is it worth touching this old stuff especially when you are not making
any functional changes.
Regards,
Arend
> Markus Elfring (3):
> Use memdup_user()
> Delete an unnecessary jump label
> Delete unnecessary variable initialisations
>
> .../net/wireless/intersil/hostap/hostap_ioctl.c | 36 ++++++++--------------
> 1 file changed, 12 insertions(+), 24 deletions(-)
>
Hi Marcus,
On Sun, Aug 21, 2016 at 2:46 AM, SF Markus Elfring
<[email protected]> wrote:
> From: Markus Elfring <[email protected]>
> Date: Sat, 20 Aug 2016 18:21:29 +0200
>
> Remove a jump label which is unneeded in this function at the end.
>
> Signed-off-by: Markus Elfring <[email protected]>
> ---
> drivers/net/wireless/intersil/hostap/hostap_ioctl.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/wireless/intersil/hostap/hostap_ioctl.c b/drivers/net/wireless/intersil/hostap/hostap_ioctl.c
> index 4e271f9..5942917 100644
> --- a/drivers/net/wireless/intersil/hostap/hostap_ioctl.c
> +++ b/drivers/net/wireless/intersil/hostap/hostap_ioctl.c
> @@ -3835,14 +3835,12 @@ static int prism2_ioctl_priv_hostapd(local_info_t *local, struct iw_point *p)
> }
>
> if (ret == 1 || !ap_ioctl) {
> - if (copy_to_user(p->pointer, param, p->length)) {
> + if (copy_to_user(p->pointer, param, p->length))
> ret = -EFAULT;
> - goto out;
> - } else if (ap_ioctl)
> + else if (ap_ioctl)
> ret = 0;
> }
>
> - out:
Does this change make any difference to the compiled code?
Thanks,
--
Julian Calaby
Email: [email protected]
Profile: http://www.google.com/profiles/julian.calaby/
From: Markus Elfring <[email protected]>
Date: Sat, 20 Aug 2016 18:19:43 +0200
Reuse existing functionality from memdup_user() instead of keeping
duplicate source code.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring <[email protected]>
---
.../net/wireless/intersil/hostap/hostap_ioctl.c | 22 ++++++----------------
1 file changed, 6 insertions(+), 16 deletions(-)
diff --git a/drivers/net/wireless/intersil/hostap/hostap_ioctl.c b/drivers/net/wireless/intersil/hostap/hostap_ioctl.c
index 3e5fa78..4e271f9 100644
--- a/drivers/net/wireless/intersil/hostap/hostap_ioctl.c
+++ b/drivers/net/wireless/intersil/hostap/hostap_ioctl.c
@@ -3041,14 +3041,9 @@ static int prism2_ioctl_priv_download(local_info_t *local, struct iw_point *p)
p->length > 1024 || !p->pointer)
return -EINVAL;
- param = kmalloc(p->length, GFP_KERNEL);
- if (param == NULL)
- return -ENOMEM;
-
- if (copy_from_user(param, p->pointer, p->length)) {
- ret = -EFAULT;
- goto out;
- }
+ param = memdup_user(p->pointer, p->length);
+ if (IS_ERR(param))
+ return PTR_ERR(param);
if (p->length < sizeof(struct prism2_download_param) +
param->num_areas * sizeof(struct prism2_download_area)) {
@@ -3803,14 +3798,9 @@ static int prism2_ioctl_priv_hostapd(local_info_t *local, struct iw_point *p)
p->length > PRISM2_HOSTAPD_MAX_BUF_SIZE || !p->pointer)
return -EINVAL;
- param = kmalloc(p->length, GFP_KERNEL);
- if (param == NULL)
- return -ENOMEM;
-
- if (copy_from_user(param, p->pointer, p->length)) {
- ret = -EFAULT;
- goto out;
- }
+ param = memdup_user(p->pointer, p->length);
+ if (IS_ERR(param))
+ return PTR_ERR(param);
switch (param->cmd) {
case PRISM2_SET_ENCRYPTION:
--
2.9.3
From: Markus Elfring <[email protected]>
Date: Sat, 20 Aug 2016 18:23:14 +0200
The local variable "ret" will be set to an appropriate value a bit later.
Thus omit the explicit initialisation at the beginning of four functions.
Signed-off-by: Markus Elfring <[email protected]>
---
drivers/net/wireless/intersil/hostap/hostap_ioctl.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/net/wireless/intersil/hostap/hostap_ioctl.c b/drivers/net/wireless/intersil/hostap/hostap_ioctl.c
index 5942917..c37b0bb 100644
--- a/drivers/net/wireless/intersil/hostap/hostap_ioctl.c
+++ b/drivers/net/wireless/intersil/hostap/hostap_ioctl.c
@@ -2895,7 +2895,7 @@ static int prism2_ioctl_priv_monitor(struct net_device *dev, int *i)
{
struct hostap_interface *iface;
local_info_t *local;
- int ret = 0;
+ int ret;
u32 mode;
iface = netdev_priv(dev);
@@ -3035,7 +3035,7 @@ static int ap_mac_cmd_ioctl(local_info_t *local, int *cmd)
static int prism2_ioctl_priv_download(local_info_t *local, struct iw_point *p)
{
struct prism2_download_param *param;
- int ret = 0;
+ int ret;
if (p->length < sizeof(struct prism2_download_param) ||
p->length > 1024 || !p->pointer)
@@ -3791,7 +3791,7 @@ static int prism2_ioctl_scan_req(local_info_t *local,
static int prism2_ioctl_priv_hostapd(local_info_t *local, struct iw_point *p)
{
struct prism2_hostapd_param *param;
- int ret = 0;
+ int ret;
int ap_ioctl = 0;
if (p->length < sizeof(struct prism2_hostapd_param) ||
@@ -3954,7 +3954,7 @@ int hostap_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
struct iwreq *wrq = (struct iwreq *) ifr;
struct hostap_interface *iface;
local_info_t *local;
- int ret = 0;
+ int ret;
iface = netdev_priv(dev);
local = iface->local;
--
2.9.3
> @@ -2289,6 +2299,10 @@ sub process {
> }
>
> if ($found_file) {
> + if (is_maintained_obsolete($realfile)) {
> + WARN("OBSOLETE",
> + "$realfile is marked as 'obsolete' in the MAINTAINERS hierarchy. No unnecessary modifications please.\n");
> + }
How do you think about to avoid a double negation in such a warning message?
Would a wording like "… Only really necessary modifications please.\n"
be more useful here?
Regards,
Markus
From: Markus Elfring <[email protected]>
Date: Sat, 20 Aug 2016 18:21:29 +0200
Remove a jump label which is unneeded in this function at the end.
Signed-off-by: Markus Elfring <[email protected]>
---
drivers/net/wireless/intersil/hostap/hostap_ioctl.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/net/wireless/intersil/hostap/hostap_ioctl.c b/drivers/net/wireless/intersil/hostap/hostap_ioctl.c
index 4e271f9..5942917 100644
--- a/drivers/net/wireless/intersil/hostap/hostap_ioctl.c
+++ b/drivers/net/wireless/intersil/hostap/hostap_ioctl.c
@@ -3835,14 +3835,12 @@ static int prism2_ioctl_priv_hostapd(local_info_t *local, struct iw_point *p)
}
if (ret == 1 || !ap_ioctl) {
- if (copy_to_user(p->pointer, param, p->length)) {
+ if (copy_to_user(p->pointer, param, p->length))
ret = -EFAULT;
- goto out;
- } else if (ap_ioctl)
+ else if (ap_ioctl)
ret = 0;
}
- out:
kfree(param);
return ret;
}
--
2.9.3
On Tue, 23 Aug 2016, SF Markus Elfring wrote:
> > Use get_maintainer to check the status of individual files.
> > If "obsolete", suggest leaving the files alone.
>
> Will another software system like the "kbuild test robot"
> need any more fine-tuning for this change?
It only works on files in which there have been commits, thus by
definition not obsolete.
julia
>
> Regards,
> Markus
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
On Mon, 2016-08-22 at 18:49 +0300, Kalle Valo wrote:
> Arend van Spriel <[email protected]> writes:
[]
> But yeah, not really sure what to do with these obsolete drivers like
> hostap, ray_cs and wl3501.
Maybe marking sections obsolete in MAINTAINERS could
flag some "shouldn't touch this" warning for old code
in checkpatch.pl and/or get_maintainer.pl
Arend van Spriel <[email protected]> writes:
> On 20-08-16 18:43, SF Markus Elfring wrote:
>> From: Markus Elfring <[email protected]>
>> Date: Sat, 20 Aug 2016 18:35:43 +0200
>>
>> A few update suggestions were taken into account
>> from static source code analysis.
>
> Is it worth touching this old stuff especially when you are not making
> any functional changes.
On the other hand if these patches break something this might be a good
way to get feedback if someone is really using this driver ;)
But yeah, not really sure what to do with these obsolete drivers like
hostap, ray_cs and wl3501.
--
Kalle Valo
Use get_maintainer to check the status of individual files.
If "obsolete", suggest leaving the files alone.
Signed-off-by: Joe Perches <[email protected]>
---
scripts/checkpatch.pl | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 4de3cc4..df5e9d9 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -704,6 +704,16 @@ sub seed_camelcase_file {
}
}
+sub is_maintained_obsolete {
+ my ($filename) = @_;
+
+ return 0 if (!(-e "$root/scripts/get_maintainer.pl"));
+
+ my $status = `perl $root/scripts/get_maintainer.pl --status --nom --nol --nogit --nogit-fallback $filename 2>&1`;
+
+ return $status =~ /obsolete/i;
+}
+
my $camelcase_seeded = 0;
sub seed_camelcase_includes {
return if ($camelcase_seeded);
@@ -2289,6 +2299,10 @@ sub process {
}
if ($found_file) {
+ if (is_maintained_obsolete($realfile)) {
+ WARN("OBSOLETE",
+ "$realfile is marked as 'obsolete' in the MAINTAINERS hierarchy. No unnecessary modifications please.\n");
+ }
if ($realfile =~ m@^(?:drivers/net/|net/|drivers/staging/)@) {
$check = 1;
} else {
--
2.8.0.rc4.16.g56331f8
> Use get_maintainer to check the status of individual files.
> If "obsolete", suggest leaving the files alone.
Will another software system like the "kbuild test robot"
need any more fine-tuning for this change?
Regards,
Markus
SF Markus Elfring <[email protected]> wrote:
> From: Markus Elfring <[email protected]>
> Date: Sat, 20 Aug 2016 18:21:29 +0200
>
> Remove a jump label which is unneeded in this function at the end.
>
> Signed-off-by: Markus Elfring <[email protected]>
2 patches set to Rejected.
9291771 [2/3] hostap: Delete an unnecessary jump label in prism2_ioctl_priv_hostapd()
9291775 [3/3] hostap: Delete unnecessary initialisations for the variable "ret"
Reason: The benefit is not clear.
--
https://patchwork.kernel.org/patch/9291771/
Documentation about submitting wireless patches and checking status
from patchwork:
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
SF Markus Elfring <[email protected]> wrote:
> From: Markus Elfring <[email protected]>
> Date: Sat, 20 Aug 2016 18:19:43 +0200
>
> Reuse existing functionality from memdup_user() instead of keeping
> duplicate source code.
>
> This issue was detected by using the Coccinelle software.
>
> Signed-off-by: Markus Elfring <[email protected]>
Patch set to Rejected.
[1/3] hostap: Use memdup_user() rather than duplicating i... 2016-08-20 SF Markus El Rejected
Reason: A similar patch is already applied.
Applying: hostap: Use memdup_user() rather than duplicating its implementation
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging drivers/net/wireless/intersil/hostap/hostap_ioctl.c
CONFLICT (content): Merge conflict in drivers/net/wireless/intersil/hostap/hostap_ioctl.c
Failed to merge in the changes.
Patch failed at 0001 hostap: Use memdup_user() rather than duplicating its implementation
--
https://patchwork.kernel.org/patch/9306999/
Documentation about submitting wireless patches and checking status
from patchwork:
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches