Linus Reviews Your Code — Week 1: AbstractLockFactory & the Deadlock Machine
Linus eviscerates two commits this week: an AbstractLockFactory wrapped around a single mutex call, and a spinlock that forgot to disable interrupts. Two out of twelve rejected. He's seen worse.
Linus Reviews Your Code — Week 1
This week's commit queue: 12 commits across 3 repos. Most of them are boring. Two of them are not.
Commit 1 — REJECTED
Repo:
kworker-sched · Commit: mutex: add AbstractLockFactory wrapper+class AbstractLockFactory {
+ virtual AbstractLock* createLock() = 0;
+};
+
+class PthreadMutexLock : public AbstractLock {
+ void acquire() override { pthread_mutex_lock(&m_); }
+ pthread_mutex_t m_;
+};"Someone wrapped a single mutex lock in three layers of abstraction. Three. AnAbstractLockFactory. You don't need a factory to callpthread_mutex_lock. This is enterprise brain damage. I want the author to write 'I will not abstract simple things' fifty times."
Verdict: Rejected. Rewrite as a plain call. Delete the factory. Delete the base class. Delete your inheritance hierarchy.
Commit 2 — REJECTED
Repo:
kworker-sched · Commit: sched: add spinlock around task queue+spin_lock(&task_queue_lock);
+list_add_tail(&task->entry, &task_queue);
+spin_unlock(&task_queue_lock);(Interrupts not disabled —
spin_lock_irqsave was not used.)"Somebody inkworker-scheddecided spin-locks don't need to disable interrupts. Congratulations, you just invented a deadlock machine. This is the code that makes me reply in all caps. Go readDocumentation/locking. It's right there."
Verdict: Rejected. Use
spin_lock_irqsave / spin_unlock_irqrestore. This is not optional.This Week's Scorecard
| Stat | Count |
|---|---|
| Total commits reviewed | 12 |
| Repos covered | 3 |
| Rejected | 2 |
| Crimes against locking discipline | 1 |
| Gratuitous factory patterns | 1 |
"Two out of twelve rejected. I've seen worse. See you Friday."
Add more perspectives or context around this content.