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?

It's good and similar to what I have though of few times as well. I have just two ideas for making it even better. Firstly, there is one thing we need to fix and that is that bunch of APIs should have a job and they don't. Basically every API shold have a job, even if it's just a QEMU_JOB_QUERY, that's why we have masks that say which jobs are allowed when. So what if we had one function that fetches the domain object and sets a job there? That would solve few mishaps and made the code a bit shorter. Another thing is that since you know whether to call ExitMonitor and EndJob, you can incorporate them into one function, like there was the qemuDomObjEndAPI() if you put the inMonitor and inJob variables somewhere accessible. Later on, if we introduce job control everywhere, we could make it part of virDomainObjEndAPI() and the starting function would be also driver-agnostic. But that's for the future (and we all know the future was yesterday). Anyway, I think it's a good idea and with some polish (lowercase 'p') it'll rock.

Thanks,

Jirka

-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list