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

[Martin Kletzander]

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 at redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20151022/b71235fc/attachment-0001.sig>