On Tue, 2017-12-19 at 14:28 -0700, Jason Gunthorpe wrote:
> On Tue, Dec 19, 2017 at 09:21:05PM +0000, Bart Van Assche wrote:
> > On Tue, 2017-12-19 at 14:16 -0700, Jason Gunthorpe wrote:
> > > The very first thing free_res does is the same lock/signal/unlock
> > > sequence, so there is no reason to open code it before calling
> > > free_res.
> > >
> > > Signed-off-by: Jason Gunthorpe <jgg@xxxxxxxxxxxx>
> > > srp_daemon/srp_daemon.c | 4 ----
> > > 1 file changed, 4 deletions(-)
> > >
> > > Noticed while looking at honli's patch.. Didn't make a PR.
> > >
> > > diff --git a/srp_daemon/srp_daemon.c b/srp_daemon/srp_daemon.c
> > > index cec36db2e0f12e..a1798558373490 100644
> > > +++ b/srp_daemon/srp_daemon.c
> > > @@ -2074,10 +2074,6 @@ static int ibsrpdm(int argc, char *argv[])
> > > pr_err("Querying SRP targets failed\n");
> > >
> > > assert(res->sync_res);
> > > - pthread_mutex_lock(&res->sync_res->retry_mutex);
> > > - res->sync_res->stop_threads = 1;
> > > - pthread_cond_signal(&res->sync_res->retry_cond);
> > > - pthread_mutex_unlock(&res->sync_res->retry_mutex);
> >
> > Hello Jason,
> >
> > Since this patch removes all code that dereferences res->sync_res from
> > the ibsrpdm() function, have you considered to remove assert(res->sync_res)
> > too?
>
> Well, that would be a behavior change.. free_res works OK if sync_res
> is NULL, so I'm guessing that the assert had some meaning to show that
> the reconnect_thread was always started for ibsrpd?
>
> Or maybe we shouldn't care about that?
>
> Happy to drop it if you think it makes sense.
Hello Jason,
If the alloc_res() return value is not NULL then that implies that res->sync_res
!= NULL. The assert() I referred to is only reached if alloc_res() succeeded. So
I think it it is safe to leave out that assert() statement.
Bart.
��.n��������+%������w��{.n�����{���fk��ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f
