[RFC] Simplifying usage of {Enter, Exit}Monitor and {Begin/End}Job

[Jiri Denemark <jdenemar@xxxxxxxxxx>]

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?

Thanks,

Jirka

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