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

[Jinpu Wang <jinpu.wang@cloud.ionos.com>]

> > > > > > +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