[libvirt] [PATCH 19/24] qemu: Introduce qemuProcessLaunch

[Jiri Denemark]

On Fri, Nov 13, 2015 at 10:17:16 -0500, John Ferlan wrote:
> 
> 
> On 11/12/2015 01:37 PM, Jiri Denemark wrote:
> > Once qemuProcessInit was called, qemuProcessLaunch will launch a new
> > QEMU process with stopped virtual CPUs.
> > 
> > Signed-off-by: Jiri Denemark <jdenemar at redhat.com>
> > ---
> >  src/qemu/qemu_process.c | 162 ++++++++++++++++++++++++++++++++----------------
> >  src/qemu/qemu_process.h |   9 +++
> >  2 files changed, 118 insertions(+), 53 deletions(-)
> > 
> 
> Been following along with the review so far - have a question though...
> 
> > diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> > index 5735935..0314c4a 100644
> > --- a/src/qemu/qemu_process.c
> > +++ b/src/qemu/qemu_process.c
> 
> [..]
> 
> > +int
> > +qemuProcessLaunch(virConnectPtr conn,
> > +                  virQEMUDriverPtr driver,
> > +                  virDomainObjPtr vm,
> > +                  qemuDomainAsyncJob asyncJob,
> > +                  qemuProcessIncomingDefPtr incoming,
> > +                  virDomainSnapshotObjPtr snapshot,
> > +                  virNetDevVPortProfileOp vmop,
> > +                  unsigned int flags)
> 
> [...]
> 
> >      VIR_DEBUG("Setting domain security labels");
> > -    if (virSecurityManagerSetAllLabel(driver->securityManager,
> > -                                      vm->def, migratePath) < 0)
> > +    if (incoming &&
> > +        virSecurityManagerSetAllLabel(driver->securityManager,
> > +                                      vm->def, incoming->path) < 0)
> 
> shouldn't this be
> 
>     if (virSecurityManagerSetAllLabel(driver->securityManager,
>                                       vm->def, incoming ?
>                                       incoming->path : NULL) < 0)
> 
> Previously we'd call SetAllLabel with migratePath as NULL anyway...
> 
> >          goto error;

Hmm, right you are. Thanks for catching this.

> 
> [...]
> 
> > +
> > + error:
> > +    /* We jump here if we failed to start the VM for any reason, or
> > +     * if we failed to initialize the now running VM. kill it off and
> > +     * pretend we never started it */
> > +    qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED, stop_flags);
> 
> Doesn't this happen in qemuProcessStart too?  IOW would this happen twice?

Well, yes, although running it twice does nothing except of logging a
"VM not active" debug message. However, a much bigger problem is in the
previous patch, which causes qemuProcessStop in case we fail to start a
domain because it is already running, not something you'd expect :-)

Jirka