On Thu, Oct 22, 2015 at 01:47:28PM +0200, Jiri Denemark wrote:
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 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. 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. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|