Re: [PATCH v7 06/25] RDMA/rtrs: client: main functionality

From: Jinpu Wang <jinpu.wang@cloud.ionos.com>
To: Leon Romanovsky <leon@kernel.org>
Cc: Jack Wang <jinpuwang@gmail.com>,
	linux-block@vger.kernel.org, linux-rdma@vger.kernel.org,
	Jens Axboe <axboe@kernel.dk>,
	Christoph Hellwig <hch@infradead.org>,
	Sagi Grimberg <sagi@grimberg.me>,
	Bart Van Assche <bvanassche@acm.org>,
	Doug Ledford <dledford@redhat.com>,
	Jason Gunthorpe <jgg@ziepe.ca>,
	Danil Kipnis <danil.kipnis@cloud.ionos.com>,
	Roman Penyaev <rpenyaev@suse.de>
Subject: Re: [PATCH v7 06/25] RDMA/rtrs: client: main functionality
Date: Mon, 20 Jan 2020 12:24:21 +0100	[thread overview]
Message-ID: <CAMGffE=22zvvOFJpzyJPOLQFwA+9oMoxZSj=FAZV6RBonwJaEA@mail.gmail.com> (raw)
In-Reply-To: <20200118101254.GD18467@unreal>

> > > > > > +static bool __rtrs_clt_change_state(struct rtrs_clt_sess *sess,
> > > > > > +                                  enum rtrs_clt_state new_state)
> > > > > > +{
> > > > > > +     enum rtrs_clt_state old_state;
> > > > > > +     bool changed = false;
> > > > > > +
> > > > > > +     lockdep_assert_held(&sess->state_wq.lock);
> > > > > > +
> > > > > > +     old_state = sess->state;
> > > > > > +     switch (new_state) {
> > > > > > +     case RTRS_CLT_CONNECTING:
> > > > > > +             switch (old_state) {
> > > > >
> > > > > Double switch is better to be avoided.
> > > > what's the better way to do it?
> > >
> > > Rewrite function to be more readable.
> > Frankly I think it's easy to read, depends on old_state change to new state.
> > see also scsi_device_set_state
>
> If you so in favor of switch inside switch, at lest do it properly.
>
> The scsi_device_set_state() function implements success-oriented
> approach and has very clear state machine without distraction and
> extra variables like changed/not_changed. You have completely
> opposite implementation to scsi_device_set_state().
Thanks for reviewing our code during the weekend, I respect the effort.

The reason we have "changed" variable is only when the state changed
as expected,
the caller will do further action like reconnect or close the session.

I will add kernel-doc around the function to make it more clear.

Thanks Leon