On Thu, Mar 19, 2020 at 1:48 PM Eric Blake <ebl...@redhat.com> wrote:
>
> On 3/19/20 11:19 AM, dnbrd...@gmail.com wrote:
> > From: danbrodsky <dnbrd...@gmail.com>
> >
> > - 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 <dnbrd...@gmail.com>
> > ---
> > 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.
Is this necessary for a series of fairly basic changes? Most files are only
modified on 1 or 2 lines.
>
> >
> > 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?
>
Sorry, it looks like my editor was autoformatting some areas of the text.
I'll remove
those changes in the next version.
> > }
> > }
> >
> > 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.
>
Should I not be including changes that fix warnings in code check? I'll
correct
the mistakes and submit a new version without all the whitespace churn.
