1. Patchew
  2. Libvirt
  3. [libvirt] [PATCH 0/2] Fix cgroupv2 issue on Fedora 31
  4. View patch

[libvirt] [PATCH 2/2] vircgroup: Add some VIR_DEBUG statements

Cole Robinson posted 2 patches 6 years, 4 months ago
[libvirt] [PATCH 2/2] vircgroup: Add some VIR_DEBUG statements
Posted by Cole Robinson 6 years, 4 months ago 
These helped with debugging
https://bugzilla.redhat.com/show_bug.cgi?id=1612383

Signed-off-by: Cole Robinson <crobinso@redhat.com>
---
 src/util/vircgroup.c   | 3 ++-
 src/util/vircgroupv2.c | 9 +++++++++
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
index 825f62a97b..4f9d80666d 100644
--- a/src/util/vircgroup.c
+++ b/src/util/vircgroup.c
@@ -1157,7 +1157,8 @@ virCgroupNewMachineSystemd(const char *name,
     virCgroupFree(&init);
 
     if (!path || STREQ(path, "/") || path[0] != '/') {
-        VIR_DEBUG("Systemd didn't setup its controller");
+        VIR_DEBUG("Systemd didn't setup its controller, path=%s",
+                  NULLSTR(path));
         return -2;
     }
 
diff --git a/src/util/vircgroupv2.c b/src/util/vircgroupv2.c
index 0cb20e4896..f3f83c1e95 100644
--- a/src/util/vircgroupv2.c
+++ b/src/util/vircgroupv2.c
@@ -155,10 +155,14 @@ virCgroupV2CopyPlacement(virCgroupPtr group,
                          const char *path,
                          virCgroupPtr parent)
 {
+    VIR_DEBUG("group=%p path=%s parent=%p", group, path, parent);
+
     if (path[0] == '/') {
         if (VIR_STRDUP(group->unified.placement, path) < 0)
             return -1;
     } else {
+        VIR_DEBUG("parent->unified.placement=%s", parent->unified.placement);
+
         /*
          * parent == "/" + path="" => "/"
          * parent == "/libvirt.service" + path == "" => "/libvirt.service"
@@ -172,6 +176,7 @@ virCgroupV2CopyPlacement(virCgroupPtr group,
             return -1;
     }
 
+    VIR_DEBUG("set group->unified.placement=%s", group->unified.placement);
     return 0;
 }
 
@@ -200,6 +205,9 @@ virCgroupV2DetectPlacement(virCgroupPtr group,
     if (group->unified.placement)
         return 0;
 
+    VIR_DEBUG("group=%p path=%s controllers=%s selfpath=%s",
+              group, path, controllers, selfpath);
+
     /* controllers="" indicates the cgroupv2 controller path */
     if (STRNEQ_NULLABLE(controllers, ""))
         return 0;
@@ -216,6 +224,7 @@ virCgroupV2DetectPlacement(virCgroupPtr group,
                     path) < 0)
         return -1;
 
+    VIR_DEBUG("set group->unified.placement=%s", group->unified.placement);
     return 0;
 }
 
-- 
2.23.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] vircgroup: Add some VIR_DEBUG statements
Posted by Pavel Hrdina 6 years, 4 months ago 
On Thu, Sep 26, 2019 at 05:18:41PM -0400, Cole Robinson wrote:
> These helped with debugging
> https://bugzilla.redhat.com/show_bug.cgi?id=1612383
> 
> Signed-off-by: Cole Robinson <crobinso@redhat.com>
> ---
>  src/util/vircgroup.c   | 3 ++-
>  src/util/vircgroupv2.c | 9 +++++++++
>  2 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
> index 825f62a97b..4f9d80666d 100644
> --- a/src/util/vircgroup.c
> +++ b/src/util/vircgroup.c
> @@ -1157,7 +1157,8 @@ virCgroupNewMachineSystemd(const char *name,
>      virCgroupFree(&init);
>  
>      if (!path || STREQ(path, "/") || path[0] != '/') {
> -        VIR_DEBUG("Systemd didn't setup its controller");
> +        VIR_DEBUG("Systemd didn't setup its controller, path=%s",
> +                  NULLSTR(path));
>          return -2;
>      }
>  
> diff --git a/src/util/vircgroupv2.c b/src/util/vircgroupv2.c
> index 0cb20e4896..f3f83c1e95 100644
> --- a/src/util/vircgroupv2.c
> +++ b/src/util/vircgroupv2.c
> @@ -155,10 +155,14 @@ virCgroupV2CopyPlacement(virCgroupPtr group,
>                           const char *path,
>                           virCgroupPtr parent)
>  {
> +    VIR_DEBUG("group=%p path=%s parent=%p", group, path, parent);
> +
>      if (path[0] == '/') {
>          if (VIR_STRDUP(group->unified.placement, path) < 0)
>              return -1;
>      } else {
> +        VIR_DEBUG("parent->unified.placement=%s", parent->unified.placement);
> +

This one seems a bit redundant as the parent->unified.placement will be
part of the group->unified.placement together with path.

>          /*
>           * parent == "/" + path="" => "/"
>           * parent == "/libvirt.service" + path == "" => "/libvirt.service"
> @@ -172,6 +176,7 @@ virCgroupV2CopyPlacement(virCgroupPtr group,
>              return -1;
>      }
>  
> +    VIR_DEBUG("set group->unified.placement=%s", group->unified.placement);
>      return 0;
>  }
>  
> @@ -200,6 +205,9 @@ virCgroupV2DetectPlacement(virCgroupPtr group,
>      if (group->unified.placement)
>          return 0;
>  
> +    VIR_DEBUG("group=%p path=%s controllers=%s selfpath=%s",
> +              group, path, controllers, selfpath);
> +
>      /* controllers="" indicates the cgroupv2 controller path */
>      if (STRNEQ_NULLABLE(controllers, ""))
>          return 0;
> @@ -216,6 +224,7 @@ virCgroupV2DetectPlacement(virCgroupPtr group,
>                      path) < 0)
>          return -1;
>  
> +    VIR_DEBUG("set group->unified.placement=%s", group->unified.placement);
>      return 0;

When I was writing the code I had a lot of debug messages around the
code but before posting the patches I removed a lot of it in order to
not spam debug logs with everything.  Finding the sweet spot is
difficult, the function entry debug logs are definitely good and can
help a lot to track the code workflow, but I'm not so sure about the
debug logs at the end of functions to log what was configured.  I'll
leave it up to you whether you want to keep it or not.

Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] vircgroup: Add some VIR_DEBUG statements
Posted by Cole Robinson 6 years, 4 months ago 
On 9/27/19 7:03 AM, Pavel Hrdina wrote:
> On Thu, Sep 26, 2019 at 05:18:41PM -0400, Cole Robinson wrote:
>> These helped with debugging
>> https://bugzilla.redhat.com/show_bug.cgi?id=1612383
>>
>> Signed-off-by: Cole Robinson <crobinso@redhat.com>
>> ---
>>   src/util/vircgroup.c   | 3 ++-
>>   src/util/vircgroupv2.c | 9 +++++++++
>>   2 files changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
>> index 825f62a97b..4f9d80666d 100644
>> --- a/src/util/vircgroup.c
>> +++ b/src/util/vircgroup.c
>> @@ -1157,7 +1157,8 @@ virCgroupNewMachineSystemd(const char *name,
>>       virCgroupFree(&init);
>>   
>>       if (!path || STREQ(path, "/") || path[0] != '/') {
>> -        VIR_DEBUG("Systemd didn't setup its controller");
>> +        VIR_DEBUG("Systemd didn't setup its controller, path=%s",
>> +                  NULLSTR(path));
>>           return -2;
>>       }
>>   
>> diff --git a/src/util/vircgroupv2.c b/src/util/vircgroupv2.c
>> index 0cb20e4896..f3f83c1e95 100644
>> --- a/src/util/vircgroupv2.c
>> +++ b/src/util/vircgroupv2.c
>> @@ -155,10 +155,14 @@ virCgroupV2CopyPlacement(virCgroupPtr group,
>>                            const char *path,
>>                            virCgroupPtr parent)
>>   {
>> +    VIR_DEBUG("group=%p path=%s parent=%p", group, path, parent);
>> +
>>       if (path[0] == '/') {
>>           if (VIR_STRDUP(group->unified.placement, path) < 0)
>>               return -1;
>>       } else {
>> +        VIR_DEBUG("parent->unified.placement=%s", parent->unified.placement);
>> +
> 
> This one seems a bit redundant as the parent->unified.placement will be
> part of the group->unified.placement together with path.
> 
>>           /*
>>            * parent == "/" + path="" => "/"
>>            * parent == "/libvirt.service" + path == "" => "/libvirt.service"
>> @@ -172,6 +176,7 @@ virCgroupV2CopyPlacement(virCgroupPtr group,
>>               return -1;
>>       }
>>   
>> +    VIR_DEBUG("set group->unified.placement=%s", group->unified.placement);
>>       return 0;
>>   }
>>   
>> @@ -200,6 +205,9 @@ virCgroupV2DetectPlacement(virCgroupPtr group,
>>       if (group->unified.placement)
>>           return 0;
>>   
>> +    VIR_DEBUG("group=%p path=%s controllers=%s selfpath=%s",
>> +              group, path, controllers, selfpath);
>> +
>>       /* controllers="" indicates the cgroupv2 controller path */
>>       if (STRNEQ_NULLABLE(controllers, ""))
>>           return 0;
>> @@ -216,6 +224,7 @@ virCgroupV2DetectPlacement(virCgroupPtr group,
>>                       path) < 0)
>>           return -1;
>>   
>> +    VIR_DEBUG("set group->unified.placement=%s", group->unified.placement);
>>       return 0;
> 
> When I was writing the code I had a lot of debug messages around the
> code but before posting the patches I removed a lot of it in order to
> not spam debug logs with everything.  Finding the sweet spot is
> difficult, the function entry debug logs are definitely good and can
> help a lot to track the code workflow, but I'm not so sure about the
> debug logs at the end of functions to log what was configured.  I'll
> leave it up to you whether you want to keep it or not.
> 
> Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
> 

I fixed your suggestions and dropped the debug statements at the end of 
functions, I think the entry ones are sufficient

Thanks,
Cole

- Cole

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.