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