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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list