tcp: cleanup of syncache_expand()

[tuexen]

Summary

• Consistently free the string after unlocking the sch, if possible.
• Remove the failure handling in case of sc != NULL, since this is not possible anymore.
• Remove the use of goto and instead return 0 in the three cases.

The only change in behavior is that in three out of the four cases, where 0 is returned, *lsop is not set to NULL anymore. So the behavior is now consistent and also documented in a comment. The current in tree callers only look at *lsop, if and only if syncache_expand() returns 1.

[peter.lei_ieee.org]

sys/netinet/tcp_syncache.c

1219

Doesn't this free() belong in the if ((s= block above?

And for consistency with the rest of the function, the SCH_UNLOCK(sch) could be done before the tcp_log_addrs and free.

[tuexen]

sys/netinet/tcp_syncache.c

1219

Doesn't this free() belong in the if ((s= block above?

Conceptually yes. But then we would hold the lock when calling free and jtl@ preferres to not do that.

And for consistency with the rest of the function, the SCH_UNLOCK(sch) could be done before the tcp_log_addrs and free.

But when generating the log message, we use sc->sc_tsreflect. So if we do SCH_UNLOCK(sch) before log(), we would potentially use memory already freed.
So for log messages without any reference to sc, I do the free() in the if() and do SCH_UNLOCK(sch) before the if(). But if the log messages have a reference to sc, I unlock first and then call free right after the if().
Please note that s is set in any case and calling free() with s == NULL is OK.

[peter.lei_ieee.org]

sys/netinet/tcp_syncache.c

1219

Doesn't this free() belong in the if ((s= block above?

Conceptually yes. But then we would hold the lock when calling free and jtl@ preferres to not do that.

Yep, agree we should free w/o lock (which the below would this ordering necessary)

And for consistency with the rest of the function, the SCH_UNLOCK(sch) could be done before the tcp_log_addrs and free.

But when generating the log message, we use sc->sc_tsreflect. So if we do SCH_UNLOCK(sch) before log(), we would potentially use memory already freed.

Yes, of course- somehow I missed the use of sc->sc_tsreflect!

So for log messages without any reference to sc, I do the free() in the if() and do SCH_UNLOCK(sch) before the if(). But if the log messages have a reference to sc, I unlock first and then call free right after the if().
Please note that s is set in any case and calling free() with s == NULL is OK.