[libvirt] [PATCH] util: remove unnecessary code to avoid error overwrite

[lhuang]

On 10/29/2015 05:21 AM, John Ferlan wrote:
>
> On 10/28/2015 03:06 AM, Luyao Huang wrote:
>> virCgroupRemove use VIR_ERROR to log the error and won't
>> change the virLastErr in the thread, so no need do this.
>>
>> Signed-off-by: Luyao Huang <lhuang at redhat.com>
>> ---
>>   src/util/vircgroup.c | 10 ----------
>>   1 file changed, 10 deletions(-)
>>
> Although virCgroupRemove calls virCgroupRemoveRecursively to remove the
> cgroups and as noted in the recursive function the usage of VIR_ERROR
> instead of virReportError is true.
>
> However, virCgroupRemove also calls virCgroupPathOfController which
> calls virReport[System]Error so I don't think we can 'remove' these
> overwrite protections... Consider the case when the recursive function
> is never called because we keep continuing.

You're right, i just check virCgroupRemoveRecursively() , and 
virCgroupPathOfController() calls virReport[System]Error. Then i think 
we need avoid  the error overwrite when call virCgroupPathOfController() 
in virCgroupRemove(). Like this:

diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
index 0379c2e..3ba6da3 100644
--- a/src/util/vircgroup.c
+++ b/src/util/vircgroup.c
@@ -3524,15 +3524,20 @@ virCgroupRemove(virCgroupPtr group)
          if (STREQ(group->controllers[i].placement, "/"))
              continue;

+        virErrorPtr saved = virSaveLastError();
          if (virCgroupPathOfController(group,
                                        i,
                                        NULL,
-                                      &grppath) != 0)
+                                      &grppath) != 0) {
+            virSetError(saved);
+            virFreeError(saved);
              continue;
+        }

          VIR_DEBUG("Removing cgroup %s and all child cgroups", grppath);
          rc = virCgroupRemoveRecursively(grppath);
          VIR_FREE(grppath);
+        virFreeError(saved);
      }
      VIR_DEBUG("Done removing cgroup %s", group->path);



Since we just skip remove the current cgroup if 
virCgroupPathOfController get failure, and return rc at last.So i think 
we really don't care about the error in virCgroupPathOfController in 
this place , and we should add some codes to avoid the error overwrite here.

Thanks a lot for your review and help.

Luyao

> John
>> diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
>> index 0379c2e..a120a15 100644
>> --- a/src/util/vircgroup.c
>> +++ b/src/util/vircgroup.c
>> @@ -1670,13 +1670,8 @@ virCgroupNewMachineSystemd(const char *name,
>>       }
>>   
>>       if (virCgroupAddTask(*group, pidleader) < 0) {
>> -        virErrorPtr saved = virSaveLastError();
>>           virCgroupRemove(*group);
>>           virCgroupFree(group);
>> -        if (saved) {
>> -            virSetError(saved);
>> -            virFreeError(saved);
>> -        }
>>       }
>>   
>>       ret = 0;
>> @@ -1728,13 +1723,8 @@ virCgroupNewMachineManual(const char *name,
>>           goto cleanup;
>>   
>>       if (virCgroupAddTask(*group, pidleader) < 0) {
>> -        virErrorPtr saved = virSaveLastError();
>>           virCgroupRemove(*group);
>>           virCgroupFree(group);
>> -        if (saved) {
>> -            virSetError(saved);
>> -            virFreeError(saved);
>> -        }
>>       }
>>   
>>    done:
>>