The functions ceph_put_snap_context() and iput() test whether their argument
is NULL and then return 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]>
---
fs/ceph/caps.c | 3 +--
fs/ceph/mds_client.c | 6 ++----
fs/ceph/snap.c | 9 +++------
3 files changed, 6 insertions(+), 12 deletions(-)
diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
index 6d1cd45..7d99fc8 100644
--- a/fs/ceph/caps.c
+++ b/fs/ceph/caps.c
@@ -3136,8 +3136,7 @@ flush_cap_releases:
done:
mutex_unlock(&session->s_mutex);
done_unlocked:
- if (inode)
- iput(inode);
+ iput(inode);
return;
bad:
diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
index bad07c0..3b0ab05 100644
--- a/fs/ceph/mds_client.c
+++ b/fs/ceph/mds_client.c
@@ -523,8 +523,7 @@ void ceph_mdsc_release_request(struct kref *kref)
}
if (req->r_locked_dir)
ceph_put_cap_refs(ceph_inode(req->r_locked_dir), CEPH_CAP_PIN);
- if (req->r_target_inode)
- iput(req->r_target_inode);
+ iput(req->r_target_inode);
if (req->r_dentry)
dput(req->r_dentry);
if (req->r_old_dentry)
@@ -995,8 +994,7 @@ out:
session->s_cap_iterator = NULL;
spin_unlock(&session->s_cap_lock);
- if (last_inode)
- iput(last_inode);
+ iput(last_inode);
if (old_cap)
ceph_put_cap(session->s_mdsc, old_cap);
diff --git a/fs/ceph/snap.c b/fs/ceph/snap.c
index f01645a..c1cc993 100644
--- a/fs/ceph/snap.c
+++ b/fs/ceph/snap.c
@@ -365,8 +365,7 @@ static int build_snap_context(struct ceph_snap_realm *realm)
realm->ino, realm, snapc, snapc->seq,
(unsigned int) snapc->num_snaps);
- if (realm->cached_context)
- ceph_put_snap_context(realm->cached_context);
+ ceph_put_snap_context(realm->cached_context);
realm->cached_context = snapc;
return 0;
@@ -590,15 +589,13 @@ static void queue_realm_cap_snaps(struct ceph_snap_realm
*realm)
if (!inode)
continue;
spin_unlock(&realm->inodes_with_caps_lock);
- if (lastinode)
- iput(lastinode);
+ iput(lastinode);
lastinode = inode;
ceph_queue_cap_snap(ci);
spin_lock(&realm->inodes_with_caps_lock);
}
spin_unlock(&realm->inodes_with_caps_lock);
- if (lastinode)
- iput(lastinode);
+ iput(lastinode);
list_for_each_entry(child, &realm->children, child_item) {
dout("queue_realm_cap_snaps %p %llx queue child %p %llx\n",
--
2.1.3
On Sun, Nov 2, 2014 at 5:20 PM, SF Markus Elfring
<[email protected]> wrote:
> The functions ceph_put_snap_context() and iput() test whether their argument
> is NULL and then return 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]>
> ---
> fs/ceph/caps.c | 3 +--
> fs/ceph/mds_client.c | 6 ++----
> fs/ceph/snap.c | 9 +++------
> 3 files changed, 6 insertions(+), 12 deletions(-)
[CC'ed Zheng]
Applied, but see below.
>
> diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
> index 6d1cd45..7d99fc8 100644
> --- a/fs/ceph/caps.c
> +++ b/fs/ceph/caps.c
> @@ -3136,8 +3136,7 @@ flush_cap_releases:
> done:
> mutex_unlock(&session->s_mutex);
> done_unlocked:
> - if (inode)
> - iput(inode);
> + iput(inode);
> return;
>
> bad:
> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> index bad07c0..3b0ab05 100644
> --- a/fs/ceph/mds_client.c
> +++ b/fs/ceph/mds_client.c
> @@ -523,8 +523,7 @@ void ceph_mdsc_release_request(struct kref *kref)
> }
> if (req->r_locked_dir)
> ceph_put_cap_refs(ceph_inode(req->r_locked_dir), CEPH_CAP_PIN);
> - if (req->r_target_inode)
> - iput(req->r_target_inode);
> + iput(req->r_target_inode);
> if (req->r_dentry)
> dput(req->r_dentry);
dput() also checks for NULL argument, but the check is wrapped into
unlikely(), which is why I presume it wasn't picked up. It would be
great if you could improve your coccinelle script to handle
{un,}likely() as well.
> if (req->r_old_dentry)
> @@ -995,8 +994,7 @@ out:
> session->s_cap_iterator = NULL;
> spin_unlock(&session->s_cap_lock);
>
> - if (last_inode)
> - iput(last_inode);
> + iput(last_inode);
> if (old_cap)
> ceph_put_cap(session->s_mdsc, old_cap);
>
> diff --git a/fs/ceph/snap.c b/fs/ceph/snap.c
> index f01645a..c1cc993 100644
> --- a/fs/ceph/snap.c
> +++ b/fs/ceph/snap.c
> @@ -365,8 +365,7 @@ static int build_snap_context(struct ceph_snap_realm *realm)
> realm->ino, realm, snapc, snapc->seq,
> (unsigned int) snapc->num_snaps);
>
> - if (realm->cached_context)
> - ceph_put_snap_context(realm->cached_context);
> + ceph_put_snap_context(realm->cached_context);
> realm->cached_context = snapc;
> return 0;
>
> @@ -590,15 +589,13 @@ static void queue_realm_cap_snaps(struct ceph_snap_realm
> *realm)
The patch was corrupted, that should have been a single line. I fixed
it up but you may want to look into your email client settings.
Thanks,
Ilya
> dput() also checks for NULL argument, but the check is wrapped into
> unlikely(), which is why I presume it wasn't picked up. It would be
> great if you could improve your coccinelle script to handle
> {un,}likely() as well.
Thanks for your suggestion.
Should I consider any more fine-tuning for the affected script
"list_input_parameter_validation1.cocci" in the near future?
https://lkml.org/lkml/2014/3/5/362
http://article.gmane.org/gmane.comp.version-control.coccinelle/3514
>> @@ -590,15 +589,13 @@ static void queue_realm_cap_snaps(struct ceph_snap_realm
>> *realm)
>
> The patch was corrupted, that should have been a single line. I fixed
> it up but you may want to look into your email client settings.
Thanks for your feedback.
Does this example show a conflict between long comments after patch ranges and
line length limitation for email eventually?
Regards,
Markus
On Mon, Nov 3, 2014 at 4:27 PM, SF Markus Elfring
<[email protected]> wrote:
>> dput() also checks for NULL argument, but the check is wrapped into
>> unlikely(), which is why I presume it wasn't picked up. It would be
>> great if you could improve your coccinelle script to handle
>> {un,}likely() as well.
>
> Thanks for your suggestion.
>
> Should I consider any more fine-tuning for the affected script
> "list_input_parameter_validation1.cocci" in the near future?
> https://lkml.org/lkml/2014/3/5/362
> http://article.gmane.org/gmane.comp.version-control.coccinelle/3514
Make sure it at least catches stuff like:
{
if (input) {
}
}
{
if (likely(input)) {
}
}
{
if (!input)
return;
...
}
{
if (unlikely(!input))
return;
...
}
And of course each match then has to be validated manually.
>
>
>>> @@ -590,15 +589,13 @@ static void queue_realm_cap_snaps(struct ceph_snap_realm
>>> *realm)
>>
>> The patch was corrupted, that should have been a single line. I fixed
>> it up but you may want to look into your email client settings.
>
> Thanks for your feedback.
>
> Does this example show a conflict between long comments after patch ranges and
> line length limitation for email eventually?
There is no line length limitation for email, at least one that would
be relevant here. Patches should be sent verbatim, no line wrapping,
expandtab, etc or they won't apply. I'd recommend git-send-email, but
if you want to make thunderbird work for patches (which is what you
seem to be using) have a look at the "Thunderbird (GUI)" section of
Documentation/email-clients.txt in the kernel tree.
Thanks,
Ilya