Hi Jonathan, On 10/16/17 7:04 PM, Jonathan Looney wrote: > I apologize for just getting to this, but your code just made it into my > local tree. > > The non-INVARIANTS case looks incorrect. Because tw stays on the list, > it can end up stuck at the front of the queue and block everything > behind it. > > Personally, I would be more comfortable just panic'ing here. This > indicates a problem. And, depending on the way things got corrupted, you > could end up with other hard-to-debug problems if you continue with a > corrupted state.
Good remark, actually, this logic: #ifdef INVARIANTS -> panic() #else INVARIANTS -> log(LOG_ERR) was introduced with the main fix: https://reviews.freebsd.org/rS307551 Why not panic()-ing in both cases INVARIANTS and !INVARIANTS already? The rational is that r307551 fixes an issue introduced with r185775, pushed 8 years before: https://reviews.freebsd.org/rS185775 Obviously, this double free is either rarely reproduced and/or has only minor effects for most of people (like a connection closes sooner than expected), only few people got annoying kernel panic/infinite loop. Thus before introducing a panic() that could potentially impact a fair amount of people, better introducing a log(LOG_ERR) first. Now, when replace these log(LOG_ERR) calls by a proper panic()? I would say when 11.0 (which is the last supported release without the r307551 fix) reaches the EoL state: End of October 2017 I believe. And so far, so good, no log(LOG_ERR)/panic()/similar issues have been witness with r307551 fix. Of course, if you think it is too cautious, you can create a review with the panic() calls change to get wider point of views. My 2 cents. -- Julien _______________________________________________ svn-src-head@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
