[libvirt] [PATCH] qemu: lock: unlock vm during qemuBuildCommandLine

[Daniel P. Berrange]

On Thu, Apr 23, 2015 at 11:19:34AM +0200, Michal Privoznik wrote:
> On 23.04.2015 10:30, Peter Krempa wrote:
> > On Thu, Apr 23, 2015 at 11:44:49 +0800, zhang bo wrote:
> >> The function qemuBuildCommandLine() may take a long time, for example
> >> if we configure tens of vifs for the guest, each may cost hundrands of
> >> milliseconds to create tap dev, senconds in total. Thus, unlock vm
> >> before calling it.
> >>
> >> Signed-off-by: Zhang Bo <oscar.zhangbo at huawei.com>
> >> Signed-off-by: Zhou Yimin <zhouyimin at huawei.com>
> >> ---
> >>  src/qemu/qemu_process.c | 6 +++++-
> >>  1 file changed, 5 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> >> index 753afe8..d1aaaec 100644
> >> --- a/src/qemu/qemu_process.c
> >> +++ b/src/qemu/qemu_process.c
> >> @@ -4628,14 +4628,18 @@ int qemuProcessStart(virConnectPtr conn,
> >>      }
> >>
> >>      VIR_DEBUG("Building emulator command line");
> >> +    virObjectUnlock(vm);
> >>      if (!(cmd = qemuBuildCommandLine(conn, driver, vm->def, priv->monConfig,
> >>                                       priv->monJSON, priv->qemuCaps,
> >>                                       migrateFrom, stdin_fd, snapshot, vmop,
> >>                                       &buildCommandLineCallbacks, false,
> >>                                       qemuCheckFips(),
> >>                                       priv->autoNodeset,
> >> -                                     &nnicindexes, &nicindexes)))
> >> +                                     &nnicindexes, &nicindexes))) {
> >> +        virObjectLock(vm);
> >>          goto cleanup;
> >> +    }
> >> +    virObjectLock(vm);
> > 
> > Why do you need to unlock the object? The VM is starting at this point
> > so you won't be able to save any time since APIs will either be blocked
> > by a job or by the fact that the VM was not started.
> 
> Not true. We still allow QUERY jobs, or APIs that lock a domain object
> but don't necessarily take a job. For instance, if in one thread I'm
> doing virDomainGetOSType(), virDomainGetInfo(), ... (essentially 'virsh
> dominfo') while the other just starts the domain. I don't see a reason
> why virDomainGetOSType() should wait for the domain to be started up.
> Domain state should have no affect on the guest OS type, should it?
> 
> On the other hand, I don't think we can just lock and unlock the domain
> object as we please. qemuBuildCommandLine is a very complex function and
> as such it calls many others. Some of them may rely on the fact, that
> the object is locked by caller.

Yeah, I'm pretty wary of allowing unlocking of the config while in the
build command line function.

I think I'd rather than it were refactored so that it was always fast
and has no side effects on external system state.

ie, we should allocate all the TAP devices upfront in a separate
function, and then just pass in the list of TAP device file
descriptors to qemuBuildCommandLine so it can generate the CLI
args fast.

Then we can reliably determine that the code which allocates TAP
devices is safe to run unlocked.

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 :|