[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] lockable: replaced locks with lock guard macros where approp
|
From: |
Eric Blake |
|
Subject: |
Re: [PATCH] lockable: replaced locks with lock guard macros where appropriate |
|
Date: |
Thu, 19 Mar 2020 15:48:26 -0500 |
|
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.5.0 |
On 3/19/20 11:19 AM, address@hidden wrote:
From: danbrodsky <address@hidden>
- ran regexp "qemu_mutex_lock\(.*\).*\n.*if" to find targets
- replaced result with QEMU_LOCK_GUARD if all unlocks at function end
- replaced result with WITH_QEMU_LOCK_GUARD if unlock not at end
Signed-off-by: danbrodsky <address@hidden>
---
block/iscsi.c | 23 +++++++------------
block/nfs.c | 53 ++++++++++++++++++++-----------------------
cpus-common.c | 13 ++++-------
hw/display/qxl.c | 44 +++++++++++++++++------------------
hw/vfio/platform.c | 4 +---
migration/migration.c | 3 +--
migration/multifd.c | 8 +++----
migration/ram.c | 3 +--
monitor/misc.c | 4 +---
ui/spice-display.c | 14 ++++++------
util/log.c | 4 ++--
util/qemu-timer.c | 17 +++++++-------
util/rcu.c | 8 +++----
util/thread-pool.c | 3 +--
util/vfio-helpers.c | 4 ++--
15 files changed, 90 insertions(+), 115 deletions(-)
That's a rather big patch touching multiple areas of code at once; I'm
not sure if it would be easier to review if you were to break it up into
a series of smaller patches each touching a smaller group of related
files. For example, I don't mind reviwing block/, but tend to shy away
from migration/ code.
diff --git a/block/iscsi.c b/block/iscsi.c
index 682abd8e09..df73bde114 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -1086,23 +1086,21 @@ static BlockAIOCB *iscsi_aio_ioctl(BlockDriverState *bs,
acb->task->expxferlen = acb->ioh->dxfer_len;
data.size = 0;
- qemu_mutex_lock(&iscsilun->mutex);
+ QEMU_LOCK_GUARD(&iscsilun->mutex);
if (acb->task->xfer_dir == SCSI_XFER_WRITE) {
if (acb->ioh->iovec_count == 0) {
data.data = acb->ioh->dxferp;
data.size = acb->ioh->dxfer_len;
} else {
scsi_task_set_iov_out(acb->task,
- (struct scsi_iovec *) acb->ioh->dxferp,
- acb->ioh->iovec_count);
+ (struct scsi_iovec *)acb->ioh->dxferp,
+ acb->ioh->iovec_count);
This looks like a spurious whitespace change. Why is it part of the patch?
}
}
if (iscsi_scsi_command_async(iscsi, iscsilun->lun, acb->task,
iscsi_aio_ioctl_cb,
- (data.size > 0) ? &data : NULL,
- acb) != 0) {
- qemu_mutex_unlock(&iscsilun->mutex);
+ (data.size > 0) ? &data : NULL, acb) != 0) {
scsi_free_scsi_task(acb->task);
Unwrapping the line fit in 80 columns, but again, why are you mixing
whitespace changes in rather than focusing on the cleanup of mutex
actions? Did you create this patch mechanically with a tool like
Coccinelle, as the source of your reflowing of lines? If so, what was
the input to Coccinelle; if it was some other automated tool, can you
include the formula so that someone else could reproduce your changes
(whitespace and all)? If it was not automated, that's also okay, but
then I would not expect as much whitespace churn.
@@ -1111,18 +1109,16 @@ static BlockAIOCB *iscsi_aio_ioctl(BlockDriverState *bs,
/* tell libiscsi to read straight into the buffer we got from ioctl */
if (acb->task->xfer_dir == SCSI_XFER_READ) {
if (acb->ioh->iovec_count == 0) {
- scsi_task_add_data_in_buffer(acb->task,
- acb->ioh->dxfer_len,
+ scsi_task_add_data_in_buffer(acb->task, acb->ioh->dxfer_len,
acb->ioh->dxferp);
} else {
scsi_task_set_iov_in(acb->task,
- (struct scsi_iovec *) acb->ioh->dxferp,
+ (struct scsi_iovec *)acb->ioh->dxferp,
acb->ioh->iovec_count);
Again, spurious whitespace changes.
}
}
iscsi_set_events(iscsilun);
- qemu_mutex_unlock(&iscsilun->mutex);
return &acb->common;
}
@@ -1395,20 +1391,17 @@ static void iscsi_nop_timed_event(void *opaque)
{
IscsiLun *iscsilun = opaque;
- qemu_mutex_lock(&iscsilun->mutex);
+ QEMU_LOCK_GUARD(&iscsilun->mutex);
if (iscsi_get_nops_in_flight(iscsilun->iscsi) >= MAX_NOP_FAILURES) {
error_report("iSCSI: NOP timeout. Reconnecting...");
iscsilun->request_timed_out = true;
} else if (iscsi_nop_out_async(iscsilun->iscsi, NULL, NULL, 0, NULL) !=
0) {
error_report("iSCSI: failed to sent NOP-Out. Disabling NOP
messages.");
- goto out;
+ return;
}
timer_mod(iscsilun->nop_timer, qemu_clock_get_ms(QEMU_CLOCK_REALTIME) + NOP_INTERVAL);
iscsi_set_events(iscsilun);
-
-out:
- qemu_mutex_unlock(&iscsilun->mutex);
}
But the cleanup itself is functionally correct in this file.
static void iscsi_readcapacity_sync(IscsiLun *iscsilun, Error **errp)
diff --git a/block/nfs.c b/block/nfs.c
index 9a6311e270..37e8b82731 100644
--- a/block/nfs.c
+++ b/block/nfs.c
@@ -273,15 +273,14 @@ static int coroutine_fn nfs_co_preadv(BlockDriverState
*bs, uint64_t offset,
nfs_co_init_task(bs, &task);
task.iov = iov;
- qemu_mutex_lock(&client->mutex);
- if (nfs_pread_async(client->context, client->fh,
- offset, bytes, nfs_co_generic_cb, &task) != 0) {
- qemu_mutex_unlock(&client->mutex);
- return -ENOMEM;
- }
+ WITH_QEMU_LOCK_GUARD(&client->mutex) {
+ if (nfs_pread_async(client->context, client->fh,
+ offset, bytes, nfs_co_generic_cb, &task) != 0) {
+ return -ENOMEM;
+ }
- nfs_set_events(client);
- qemu_mutex_unlock(&client->mutex);
+ nfs_set_events(client);
+ }
while (!task.complete) {
qemu_coroutine_yield();
}
This one also looks correct.
@@ -290,7 +289,7 @@ static int coroutine_fn nfs_co_preadv(BlockDriverState *bs,
uint64_t offset,
return task.ret;
}
- /* zero pad short reads */
+/* zero pad short reads */
if (task.ret < iov->size) {
Another spurious whitespace change; in fact, the indentation on the
comment is now wrong.
+++ b/cpus-common.c
And that's as far as I feel comfortable reviewing.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org