From: Markus Elfring <[email protected]>
Further update suggestions were taken into account after a patch was applied
from static source code analysis.
Markus Elfring (2):
Delete unnecessary checks before two function calls
One function call less in mac_ioctl() after error detection
drivers/staging/wilc1000/linux_wlan.c | 17 +++++------------
1 file changed, 5 insertions(+), 12 deletions(-)
--
2.4.4
On Sat, 27 Jun 2015, SF Markus Elfring wrote:
> From: Markus Elfring <[email protected]>
> Date: Sat, 27 Jun 2015 16:00:59 +0200
>
> The kfree() function was called in two cases by the mac_ioctl() function
> during error handling even if the passed variable did not contain a pointer
> for a valid data item.
>
> * This implementation detail could be improved by the introduction
> of another jump label.
>
> * Drop an unnecessary initialisation for the variable "buff" then.
>
> Signed-off-by: Markus Elfring <[email protected]>
> ---
> drivers/staging/wilc1000/linux_wlan.c | 10 ++++------
> 1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/staging/wilc1000/linux_wlan.c b/drivers/staging/wilc1000/linux_wlan.c
> index 2aa8d9b..f492310 100644
> --- a/drivers/staging/wilc1000/linux_wlan.c
> +++ b/drivers/staging/wilc1000/linux_wlan.c
> @@ -2351,16 +2351,13 @@ int mac_close(struct net_device *ndev)
>
> int mac_ioctl(struct net_device *ndev, struct ifreq *req, int cmd)
> {
> -
> - u8 *buff = NULL;
> + u8 *buff;
> s8 rssi;
> u32 size = 0, length = 0;
> perInterface_wlan_t *nic;
> struct WILC_WFI_priv *priv;
> s32 s32Error = WILC_SUCCESS;
>
> -
> -
Removing these blank lines seems to have nothing to do with the topic of
the patch.
julia
> /* struct iwreq *wrq = (struct iwreq *) req; // tony moved to case SIOCSIWPRIV */
> #ifdef USE_WIRELESS
> nic = netdev_priv(ndev);
> @@ -2405,7 +2402,7 @@ int mac_ioctl(struct net_device *ndev, struct ifreq *req, int cmd)
> if (copy_to_user(wrq->u.data.pointer, buff, size)) {
> PRINT_ER("%s: failed to copy data to user buffer\n", __func__);
> s32Error = -EFAULT;
> - goto done;
> + goto free_buffer;
> }
> }
> }
> @@ -2420,8 +2417,9 @@ int mac_ioctl(struct net_device *ndev, struct ifreq *req, int cmd)
> }
> }
>
> -done:
> +free_buffer:
> kfree(buff);
> +done:
> return s32Error;
> }
>
> --
> 2.4.4
>
> --
> 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
>
From: Markus Elfring <[email protected]>
Date: Sat, 27 Jun 2015 15:56:57 +0200
The functions kfree() and release_firmware() test whether their argument
is NULL and then return immediately.
Thus the test around the calls is not needed.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring <[email protected]>
---
drivers/staging/wilc1000/linux_wlan.c | 9 ++-------
1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/drivers/staging/wilc1000/linux_wlan.c b/drivers/staging/wilc1000/linux_wlan.c
index b352c50..2aa8d9b 100644
--- a/drivers/staging/wilc1000/linux_wlan.c
+++ b/drivers/staging/wilc1000/linux_wlan.c
@@ -2421,11 +2421,7 @@ int mac_ioctl(struct net_device *ndev, struct ifreq *req, int cmd)
}
done:
-
- if (buff != NULL) {
- kfree(buff);
- }
-
+ kfree(buff);
return s32Error;
}
@@ -2707,8 +2703,7 @@ static void __exit exit_wilc_driver(void)
}
}
-
- if ((g_linux_wlan != NULL) && g_linux_wlan->wilc_firmware != NULL) {
+ if (g_linux_wlan) {
release_firmware(g_linux_wlan->wilc_firmware);
g_linux_wlan->wilc_firmware = NULL;
}
--
2.4.4
From: Markus Elfring <[email protected]>
Date: Sat, 27 Jun 2015 16:00:59 +0200
The kfree() function was called in two cases by the mac_ioctl() function
during error handling even if the passed variable did not contain a pointer
for a valid data item.
* This implementation detail could be improved by the introduction
of another jump label.
* Drop an unnecessary initialisation for the variable "buff" then.
Signed-off-by: Markus Elfring <[email protected]>
---
drivers/staging/wilc1000/linux_wlan.c | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/drivers/staging/wilc1000/linux_wlan.c b/drivers/staging/wilc1000/linux_wlan.c
index 2aa8d9b..f492310 100644
--- a/drivers/staging/wilc1000/linux_wlan.c
+++ b/drivers/staging/wilc1000/linux_wlan.c
@@ -2351,16 +2351,13 @@ int mac_close(struct net_device *ndev)
int mac_ioctl(struct net_device *ndev, struct ifreq *req, int cmd)
{
-
- u8 *buff = NULL;
+ u8 *buff;
s8 rssi;
u32 size = 0, length = 0;
perInterface_wlan_t *nic;
struct WILC_WFI_priv *priv;
s32 s32Error = WILC_SUCCESS;
-
-
/* struct iwreq *wrq = (struct iwreq *) req; // tony moved to case SIOCSIWPRIV */
#ifdef USE_WIRELESS
nic = netdev_priv(ndev);
@@ -2405,7 +2402,7 @@ int mac_ioctl(struct net_device *ndev, struct ifreq *req, int cmd)
if (copy_to_user(wrq->u.data.pointer, buff, size)) {
PRINT_ER("%s: failed to copy data to user buffer\n", __func__);
s32Error = -EFAULT;
- goto done;
+ goto free_buffer;
}
}
}
@@ -2420,8 +2417,9 @@ int mac_ioctl(struct net_device *ndev, struct ifreq *req, int cmd)
}
}
-done:
+free_buffer:
kfree(buff);
+done:
return s32Error;
}
--
2.4.4
Hi Markus,
On Wed, Jul 8, 2015 at 7:28 PM, SF Markus Elfring
<[email protected]> wrote:
>> If it's harmless, then no, but in this case, people are questioning
>> why you're adding it as it adds no value
>
> Some Git software developers care to keep the information complete
> for the author commit.
>
>
>> to anyone and makes it look like you don't know what you're doing.
>
> I specify message field overrides in my update suggestions intentionally.
>
>
>> The issue is that the headers you're adding, From: and Date: are unnecessary.
>
> We have got different opinions about the purpose.
Let me rephrase that: they _should_ be unnecessary.
>> The From: header you add is unnecessary as your email's From: header
>> has the exact same information.
>
> I would like to point out that there is a slight difference in my use case.
Then fix your email client or patch submission process.
>> The reason it's there is because sometimes people forward patches on
>> from other people, e.g. if I were to resend one of your patches,
>> I'd add a From: header to the body of the email so it'd be credited to you.
>
> I am also interested in such an use case.
This happens automatically if you use git-format-patch on a commit you
didn't author. Nobody is adding this manually.
>> The Date: header you add is unnecessary as git-format-patch sets the
>> date header in the email it produces to the author date stored in the commit.
>
> How do you think about my extra patch preparation for the mentioned
> mail forwarding?
It sounds like you're using what could only be described as a
Rube-Goldberg process to send email. Is there truly no way to simplify
that process? (Also, are the servers you send it through re-writing
the original headers?)
You should be sending the patches directly with SMTP using
git-send-email, if you're not, then you're making things overly
complicated for yourself.
>> So if you're sending your patches in emails produced by git-format-patch,
>> there's absolutely no reason to include it.
>
> I disagree here to some degree.
>
> The difference in suggested commit timestamps of a few minutes might look
> negligible for some patches. There are few occasions where the delay between
> a concrete commit and its publishing by an interface like email
> can become days.
I made a commit a month ago, it got rebased today, in fact, I sent
it's metadata in my previous email. When I ran git-format-patch on it,
the timestamp in the headers of the email produced was the date from a
month ago.
If I then sent that email, I believe it'd make it to the list here
with that date intact. If it hadn't, I'd be trying to figure out why
and either fix it or find a different path to get my patch here.
>> They are both almost completely irrelevant for most workflows as people
>> are less interested in when a commit was made and more interested in what
>> release it's in, how it was merged, etc. All of which should be
>> determined without using the timestamp.
>
> How often will it matter who made and published a change first?
If multiple people are submitting identical changes, then the one that
is applied is the one the maintainer sees first, which will most
likely be determined by which one hit their inbox / list first. Nobody
is going to look at timestamps in emails to determine which one will
be applied.
If you're worried about which one of several versions of a patch will
be applied, change the subject to [PATCH v2] ..... instead of [PATCH]
.... for the second version.
>> To be honest, I've only ever used that timestamp for reporting
>> purposes at work, and I'd be surprised if anyone was doing anything
>> other than that with them.
>
> Thanks for your detailed feedback.
>
>
>> How would you feel if someone came in to your place of work
>> and told you to change how you do the job you've been doing for years
>> without a good reason?
>
> You might feel uncomfortable for a moment if you would interpret
> such a suggestion as a personal attack.
I'm not interpreting this as a personal attack.
> I guess that I point only a few technical details out which can change
> the popularity of existing functionality from the Git software.
Git was originally written to manage the Linux kernel. This feature
was probably added by either Linus himself or one of the original
developers who I believe were kernel developers. The patch submission
process has been around for a long time, if this feature isn't used,
it's for a good reason.
Having a feature doesn't mean that it should be used.
Thanks,
--
Julian Calaby
Email: [email protected]
Profile: http://www.google.com/profiles/julian.calaby/
> I can't remember ever changing or explicitly preserving the commit date.
> I don't think I care enough.
Would any more software developers and maintainers like to share
their experiences around such details?
When do commit timestamps become relevant as a documentation item
for contribution authorship?
> Remembering the author separately from the committer is something
> git does by design anyway.
Do you usually just reuse a procedure from a well-known command
for which a description is provided like the following?
http://git-scm.com/docs/git-am
'…
"From: " and "Subject: " lines starting the body override
the respective commit author name and title values
taken from the headers.
…'
Will further fields be eventually mentioned there?
Regards,
Markus
> There's a file in the documentation directory of the kernel
> tree describing submitting patches and email client setup.
> Read them both,
I read this information several times.
> do what they say without anything extra.
Do you see any special consequences if a bit of "extra" functionality
is already provided by the Git software for a while?
> Your attempts to "improve" on the system are unnecessary
It seems that my approach does not need improvements for the current
command "git am".
Would a few extensions for the available documentation help to clarify
the situation?
Do items like "commit mail address" and "commit timestamp"
belong together for the data structure "author" by design
in this content management system?
> and annoying people.
I understand that various update suggestions can be surprising.
It is also usual that corresponding acceptance might take
a bit longer than what some contributors would prefer.
Regards,
Markus
>> The kfree() function was called in two cases by the mac_ioctl() function
>> during error handling even if the passed variable did not contain a pointer
>> for a valid data item.
>>
>> * This implementation detail could be improved by the introduction
>> of another jump label.
>>
>> * Drop an unnecessary initialisation for the variable "buff" then.
>
> That's a different issue, please fix it in a separate patch.
I have got an other opinion here.
This update suggestion depends on a small source code clean-up
from the previous patch, doesn't it?
Do you want that I split the second suggestion from this series
into more steps?
Regards,
Markus
On Wed, Jul 08, 2015 at 09:05:53PM +1000, Julian Calaby wrote:
> If multiple people are submitting identical changes, then the one that
> is applied is the one the maintainer sees first, which will most
> likely be determined by which one hit their inbox / list first. Nobody
> is going to look at timestamps in emails to determine which one will
> be applied.
And some maintainers may choose *not* to act on a patch first, even if
they see it first. They might be focused on bug fix patches, and not
act on cleanup or feature patches until -rc3 or rc4. Or maybe they
will use separate branches for "urgent_for_linus" patches, so two
different patchs may end up in completely different git flows.
> If you're worried about which one of several versions of a patch will
> be applied, change the subject to [PATCH v2] ..... instead of [PATCH]
> .... for the second version.
*Please* do this. In fact, this is the one thing I wish git
send-email would do automatically, along with having a place to edit
and track the 0/N summary patch.
> >> To be honest, I've only ever used that timestamp for reporting
> >> purposes at work, and I'd be surprised if anyone was doing anything
> >> other than that with them.
> >
> > Thanks for your detailed feedback.
Note also that some maintainers have work flow that deliberately smash
the date (i.e., because they are using a system such as guilt), so if
you are depending on the submitted timestamp, it's going to break on
you.
- Ted
> Note also that some maintainers have work flow that deliberately smash
> the date (i.e., because they are using a system such as guilt),
> so if you are depending on the submitted timestamp, it's going to
> break on you.
Thanks for your hint.
I am just trying to offer the possibility for the reuse of a more
precise commit timestamp together with an appropriate author mail
address for my update suggestions.
Do you reject any more such message field overrides?
Regards,
Markus
Hi Markus,
On Wed, Jul 8, 2015 at 2:15 AM, SF Markus Elfring
<[email protected]> wrote:
>> I can't remember ever changing or explicitly preserving the commit date.
>> I don't think I care enough.
>
> Would any more software developers and maintainers like to share
> their experiences around such details?
>
> When do commit timestamps become relevant as a documentation item
> for contribution authorship?
They are never relevant.
"When" a commit happened is never relevant except in relation to other
things, at which point the actual date and time is almost completely
irrelevant.
Just submit your patches using git-format-patch or git-send-email and
friends. There's a file in the documentation directory of the kernel
tree describing submitting patches and email client setup. Read them
both, do what they say without anything extra.
>> Remembering the author separately from the committer is something
>> git does by design anyway.
>
> Do you usually just reuse a procedure from a well-known command
> for which a description is provided like the following?
> http://git-scm.com/docs/git-am
> '…
> "From: " and "Subject: " lines starting the body override
> the respective commit author name and title values
> taken from the headers.
> …'
>
> Will further fields be eventually mentioned there?
Why? Just do what is described in SubmittingPatches. Your attempts to
"improve" on the system are unnecessary and annoying people. The
instructions there are the recommended way to do things for a reason.
Thanks,
--
Julian Calaby
Email: [email protected]
Profile: http://www.google.com/profiles/julian.calaby/
> The date, as far as I know, is ignored. It is the commit date,
> not the authoring date, and once your patch is applied by a maintainer
> (i.e. committed), the date gets reset anyway.
Thanks for your feedback.
> No need to try and preserve it.
I find that it might occasionally help to share and keep the record
on timestamps about the evolution for an original update suggestion.
Regards,
Markus
Hi Markus,
On Wed, Jul 8, 2015 at 5:09 PM, SF Markus Elfring
<[email protected]> wrote:
>> There's a file in the documentation directory of the kernel
>> tree describing submitting patches and email client setup.
>> Read them both,
>
> I read this information several times.
>
>
>> do what they say without anything extra.
>
> Do you see any special consequences if a bit of "extra" functionality
> is already provided by the Git software for a while?
If it's harmless, then no, but in this case, people are questioning
why you're adding it as it adds no value to anyone and makes it look
like you don't know what you're doing.
>> Your attempts to "improve" on the system are unnecessary
>
> It seems that my approach does not need improvements for the current
> command "git am".
> Would a few extensions for the available documentation help to clarify
> the situation?
The issue is that the headers you're adding, From: and Date: are unnecessary.
The From: header you add is unnecessary as your email's From: header
has the exact same information. The reason it's there is because
sometimes people forward patches on from other people, e.g. if I were
to resend one of your patches, I'd add a From: header to the body of
the email so it'd be credited to you.
The Date: header you add is unnecessary as git-format-patch sets the
date header in the email it produces to the author date stored in the
commit. (see below) So if you're sending your patches in emails
produced by git-format-patch, there's absolutely no reason to include
it.
> Do items like "commit mail address" and "commit timestamp"
> belong together for the data structure "author" by design
> in this content management system?
The information stored for a commit is:
= = = = = =
tree 09496defc9eb793c665a7b80aa22f24c7bd5f204
parent 63c07589832bfe5ec49f2523ddb0e94a20af0f31
author Julian Calaby <[email protected]> 1435196810 +1000
committer Julian Calaby <[email protected]> 1436322540 +1000
= = = = = =
Then the subject and commit message.
The numbers after the email addresses are the timestamps. They are
both almost completely irrelevant for most workflows as people are
less interested in when a commit was made and more interested in what
release it's in, how it was merged, etc. All of which should be
determined without using the timestamp.
To be honest, I've only ever used that timestamp for reporting
purposes at work, and I'd be surprised if anyone was doing anything
other than that with them.
In short, nobody cares, and nobody's going to be upset if the actual
time you authored a patch is different to the time recorded upstream.
>> and annoying people.
>
> I understand that various update suggestions can be surprising.
> It is also usual that corresponding acceptance might take
> a bit longer than what some contributors would prefer.
How would you feel if someone came in to your place of work and told
you to change how you do the job you've been doing for years without a
good reason?
Thanks,
--
Julian Calaby
Email: [email protected]
Profile: http://www.google.com/profiles/julian.calaby/
On Tue, Jul 7, 2015 at 1:53 PM, SF Markus Elfring
<[email protected]> wrote:
>> I think that as far as these kernel mailing lists are concerned,
>> the date of the update suggestion is the date on which you submitted the patch,
>> rather than the date you originally committed it to your local tree.
>
> I imagine that there are committers who would like to keep
> corresponding software development history a bit more accurate.
I guess it depends on what your view on accurate is.
>> If you wish to keep track of this evolution for yourself, or
>> wish to share it, you're better off stashing it somewhere in a
>> (public) git repo that you control.
>
> Would it be nicer to preserve such data directly also
> by the usual mail interface?
>
>
>> If you insist on placing the date somewhere, you can also put the date
>> there if you wish. It'll be ignored by git when applied.
>
> This content management tool provides the capability to store
> the discussed information by the parameters "--author=" and "--date=",
> doesn't it?
> Is the environment variable "GIT_AUTHOR_DATE" also interesting occasionally?
>
> How often do you take extra care for passing appropriate data there?
I can't remember ever changing or explicitly preserving the commit
date. I don't think I care enough. I did change the author on botched
patches, but that's an exception. Remembering the author separately
from the committer is something git does by design anyway.
Frans
On Wed, Jul 08, 2015 at 05:27:38PM +0200, SF Markus Elfring wrote:
> > Note also that some maintainers have work flow that deliberately smash
> > the date (i.e., because they are using a system such as guilt),
> > so if you are depending on the submitted timestamp, it's going to
> > break on you.
>
> Thanks for your hint.
>
> I am just trying to offer the possibility for the reuse of a more
> precise commit timestamp together with an appropriate author mail
> address for my update suggestions.
> Do you reject any more such message field overrides?
Well, I won't hold it against you, since I also often need to fix
patches or git commit descriptions anyway. But at the same time, my
workflow (which you have no right to dictate) **will** destroy your
timestamp. Given that you haven't explained why you want to do this,
I'm not going have much sympathy if you complain.
Personally, given that you're going through some extremely baroque
e-mail/patchsubmission scheme, I don't understand why you can't also
arrange to override the Date field in the e-mail header. But I don't
really care all that much, because I can ignore your timestamp no
matter where you put it.
Regards,
- Ted
On Tue, Jul 7, 2015 at 9:54 AM, SF Markus Elfring
<[email protected]> wrote:
>> No need to try and preserve it.
>
> I find that it might occasionally help to share and keep the record
> on timestamps about the evolution for an original update suggestion.
I think that as far as these kernel mailing lists are concerned, the
date of the update suggestion is the date on which you submitted the
patch, rather than the date you originally committed it to your local
tree. If you wish to keep track of this evolution for yourself, or
wish to share it, you're better off stashing it somewhere in a
(public) git repo that you control. If you wish, you can always make
mention of that repo below the ---, just above the diffstat. If you
insist on placing the date somewhere, you can also put the date there
if you wish. It'll be ignored by git when applied.
Cheers,
Frans
On Sat, Jun 27, 2015 at 04:36:14PM +0200, SF Markus Elfring wrote:
> From: Markus Elfring <[email protected]>
> Date: Sat, 27 Jun 2015 15:56:57 +0200
Why is this in the body of the email? Please fix your email client or
just use git send-email properly.
> Is there truly no way to simplify that process?
I see some software development possibilities which could improve
the communication with high volume mailing lists.
> You should be sending the patches directly with SMTP using git-send-email,
This tool is also fine for the publishing of a lot of patches.
> if you're not, then you're making things overly complicated for yourself.
But I prefer a graphical user interface for my mail handling so far.
> Having a feature doesn't mean that it should be used.
Does any of the "questionable functionality" get occasionally overlooked
a bit too often?
Regards,
Markus
On Tue, Jul 7, 2015 at 8:21 AM, SF Markus Elfring
<[email protected]> wrote:
>>> From: Markus Elfring <[email protected]>
>>> Date: Sat, 27 Jun 2015 15:56:57 +0200
>>
>> Why is this in the body of the email?
>
> Does the canonical patch format support to preserve
> specific details about a shown commit by specification
> of fields like "Date" and "From" in the message body?
Using "From:" in the body can be used to provide your properly spelled
name, or the name/e-mail of the actual author, if that happens not to
be the sender. git-send-email will do that automagically for you. If
"From:" is not specified in the message body, the e-mails sender will
be used as author.
The date, as far as I know, is ignored. It is the commit date, not the
authoring date, and once your patch is applied by a maintainer (i.e.
committed), the date gets reset anyway. No need to try and preserve
it.
Frans
> If it's harmless, then no, but in this case, people are questioning
> why you're adding it as it adds no value
Some Git software developers care to keep the information complete
for the author commit.
> to anyone and makes it look like you don't know what you're doing.
I specify message field overrides in my update suggestions intentionally.
> The issue is that the headers you're adding, From: and Date: are unnecessary.
We have got different opinions about the purpose.
> The From: header you add is unnecessary as your email's From: header
> has the exact same information.
I would like to point out that there is a slight difference in my use case.
> The reason it's there is because sometimes people forward patches on
> from other people, e.g. if I were to resend one of your patches,
> I'd add a From: header to the body of the email so it'd be credited to you.
I am also interested in such an use case.
> The Date: header you add is unnecessary as git-format-patch sets the
> date header in the email it produces to the author date stored in the commit.
How do you think about my extra patch preparation for the mentioned
mail forwarding?
> So if you're sending your patches in emails produced by git-format-patch,
> there's absolutely no reason to include it.
I disagree here to some degree.
The difference in suggested commit timestamps of a few minutes might look
negligible for some patches. There are few occasions where the delay between
a concrete commit and its publishing by an interface like email
can become days.
> They are both almost completely irrelevant for most workflows as people
> are less interested in when a commit was made and more interested in what
> release it's in, how it was merged, etc. All of which should be
> determined without using the timestamp.
How often will it matter who made and published a change first?
> To be honest, I've only ever used that timestamp for reporting
> purposes at work, and I'd be surprised if anyone was doing anything
> other than that with them.
Thanks for your detailed feedback.
> How would you feel if someone came in to your place of work
> and told you to change how you do the job you've been doing for years
> without a good reason?
You might feel uncomfortable for a moment if you would interpret
such a suggestion as a personal attack.
I guess that I point only a few technical details out which can change
the popularity of existing functionality from the Git software.
Regards,
Markus
> I think that as far as these kernel mailing lists are concerned,
> the date of the update suggestion is the date on which you submitted the patch,
> rather than the date you originally committed it to your local tree.
I imagine that there are committers who would like to keep
corresponding software development history a bit more accurate.
> If you wish to keep track of this evolution for yourself, or
> wish to share it, you're better off stashing it somewhere in a
> (public) git repo that you control.
Would it be nicer to preserve such data directly also
by the usual mail interface?
> If you insist on placing the date somewhere, you can also put the date
> there if you wish. It'll be ignored by git when applied.
This content management tool provides the capability to store
the discussed information by the parameters "--author=" and "--date=",
doesn't it?
Is the environment variable "GIT_AUTHOR_DATE" also interesting occasionally?
How often do you take extra care for passing appropriate data there?
Regards,
Markus
On Sat, Jun 27, 2015 at 04:37:24PM +0200, SF Markus Elfring wrote:
> From: Markus Elfring <[email protected]>
> Date: Sat, 27 Jun 2015 16:00:59 +0200
Again, please fix.
>
> The kfree() function was called in two cases by the mac_ioctl() function
> during error handling even if the passed variable did not contain a pointer
> for a valid data item.
>
> * This implementation detail could be improved by the introduction
> of another jump label.
>
> * Drop an unnecessary initialisation for the variable "buff" then.
That's a different issue, please fix it in a separate patch.
thanks,
greg k-h
>> From: Markus Elfring <[email protected]>
>> Date: Sat, 27 Jun 2015 15:56:57 +0200
>
> Why is this in the body of the email?
Does the canonical patch format support to preserve
specific details about a shown commit by specification
of fields like "Date" and "From" in the message body?
Regards,
Markus
Hi Markus,
On Wed, Jul 8, 2015 at 11:46 PM, SF Markus Elfring
<[email protected]> wrote:
>> Is there truly no way to simplify that process?
>
> I see some software development possibilities which could improve
> the communication with high volume mailing lists.
You shouldn't need any software development, most people's processes are:
1. Make changes
2. git add <files>
3. git commit
4. Repeat 1-3 for all changes they want to make
5. git-format-patch
6. git-send-email through some SMTP server to the list and appropriate
other people
>From what I've read, you appear to have some semi-automatic tool for
steps 1 through 4. I'd strongly recommend changing your patch
submission process to use git-format-patch and git-send-email. If
Sourceforge's email system doesn't let you send emails with SMTP
directly, then you might want to try sending emails through your ISP's
mail server.
Maintainers use a very similar process, however they collect and
review patches in some personal repository in addition to making
changes and committing them. Tools like patchwork are simply fancy
methods of accumulating patches into git trees. Most people are using
git-format-patch and git-send-email, possibly with some scripting
around them, to format and send patches.
>> You should be sending the patches directly with SMTP using git-send-email,
>
> This tool is also fine for the publishing of a lot of patches.
git-send-email or some other tool?
>> if you're not, then you're making things overly complicated for yourself.
>
> But I prefer a graphical user interface for my mail handling so far.
I send my patches using git-send-email through gmail's servers then
deal with literally everything else through gmail's web interface or
Android app. Using git-send-email doesn't mean you can't use a
graphical mail interface.
I used to send patches through a carefully configured instance of
Thunderbird, however this required too many steps and it loved to
mangle patches in ways that annoyed people.
>> Having a feature doesn't mean that it should be used.
>
> Does any of the "questionable functionality" get occasionally overlooked
> a bit too often?
It's not "questionable functionality", it's functionality we don't see
a need for.
If I wanted to, I could insert a patch at any point in the history of
Linux in my own repository, however any attempt to push that upstream
would cause enormous amounts of pain and annoyance, to everyone who
attempted to deal with it, so I don't.
Thanks,
--
Julian Calaby
Email: [email protected]
Profile: http://www.google.com/profiles/julian.calaby/
>> - if (strncasecmp(buff, "RSSI", length) == 0) {
>> + if (strncasecmp(buff, "RSSI", 0) == 0) {
>> + s8 rssi;
>> +
>
> Um, please think a second about if it makes any sense at all to compare
> zero chars of two strings.
Under which circumstances should the variable "length" contain an other
value than zero?
How can this open issue be fixed better?
Regards,
Markus
From: Markus Elfring <[email protected]>
Date: Sun, 24 Jul 2016 21:45:37 +0200
Three local variables were used only within a single case branch.
* Thus move the data type definition for "rssi" and "size" into the
corresponding code block.
* The variable "length" was not modified after its initialisation.
Thus pass a constant value in the affected function call instead.
Signed-off-by: Markus Elfring <[email protected]>
---
drivers/staging/wilc1000/linux_wlan.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/staging/wilc1000/linux_wlan.c b/drivers/staging/wilc1000/linux_wlan.c
index 7b1ebcc..173be16 100644
--- a/drivers/staging/wilc1000/linux_wlan.c
+++ b/drivers/staging/wilc1000/linux_wlan.c
@@ -1094,8 +1094,6 @@ int wilc_mac_close(struct net_device *ndev)
static int mac_ioctl(struct net_device *ndev, struct ifreq *req, int cmd)
{
u8 *buff = NULL;
- s8 rssi;
- u32 size = 0, length = 0;
struct wilc_vif *vif;
s32 ret = 0;
struct wilc *wilc;
@@ -1110,8 +1108,7 @@ static int mac_ioctl(struct net_device *ndev, struct ifreq *req, int cmd)
case SIOCSIWPRIV:
{
struct iwreq *wrq = (struct iwreq *)req;
-
- size = wrq->u.data.length;
+ u32 size = wrq->u.data.length;
if (size && wrq->u.data.pointer) {
buff = memdup_user(wrq->u.data.pointer,
@@ -1119,7 +1116,9 @@ static int mac_ioctl(struct net_device *ndev, struct ifreq *req, int cmd)
if (IS_ERR(buff))
return PTR_ERR(buff);
- if (strncasecmp(buff, "RSSI", length) == 0) {
+ if (strncasecmp(buff, "RSSI", 0) == 0) {
+ s8 rssi;
+
ret = wilc_get_rssi(vif, &rssi);
netdev_info(ndev, "RSSI :%d\n", rssi);
--
2.9.2
From: Markus Elfring <[email protected]>
Further update suggestions were taken into account
after a patch was applied from static source code analysis.
Markus Elfring (3):
Delete an unnecessary check before the function call "release_firmware"
One function call less in mac_ioctl() after error detection
Reduce scope for a few variables in mac_ioctl()
drivers/staging/wilc1000/linux_wlan.c | 19 ++++++++-----------
1 file changed, 8 insertions(+), 11 deletions(-)
--
2.9.2
>
> - if (strncasecmp(buff, "RSSI", length) == 0) {
> + if (strncasecmp(buff, "RSSI", 0) == 0) {
> + s8 rssi;
> +
Um, please think a second about if it makes any sense at all to compare
zero chars of two strings.
Lino
> Which circumstances do "not any sense at all" imply?
Should the expression 'strlen("RSSI")' be passed for the parameter 'length' instead?
> I suggest to fix this since it is indeed a bug,
We can agree that this function implementation was broken for a while there.
> instead of doing "micro optimizations" - which is the last thing that code in the staging area
> needs (as IIRC you have already been told by others, including the staging maintainer).
The acceptance might grow a bit more for such software fine-tuning
(like refactoring around variable usage).
Regards,
Markus
From: Markus Elfring <[email protected]>
Date: Sun, 24 Jul 2016 21:15:23 +0200
The kfree() function was called in two cases by the mac_ioctl() function
during error handling even if the passed variable did not contain a pointer
for a valid data item.
Improve this implementation detail by the introduction of another
jump label.
Signed-off-by: Markus Elfring <[email protected]>
---
drivers/staging/wilc1000/linux_wlan.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/drivers/staging/wilc1000/linux_wlan.c b/drivers/staging/wilc1000/linux_wlan.c
index cdef645..7b1ebcc 100644
--- a/drivers/staging/wilc1000/linux_wlan.c
+++ b/drivers/staging/wilc1000/linux_wlan.c
@@ -1130,7 +1130,7 @@ static int mac_ioctl(struct net_device *ndev, struct ifreq *req, int cmd)
if (copy_to_user(wrq->u.data.pointer, buff, size)) {
netdev_err(ndev, "failed to copy\n");
ret = -EFAULT;
- goto done;
+ goto free_buffer;
}
}
}
@@ -1144,11 +1144,9 @@ static int mac_ioctl(struct net_device *ndev, struct ifreq *req, int cmd)
goto done;
}
}
-
-done:
-
+free_buffer:
kfree(buff);
-
+done:
return ret;
}
--
2.9.2
> Gesendet: Dienstag, 26. Juli 2016 um 08:25 Uhr
> Von: "SF Markus Elfring" <[email protected]>
>
> >> - if (strncasecmp(buff, "RSSI", length) == 0) {
> >> + if (strncasecmp(buff, "RSSI", 0) == 0) {
> >> + s8 rssi;
> >> +
> >
> > Um, please think a second about if it makes any sense at all to compare
> > zero chars of two strings.
>
> Under which circumstances should the variable "length" contain an other
> value than zero?
Which circumstances do "not any sense at all" imply?
>
> How can this open issue be fixed better?
The code is not too complicated and I think it is very obvious which value/variable
should be passed instead of 0. I suggest to fix this since it is indeed a bug, instead of
doing "micro optimizations" - which is the last thing that code in the staging area
needs (as IIRC you have already been told by others, including the staging maintainer).
Regards,
Lino
Am 25.07.2016 21:17, schrieb Lino Sanfilippo:
>>
>> - if (strncasecmp(buff, "RSSI", length) == 0) {
>> + if (strncasecmp(buff, "RSSI", 0) == 0) {
>> + s8 rssi;
>> +
>
> Um, please think a second about if it makes any sense at all to compare
> zero chars of two strings.
>
> Lino
also:
the switch has only one case and default.
re,
wh
Hi Marcus,
On Mon, Jul 25, 2016 at 6:22 AM, SF Markus Elfring
<[email protected]> wrote:
> From: Markus Elfring <[email protected]>
> Date: Sun, 24 Jul 2016 21:15:23 +0200
>
> The kfree() function was called in two cases by the mac_ioctl() function
> during error handling even if the passed variable did not contain a pointer
> for a valid data item.
>
> Improve this implementation detail by the introduction of another
> jump label.
>
> Signed-off-by: Markus Elfring <[email protected]>
This is pointless micro-optimisation: kfree(NULL) is perfectly valid
and buff is either malloc'd memory or NULL when it's called.
Thanks,
--
Julian Calaby
Email: [email protected]
Profile: http://www.google.com/profiles/julian.calaby/
From: Markus Elfring <[email protected]>
Date: Sun, 24 Jul 2016 21:00:20 +0200
The release_firmware() function tests whether its argument is NULL and then
returns immediately. Thus the test around the call is not needed.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring <[email protected]>
---
drivers/staging/wilc1000/linux_wlan.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/wilc1000/linux_wlan.c b/drivers/staging/wilc1000/linux_wlan.c
index 3a66255..cdef645 100644
--- a/drivers/staging/wilc1000/linux_wlan.c
+++ b/drivers/staging/wilc1000/linux_wlan.c
@@ -1223,7 +1223,7 @@ void wilc_netdev_cleanup(struct wilc *wilc)
vif[i] = netdev_priv(wilc->vif[i]->ndev);
}
- if (wilc && wilc->firmware) {
+ if (wilc) {
release_firmware(wilc->firmware);
wilc->firmware = NULL;
}
--
2.9.2