[PATCH] kvm tools: Fix pthread mutex error checks

[PATCH] kvm tools: Fix pthread mutex error checks

[Pekka Enberg]

The pthread_mutex_{lock|unlock} functions return non-zero, not negative number
upon error. Fix that wrong assumption in the code.

Cc: Asias He <asias.hejun@gmail.com>
Cc: Cyrill Gorcunov <gorcunov@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>
Signed-off-by: Pekka Enberg <penberg@kernel.org>
---
 tools/kvm/8250-serial.c |   12 ++++++------
 tools/kvm/virtio-blk.c  |    8 ++++----
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/tools/kvm/8250-serial.c b/tools/kvm/8250-serial.c
index 3395f85..9394e88 100644
--- a/tools/kvm/8250-serial.c
+++ b/tools/kvm/8250-serial.c
@@ -116,7 +116,7 @@ void serial8250__inject_interrupt(struct kvm *self)
 {
 	struct serial8250_device *dev = &devices[0];
 
-	if (pthread_mutex_lock(&dev->mutex) < 0)
+	if (pthread_mutex_lock(&dev->mutex) != 0)
 		die("pthread_mutex_lock");
 
 	serial8250__receive(self, dev);
@@ -133,7 +133,7 @@ void serial8250__inject_interrupt(struct kvm *self)
 		kvm__irq_line(self, dev->irq, 1);
 	}
 
-	if (pthread_mutex_unlock(&dev->mutex) < 0)
+	if (pthread_mutex_unlock(&dev->mutex) != 0)
 		die("pthread_mutex_unlock");
 }
 
@@ -165,7 +165,7 @@ static bool serial8250_out(struct kvm *self, uint16_t port, void *data, int size
 	if (!dev)
 		return false;
 
-	if (pthread_mutex_lock(&dev->mutex) < 0)
+	if (pthread_mutex_lock(&dev->mutex) != 0)
 		die("pthread_mutex_lock");
 
 	offset		= port - dev->iobase;
@@ -239,7 +239,7 @@ static bool serial8250_out(struct kvm *self, uint16_t port, void *data, int size
 	}
 
 out_unlock:
-	if (pthread_mutex_unlock(&dev->mutex) < 0)
+	if (pthread_mutex_unlock(&dev->mutex) != 0)
 		die("pthread_mutex_unlock");
 
 	return ret;
@@ -255,7 +255,7 @@ static bool serial8250_in(struct kvm *self, uint16_t port, void *data, int size,
 	if (!dev)
 		return false;
 
-	if (pthread_mutex_lock(&dev->mutex) < 0)
+	if (pthread_mutex_lock(&dev->mutex) != 0)
 		die("pthread_mutex_lock");
 
 	offset		= port - dev->iobase;
@@ -321,7 +321,7 @@ static bool serial8250_in(struct kvm *self, uint16_t port, void *data, int size,
 		goto out_unlock;
 	}
 out_unlock:
-	if (pthread_mutex_unlock(&dev->mutex) < 0)
+	if (pthread_mutex_unlock(&dev->mutex) != 0)
 		die("pthread_mutex_unlock");
 
 	return ret;
diff --git a/tools/kvm/virtio-blk.c b/tools/kvm/virtio-blk.c
index 8f1c684..5e7f57d 100644
--- a/tools/kvm/virtio-blk.c
+++ b/tools/kvm/virtio-blk.c
@@ -70,7 +70,7 @@ static bool virtio_blk_pci_io_in(struct kvm *self, uint16_t port, void *data, in
 	unsigned long offset;
 	bool ret = true;
 
-	if (pthread_mutex_lock(&blk_device.mutex) < 0)
+	if (pthread_mutex_lock(&blk_device.mutex) != 0)
 		die("pthread_mutex_lock");
 
 	offset		= port - IOPORT_VIRTIO_BLK;
@@ -108,7 +108,7 @@ static bool virtio_blk_pci_io_in(struct kvm *self, uint16_t port, void *data, in
 	};
 
 out_unlock:
-	if (pthread_mutex_unlock(&blk_device.mutex) < 0)
+	if (pthread_mutex_unlock(&blk_device.mutex) != 0)
 		die("pthread_mutex_unlock");
 
 	return ret;
@@ -180,7 +180,7 @@ static bool virtio_blk_pci_io_out(struct kvm *self, uint16_t port, void *data, i
 	unsigned long offset;
 	bool ret = true;
 
-	if (pthread_mutex_lock(&blk_device.mutex) < 0)
+	if (pthread_mutex_lock(&blk_device.mutex) != 0)
 		die("pthread_mutex_lock");
 
 	offset		= port - IOPORT_VIRTIO_BLK;
@@ -226,7 +226,7 @@ static bool virtio_blk_pci_io_out(struct kvm *self, uint16_t port, void *data, i
 	};
 
 out_unlock:
-	if (pthread_mutex_unlock(&blk_device.mutex) < 0)
+	if (pthread_mutex_unlock(&blk_device.mutex) != 0)
 		die("pthread_mutex_unlock");
 
 	return ret;
-- 
1.7.0.4

Re: [PATCH] kvm tools: Fix pthread mutex error checks

[Ingo Molnar]

* Pekka Enberg <penberg@kernel.org> wrote:

> The pthread_mutex_{lock|unlock} functions return non-zero, not negative number
> upon error. Fix that wrong assumption in the code.

glibc/pthreads mutex API semantics are pretty silly IMO.

I *think* it would be better to try to match the kernel API here, and provide 
trivial wrappers around mutex_lock()/mutex_unlock(). We wont ever bring down 
threads in a hostile way, so we wont actually need the error returns. CPU 
threads should probably only exit once the kvm process exits, after all cleanup 
has been done.

So mutex_lock() could be implemented as something like:

	void mutex_lock(pthread_mutex_t mutex)
	{
		if (pthread_mutex_lock(mutex) != 0)
			die("unexpected pthread_mutex_lock() failure!");
	}

That way usage would be more obvious and more familar to kernel developers :-) 

[ It would also open up the possibility, in the far future, to bring lockdep to 
  user-space ;-) ]

What do you think?

Thanks,

	Ingo