[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v2 08/40] job: Move state transitions to Job

From: Eric Blake
Subject: Re: [Qemu-devel] [PATCH v2 08/40] job: Move state transitions to Job
Date: Fri, 18 May 2018 09:36:57 -0500
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0

On 05/18/2018 08:20 AM, Kevin Wolf wrote:
This moves BlockJob.status and the closely related functions
(block_)job_state_transition() and (block_)job_apply_verb to Job. The
two QAPI enums are renamed to JobStatus and JobVerb.

Signed-off-by: Kevin Wolf <address@hidden>
Reviewed-by: Max Reitz <address@hidden>
Reviewed-by: John Snow <address@hidden>

@@ -1077,14 +1021,14 @@ void coroutine_fn block_job_pause_point(BlockJob *job)
if (block_job_should_pause(job) && !block_job_is_cancelled(job)) {
-        BlockJobStatus status = job->status;
-        block_job_state_transition(job, status == BLOCK_JOB_STATUS_READY ? \
-                                   BLOCK_JOB_STATUS_STANDBY :           \
-                                   BLOCK_JOB_STATUS_PAUSED);
+        JobStatus status = job->job.status;
+        job_state_transition(&job->job, status == JOB_STATUS_READY
+                                        ? JOB_STATUS_STANDBY
+                                        : JOB_STATUS_PAUSED);

Nice that you are getting rid of the pointless \. Here, you split the ?: operator by wrapping the ? onto the start of the next line...

+++ b/job.c

+int job_apply_verb(Job *job, JobVerb verb, Error **errp)
+    assert(verb >= 0 && verb <= JOB_VERB__MAX);
+    trace_job_apply_verb(job, JobStatus_str(job->status), JobVerb_str(verb),
+                         JobVerbTable[verb][job->status] ?
+                         "allowed" : "prohibited");

while here, you put it at the end of the previous line. We have a slight preference for one style over the other (there are also false positives in both of these greps):

$ git grep '?$' | wc -c
$ git grep '^ *?' | wc -c

But I don't care which style you use, other than pointing out that a consistent style within the patch doesn't hurt. :)

So, whether or not you make a whitespace-only tweak to one of those two spots,

Reviewed-by: Eric Blake <address@hidden>

Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

reply via email to

[Prev in Thread] Current Thread [Next in Thread]