Hi all. Looking at the qemu driver code, we have a lot of code with the following pattern: if (doSomething() < 0) goto cleanup; if (qemuDomainObjBeginJob() < 0) goto cleanup; if (doSomethingElse() < 0) goto endjob; qemuDomainObjEnterMonitor(); if (qemuMonitorSomething() < 0) { qemuDomainObjExitMonitor(); goto endjob; } if (qemuMonitorSomethingElse() < 0) { qemuDomainObjExitMonitor(); goto endjob; } if (qemuDomainObjExitMonitor() < 0) goto endjob; ... ret = 0; endjob: qemuDomainObjEndJob(); cleanup: ... return ret; Sometimes qemuDomainObjExitMonitor is in its own label instead of being explicitly called on every single error path. Sometimes qemuDomainObjEndJob is called explicitly followed by goto cleanup. In either case, it's pretty easy to get this wrong. It's too easy to jump to a wrong label (especially when moving code around) or forget to call the appropriate exit function before jumping to a label. So what if we make the code more robust by changing who needs to track whether we are in a monitor or have a job. Now it's the caller that tracks it while I think we could teach the job/monitor APIs themselves to track the state: bool inJob = false; bool inMonitor = false; if (doSomething() < 0) goto cleanup; if (qemuDomainObjBeginJob(..., &inJob) < 0) goto cleanup; if (doSomethingElse() < 0) goto cleanup; qemuDomainObjEnterMonitor(..., &inMonitor); if (qemuMonitorSomething() < 0) goto cleanup; if (qemuMonitorSomethingElse() < 0) goto cleanup; if (qemuDomainObjExitMonitor(..., &inMonitor) < 0) goto cleanup; ... ret = 0; cleanup: if (qemuDomainObjExitMonitor(..., &inMonitor) < 0) ret = -1; qemuDomainObjEndJob(..., &inJob); ... return ret; It's not a lot shorter or longer but it saves us from jumping to different labels and it makes sure we always exit the monitor and end the job. So is it worth the effort or do you thing it's even worse than before or do you have any other ideas?

On Thu, Oct 22, 2015 at 13:52:29 +0100, Daniel P. Berrange wrote: ... > On a related topic, we don't have great error reporting in the (usually > unlikely) scenario that we get a stuck job / timeout. I've long thought > it could be desirable to record some metadata when we start jobs, such > as the __FUNC__ of the method which started the job, so when we report > an error we can include that info as a diagnostic aid. Do you mean something like virsh # resume cd error: Failed to resume domain cd error: Timed out during operation: cannot acquire state change lock (held by remoteDispatchDomainSuspend) This was implemented by v1.2.13-295-gb79f25e > This would > have to be against the qemuDomainObjPrivPtr struct. THis makes me > think that using the separate bool inJob/inMonitor stack variables > is not required. > > We could just add > > int threadid; > bool inJob; > bool inMonitor; > const char *jobfunc; > > to qemuDomainObjPrivPtr. That way you don't need to modify the > Enter/Exit functions to add extra arguments - we just track > everything internally. When exiting, we'd compare against the > threadid, to make sure we don't accidentally relaase a different > thread's job. Yeah, as long as we can make sure threadid is unique and stable: /* These next two functions are for debugging only, since they are not * guaranteed to give unique values for distinct threads on all * architectures, nor are the two functions guaranteed to give the same * value for the same thread. */ unsigned long long virThreadSelfID(void); unsigned long long virThreadID(virThreadPtr thread); so far we avoided using thread IDs for anything critical.