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@xxxxxxxxxx>
> > ---
> > 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list