[libvirt] [PATCH v3] lxc: fix show the wrong xml when guest start failed

[lhuang]

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 <lhuang at 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