On 02/13/2015 04:45 AM, John Ferlan wrote:
On 02/04/2015 08:42 AM, Luyao Huang wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1176503
When guest start failed, libvirt will keep the current vm->def,
this will make a issue that we cannot get a right xml after guest
start failed. And don't call the stop/release hook to do some
other clean work.
Call virLXCProcessCleanup to help us clean the source and call
the hooks if start a vm failed
Signed-off-by: Luyao Huang <lhu...@redhat.com>
---
v2: use virLXCProcessCleanup to free the source and call the hook.
v3: rework the patch to suit the virLXCProcessStart code changed.
src/lxc/lxc_process.c | 76 ++++++++++++++++++++++-----------------------------
1 file changed, 32 insertions(+), 44 deletions(-)
FWIW:
v1 review starts here:
http://www.redhat.com/archives/libvir-list/2015-January/msg00112.html
At first I was 'concerned' about the number of changes, but after doing
more investigation - I think the patch is correct in general; however, I
think it needs some adjustments as there are really two bugs here...
I'll send an update shortly for comparison/review...
Yes, i agree about these patch need more adjustment, because i think
maybe there is a better way to fix these issue but i cannot find them ;)
and thanks for your patches.
Essentially though - I think the console check*s* could be done earlier
before we get into other setup that requires going thru "cleanup:" (or
what error: was). That's "one bug" - which is a configuration error
which will prevent startup. Since it's easier to check early, provide an
error, and return -1 - that's what I think is cleaner solution.
Looks good for me
Second, the other bug which you were trying to clean up at the same time
is that existing cleanup paths don't properly clean all things up. The
error path worked only when/once the container started and some
pre-container start cleanup was done, but a few important ones were
missing - so that's a separate patch.
I also will add a patch to add/modify the debugging to help future such
efforts...
Thanks
BTW:
It seems commit id '88a1b542' from Cédric Bosdonnat caused yet another
merge conflict and seemingly from the description might have been in the
same area, but alas a quick check shows, it wasn't the same issue.
I'll leave the notes I was developing on this patch prior to just biting
the bullet and reformatting so you can perhaps "see" my thought process.
Yes, commit 88a1b542 is different from the issue i try to fix.
diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c
index 01da344..1a6cfbb 100644
--- a/src/lxc/lxc_process.c
+++ b/src/lxc/lxc_process.c
@@ -1022,6 +1022,7 @@ int virLXCProcessStart(virConnectPtr conn,
virCgroupPtr selfcgroup;
int status;
char *pidfile = NULL;
+ bool need_stop = false;
I think the check:
if (vm->def->nconsoles == 0) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("At least one PTY console is required"));
goto cleanup;
}
Should perhaps be moved to just before this code:
nttyFDs = vm->def->nconsoles;
if (VIR_ALLOC_N(ttyFDs, nttyFDs) < 0)
goto cleanup;
for (i = 0; i < vm->def->nconsoles; i++)
ttyFDs[i] = -1;
and that would "fix" the bug from the bz... as well as ensuring we don't
have a "if (VIR_ALLOC_N(ttdFDs, 0) < 0)"... In fact is there a reason
why that check cannot move much higher and be after the cgroup checks
which return -1? While t it, why not move the following too:
for (i = 0; i < vm->def->nconsoles; i++) {
if (vm->def->consoles[i]->source.type != VIR_DOMAIN_CHR_TYPE_PTY) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("Only PTY console types are supported"));
goto cleanup;
}
}
and then remove that from the loop later on.
Yes, after your words, i think console check in this place should be
improved.
This way checks that go to cleanup are a result of some error in
processing rather than some container configuration error...
Then the remainder of the code could be perhaps patch 2 which is fixing
a different, although related problem.
if (virCgroupNewSelf(&selfcgroup) < 0)
return -1;
@@ -1259,6 +1260,7 @@ int virLXCProcessStart(virConnectPtr conn,
goto cleanup;
}
...
- VIR_FREE(vm->def->seclabels[0]->imagelabel);
- VIR_DELETE_ELEMENT(vm->def->seclabels, 0, vm->def->nseclabels);
+ err = virSaveLastError();
+ if (need_stop) {
+ virLXCProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED);
+ } else {
+ virSecurityManagerRestoreAllLabel(driver->securityManager,
+ vm->def, false);
+ virSecurityManagerReleaseLabel(driver->securityManager, vm->def);
+ /* Clear out dynamically assigned labels */
+ if (vm->def->nseclabels &&
+ vm->def->seclabels[0]->type == VIR_DOMAIN_SECLABEL_DYNAMIC) {
NOTE: This is where the main conflict arises with the now new check for
'|| clearSeclabel' ...
Yes, this place have been changed and need rework this place, but i saw
you have done this in v4 , thanks a lot for your help:)
+ VIR_FREE(vm->def->seclabels[0]->model);
+ VIR_FREE(vm->def->seclabels[0]->label);
+ VIR_FREE(vm->def->seclabels[0]->imagelabel);
+ VIR_DELETE_ELEMENT(vm->def->seclabels, 0, vm->def->nseclabels);
+ }
+ virLXCProcessCleanup(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED);
Somewhat related - virLXCProcessCleanup has a VIR_DEBUG to start
"Stopping..." - perhaps that should be changed to "Cleanup..." since
"virLXCProcessStop already has "Stopping..." - that might help
differentiate for someone debugging in the future...
yes, the debug information should be improved.
Thanks for your review.
John
}
}
+ virCommandFree(cmd);
+ for (i = 0; i < nveths; i++)
+ VIR_FREE(veths[i]);
for (i = 0; i < nttyFDs; i++)
VIR_FORCE_CLOSE(ttyFDs[i]);
VIR_FREE(ttyFDs);
@@ -1415,11 +1408,6 @@ int virLXCProcessStart(virConnectPtr conn,
}
return rc;
-
- error:
- err = virSaveLastError();
- virLXCProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED);
- goto cleanup;
}
struct virLXCProcessAutostartData {
Luyao
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
