linux-kernel - Re: [PATCH] RDMA/cma: Fix checkpatch error

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 11 Dec 2019 20:33:10 -0500
From:   Max Hirsch <max.hirsch@...il.com>
To:     Jason Gunthorpe <jgg@...pe.ca>
Cc:     Doug Ledford <dledford@...hat.com>,
        Parav Pandit <parav@...lanox.com>,
        Leon Romanovsky <leon@...nel.org>,
        Steve Wise <swise@...ngridcomputing.com>,
        Bart Van Assche <bvanassche@....org>,
        Danit Goldberg <danitg@...lanox.com>,
        Matthew Wilcox <willy@...radead.org>,
        Dag Moxnes <dag.moxnes@...cle.com>,
        Myungho Jung <mhjungk@...il.com>, linux-rdma@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] RDMA/cma: Fix checkpatch error

Thanks for the quick response. This is my first patch, so I want to
follow the correct protocol. I reran checkpatch after making the
changes and there were no errors or warnings in the region I changed.

...
WARNING: line over 80 characters
#308: FILE: drivers/infiniband/core/cma.c:308:
+ return cma_dev->default_roce_tos[port - rdma_start_port(cma_dev->device)];

<<<<<<<<<<<<<<<<<< HERE IS WHERE THE ERROR WAS

WARNING: line over 80 characters
#495: FILE: drivers/infiniband/core/cma.c:495:
+ struct cma_multicast *mc = container_of(kref, struct cma_multicast, mcref);
...

I was have read that it is beneficial to make the changes very small.
I wanted to target a specific checkpatch error. If you believe it
would be better I can make a patch cleaning up the entire function.

I can also make a second patch changing ret to a bool. I did not want
to do that as a part of this patch because: I wanted a VERY small
change (increase acceptance likelihood), and there is a specific
protocol for submitting multiple commits in a patch, i.e. ordering
them correctly. I did not want to introduce a potential error source
when submitting a patch i.e. I submitted 2 commits in the wrong order.

Since you have the comment to remove the brackets around the ret
assign how would I go about modifying this patch? Should I resubmit a
patch i.e. start a new email with with your proposed changes?


On Wed, Dec 11, 2019 at 11:26 AM Jason Gunthorpe <jgg@...pe.ca> wrote:
>
> On Wed, Dec 11, 2019 at 11:16:26AM +0000, Max Hirsch wrote:
> > When running checkpatch on cma.c the following error was found:
>
> I think checkpatch will complain about your patch, did you run it?
>
> > ERROR: do not use assignment in if condition
> > #413: FILE: drivers/infiniband/tmp.c:413:
> > +     if ((ret = (id_priv->state == comp)))
> >
> > This patch moves the assignment of ret to the previous line. The if statement then checks the value of ret assigned on the previous line. The assigned value of ret is not changed. Testing involved recompiling and loading the kernel. After the changes checkpatch does not report this the error in cma.c.
> >
> > Signed-off-by: Max Hirsch <max.hirsch@...il.com>
> >  drivers/infiniband/core/cma.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
> > index 25f2b70fd8ef..bdb7a8493517 100644
> > +++ b/drivers/infiniband/core/cma.c
> > @@ -410,7 +410,8 @@ static int cma_comp_exch(struct rdma_id_private *id_priv,
> >       int ret;
> >
> >       spin_lock_irqsave(&id_priv->lock, flags);
> > -     if ((ret = (id_priv->state == comp)))
> > +     ret = (id_priv->state == comp);
>
> Brackets are not needed
>
> Ret and the return result should be changed to a bool
>
> Jason