Re: [libvirt] [PATCH 2/4] qemu: Introduce APIs for manipulating qemuDomainAgentJob

On 06/08/2018 09:45 AM, Michal Privoznik wrote: Looked at THREADS.txt before reading any code in order to get a sense for what's being done... I won't go back after reading the code so that you get a sense of how someone not knowing what's going on views things. So when reading comments here - just take them with that in mind. I have a feeling my mind/thoughts will change later on ;-). s/then// s/when thread/when the thread/ s/to qemu/to the qemu/ Oh this seems like it's going to be tricky... How does one choose which to use. At least w/ async you know it's a long running job and normal is everything else. Now you have category 3 agent specific - at this point I'm not sure you really want to mix normal and agent. If the purpose of the series is to extricate agent job from normal job, then mixing when it's convenient leads to speculation of what misunderstandings will occur for future consumers. Because of the text for "Normal job condition is used by all other jobs", I wonder if the agent job description should go above Normal job; otherwise, perhaps it'd be best to distinguish "qemu monitor" jobs in the normal description. There is no agent job condition, yet. And how on earth do you do both agent and normal? So no agent job before starting, but... ... we signal on job.cond even though we didn't necessarily obtain? Can you add a using a JobWithAgent example? Two empty lines (multiple occurrences w/ new functions). So you're competing with yourself with changes to this code from your other series. Who merges first ;-) Some random grumbling about splitting patches ;-) Part of me says this check is ripe for some future coding error, but logically AFAICT it works. So after reading the code and thinking about the documentation, IMO, having agentJob use job.cond is confusing. Having JobWithAgent is even more confusing. But it seems the code would work I think what concerns me is there are 3 paths (I read patch 4) that would seemingly indicate that by starting an agentJob we block a normal domain job. In order to start one, IIUC both agentJob and normal job must be 0. That seems reasonable until one starts thinking about races when there's a thread waiting for a JobWithAgent and a thread waiting for one or the other. Combine that with the check "if (!nested && !qemuDomainNestedJobAllowed(priv, job))" which may now pass NONE for @job if one of those threads was an agentJob. This could use an intro comment regarding usage. Read patch 3 and the following will make sense... if (agentJob == QEMU_AGENT_JOB_MODIFY_BLOCK) return qemuDomainObjBeginJobInternal(driver, obj, QEMU_JOB_MODIFY, agentJob, QEMU_ASYNC_JOB_NONE); else return qemuDomainObjBeginJobInternal(driver, obj, QEMU_JOB_NONE, agentJob, QEMU_ASYNC_JOB_NONE); if you got with the above, then this one's not necessary. Read patch 3 and this will make sense: if (priv->job.agentActive == QEMU_AGENT_JOB_MODIFY_BLOCK) { qemuDomainObjResetJob(priv); if (qemuDomainTrackJob(job)) qemuDomainObjSaveJob } Although the VIR_DEBUG above gets complicated... and this one goes away. John