[pve-devel] [PATCH cluster 1/5] pmxcfs: status: add one more missing g_free

[pve-devel] [PATCH cluster 1/5] pmxcfs: status: add one more missing g_free

[Lukas Wagner]

Otherwise we leak the memory that was already allocated for `filename`.

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
 src/pmxcfs/status.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/pmxcfs/status.c b/src/pmxcfs/status.c
index a00e793..886d0bd 100644
--- a/src/pmxcfs/status.c
+++ b/src/pmxcfs/status.c
@@ -1369,6 +1369,8 @@ static void update_rrd_data(const char *key, gconstpointer data, size_t len) {
         } else if (g_file_test(filename_pve2, G_FILE_TEST_EXISTS)) {
             // old file exists, use it
             use_pve2_file = 1;
+
+            g_free(filename);
             filename = g_strdup_printf("%s", filename_pve2);
         } else {
             // neither file exists, check for directories to decide and create file
-- 
2.47.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH cluster 2/5] pmxcfs: status: avoid unnecessary string allocations

[Lukas Wagner]

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
 src/pmxcfs/status.c | 24 ++++++------------------
 1 file changed, 6 insertions(+), 18 deletions(-)

diff --git a/src/pmxcfs/status.c b/src/pmxcfs/status.c
index 886d0bd..e438d9f 100644
--- a/src/pmxcfs/status.c
+++ b/src/pmxcfs/status.c
@@ -1374,14 +1374,12 @@ static void update_rrd_data(const char *key, gconstpointer data, size_t len) {
             filename = g_strdup_printf("%s", filename_pve2);
         } else {
             // neither file exists, check for directories to decide and create file
-            char *dir_pve2 = g_strdup_printf(RRDDIR "/pve2-node");
-            char *dir_pve90 = g_strdup_printf(RRDDIR "/pve-node-9.0");
 
-            if (g_file_test(dir_pve90, G_FILE_TEST_IS_DIR)) {
+            if (g_file_test(RRDDIR "/pve-node-9.0", G_FILE_TEST_IS_DIR)) {
 
                 int argcount = sizeof(rrd_def_node_pve9_0) / sizeof(void *) - 1;
                 create_rrd_file(filename, argcount, rrd_def_node_pve9_0);
-            } else if (g_file_test(dir_pve2, G_FILE_TEST_IS_DIR)) {
+            } else if (g_file_test(RRDDIR "/pve2-node", G_FILE_TEST_IS_DIR)) {
                 use_pve2_file = 1;
 
                 g_free(filename);
@@ -1404,8 +1402,6 @@ static void update_rrd_data(const char *key, gconstpointer data, size_t len) {
                 int argcount = sizeof(rrd_def_node_pve9_0) / sizeof(void *) - 1;
                 create_rrd_file(filename, argcount, rrd_def_node_pve9_0);
             }
-            g_free(dir_pve2);
-            g_free(dir_pve90);
         }
 
         skip = 2; // first two columns are live data that isn't archived
@@ -1450,14 +1446,12 @@ static void update_rrd_data(const char *key, gconstpointer data, size_t len) {
             filename = g_strdup_printf("%s", filename_pve2);
         } else {
             // neither file exists, check for directories to decide and create file
-            char *dir_pve2 = g_strdup_printf(RRDDIR "/pve2-vm");
-            char *dir_pve90 = g_strdup_printf(RRDDIR "/pve-vm-9.0");
 
-            if (g_file_test(dir_pve90, G_FILE_TEST_IS_DIR)) {
+            if (g_file_test(RRDDIR "/pve-vm-9.0", G_FILE_TEST_IS_DIR)) {
 
                 int argcount = sizeof(rrd_def_vm_pve9_0) / sizeof(void *) - 1;
                 create_rrd_file(filename, argcount, rrd_def_vm_pve9_0);
-            } else if (g_file_test(dir_pve2, G_FILE_TEST_IS_DIR)) {
+            } else if (g_file_test(RRDDIR "/pve2-vm", G_FILE_TEST_IS_DIR)) {
                 use_pve2_file = 1;
 
                 g_free(filename);
@@ -1472,8 +1466,6 @@ static void update_rrd_data(const char *key, gconstpointer data, size_t len) {
                 int argcount = sizeof(rrd_def_vm_pve9_0) / sizeof(void *) - 1;
                 create_rrd_file(filename, argcount, rrd_def_vm_pve9_0);
             }
-            g_free(dir_pve2);
-            g_free(dir_pve90);
         }
 
         skip = 4; // first 4 columns are live data that isn't archived
@@ -1520,17 +1512,15 @@ static void update_rrd_data(const char *key, gconstpointer data, size_t len) {
             filename = g_strdup_printf("%s", filename_pve2);
         } else {
             // neither file exists, check for directories to decide and create file
-            char *dir_pve2 = g_strdup_printf(RRDDIR "/pve2-storage");
-            char *dir_pve90 = g_strdup_printf(RRDDIR "/pve-storage-9.0");
 
-            if (g_file_test(dir_pve90, G_FILE_TEST_IS_DIR)) {
+            if (g_file_test(RRDDIR "/pve-storage-9.0", G_FILE_TEST_IS_DIR)) {
                 char *dir = g_path_get_dirname(filename);
                 checked_mkdir(dir, 0755);
                 g_free(dir);
 
                 int argcount = sizeof(rrd_def_storage_pve9_0) / sizeof(void *) - 1;
                 create_rrd_file(filename, argcount, rrd_def_storage_pve9_0);
-            } else if (g_file_test(dir_pve2, G_FILE_TEST_IS_DIR)) {
+            } else if (g_file_test(RRDDIR "/pve2-storage", G_FILE_TEST_IS_DIR)) {
                 g_free(filename);
                 filename = g_strdup_printf("%s", filename_pve2);
 
@@ -1551,8 +1541,6 @@ static void update_rrd_data(const char *key, gconstpointer data, size_t len) {
                 int argcount = sizeof(rrd_def_storage_pve9_0) / sizeof(void *) - 1;
                 create_rrd_file(filename, argcount, rrd_def_storage_pve9_0);
             }
-            g_free(dir_pve2);
-            g_free(dir_pve90);
         }
 
         // actual data columns didn't change between pve2-storage and pve-storage-9.0
-- 
2.47.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH cluster 3/5] pmxcfs: status: call g_strdup instead of g_strdup_printf when no formatting is needed

[Lukas Wagner]

This should be a tiny bit more efficient.

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
 src/pmxcfs/status.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/src/pmxcfs/status.c b/src/pmxcfs/status.c
index e438d9f..c8e072f 100644
--- a/src/pmxcfs/status.c
+++ b/src/pmxcfs/status.c
@@ -1371,7 +1371,7 @@ static void update_rrd_data(const char *key, gconstpointer data, size_t len) {
             use_pve2_file = 1;
 
             g_free(filename);
-            filename = g_strdup_printf("%s", filename_pve2);
+            filename = g_strdup(filename_pve2);
         } else {
             // neither file exists, check for directories to decide and create file
 
@@ -1383,7 +1383,7 @@ static void update_rrd_data(const char *key, gconstpointer data, size_t len) {
                 use_pve2_file = 1;
 
                 g_free(filename);
-                filename = g_strdup_printf("%s", filename_pve2);
+                filename = g_strdup(filename_pve2);
 
                 char *dir = g_path_get_dirname(filename);
                 checked_mkdir(dir, 0755);
@@ -1443,7 +1443,7 @@ static void update_rrd_data(const char *key, gconstpointer data, size_t len) {
             // old file exists, use it
             use_pve2_file = 1;
             g_free(filename);
-            filename = g_strdup_printf("%s", filename_pve2);
+            filename = g_strdup(filename_pve2);
         } else {
             // neither file exists, check for directories to decide and create file
 
@@ -1455,7 +1455,7 @@ static void update_rrd_data(const char *key, gconstpointer data, size_t len) {
                 use_pve2_file = 1;
 
                 g_free(filename);
-                filename = g_strdup_printf("%s", filename_pve2);
+                filename = g_strdup(filename_pve2);
 
                 int argcount = sizeof(rrd_def_vm) / sizeof(void *) - 1;
                 create_rrd_file(filename, argcount, rrd_def_vm);
@@ -1509,7 +1509,7 @@ static void update_rrd_data(const char *key, gconstpointer data, size_t len) {
         } else if (g_file_test(filename_pve2, G_FILE_TEST_EXISTS)) {
             // old file exists, use it
             g_free(filename);
-            filename = g_strdup_printf("%s", filename_pve2);
+            filename = g_strdup(filename_pve2);
         } else {
             // neither file exists, check for directories to decide and create file
 
@@ -1522,7 +1522,7 @@ static void update_rrd_data(const char *key, gconstpointer data, size_t len) {
                 create_rrd_file(filename, argcount, rrd_def_storage_pve9_0);
             } else if (g_file_test(RRDDIR "/pve2-storage", G_FILE_TEST_IS_DIR)) {
                 g_free(filename);
-                filename = g_strdup_printf("%s", filename_pve2);
+                filename = g_strdup(filename_pve2);
 
                 char *dir = g_path_get_dirname(filename);
                 checked_mkdir(dir, 0755);
-- 
2.47.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH cluster 4/5] pmxcfs: status: remove string literal params for g_strdup_printf

[Lukas Wagner]

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
 src/pmxcfs/status.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/pmxcfs/status.c b/src/pmxcfs/status.c
index c8e072f..e9e28bb 100644
--- a/src/pmxcfs/status.c
+++ b/src/pmxcfs/status.c
@@ -1431,7 +1431,7 @@ static void update_rrd_data(const char *key, gconstpointer data, size_t len) {
         }
 
         filename = g_strdup_printf(RRDDIR "/pve-vm-9.0/%s", vmid);
-        char *filename_pve2 = g_strdup_printf(RRDDIR "/%s/%s", "pve2-vm", vmid);
+        char *filename_pve2 = g_strdup_printf(RRDDIR "/pve2-vm/%s", vmid);
 
         int use_pve2_file = 0;
 
@@ -1500,7 +1500,7 @@ static void update_rrd_data(const char *key, gconstpointer data, size_t len) {
         }
 
         filename = g_strdup_printf(RRDDIR "/pve-storage-9.0/%s", node);
-        char *filename_pve2 = g_strdup_printf(RRDDIR "/%s/%s", "pve2-storage", node);
+        char *filename_pve2 = g_strdup_printf(RRDDIR "/pve2-storage/%s", node);
 
         // check existing rrd files and directories
         if (g_file_test(filename, G_FILE_TEST_EXISTS)) {
-- 
2.47.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH cluster 5/5] pmxcfs: status: avoid g_strdup when not needed

[Lukas Wagner]

`filename_pve2` is not accessed anymore after assigning the copy to
`filename`, so we can simply 'transfer' the ownership to `filename`
and set `filenname_pve2` to NULL.

The call to `g_free(filename_pve2)` later can handle a NULL arg, it will
simply do nothing in this case.

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
 src/pmxcfs/status.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/src/pmxcfs/status.c b/src/pmxcfs/status.c
index e9e28bb..1b848b0 100644
--- a/src/pmxcfs/status.c
+++ b/src/pmxcfs/status.c
@@ -1371,7 +1371,8 @@ static void update_rrd_data(const char *key, gconstpointer data, size_t len) {
             use_pve2_file = 1;
 
             g_free(filename);
-            filename = g_strdup(filename_pve2);
+            filename = filename_pve2;
+            filename_pve2 = NULL;
         } else {
             // neither file exists, check for directories to decide and create file
 
@@ -1383,7 +1384,8 @@ static void update_rrd_data(const char *key, gconstpointer data, size_t len) {
                 use_pve2_file = 1;
 
                 g_free(filename);
-                filename = g_strdup(filename_pve2);
+                filename = filename_pve2;
+                filename_pve2 = NULL;
 
                 char *dir = g_path_get_dirname(filename);
                 checked_mkdir(dir, 0755);
@@ -1443,7 +1445,8 @@ static void update_rrd_data(const char *key, gconstpointer data, size_t len) {
             // old file exists, use it
             use_pve2_file = 1;
             g_free(filename);
-            filename = g_strdup(filename_pve2);
+            filename = filename_pve2;
+            filename_pve2 = NULL;
         } else {
             // neither file exists, check for directories to decide and create file
 
@@ -1455,7 +1458,8 @@ static void update_rrd_data(const char *key, gconstpointer data, size_t len) {
                 use_pve2_file = 1;
 
                 g_free(filename);
-                filename = g_strdup(filename_pve2);
+                filename = filename_pve2;
+                filename_pve2 = NULL;
 
                 int argcount = sizeof(rrd_def_vm) / sizeof(void *) - 1;
                 create_rrd_file(filename, argcount, rrd_def_vm);
@@ -1509,7 +1513,8 @@ static void update_rrd_data(const char *key, gconstpointer data, size_t len) {
         } else if (g_file_test(filename_pve2, G_FILE_TEST_EXISTS)) {
             // old file exists, use it
             g_free(filename);
-            filename = g_strdup(filename_pve2);
+            filename = filename_pve2;
+            filename_pve2 = NULL;
         } else {
             // neither file exists, check for directories to decide and create file
 
@@ -1522,7 +1527,8 @@ static void update_rrd_data(const char *key, gconstpointer data, size_t len) {
                 create_rrd_file(filename, argcount, rrd_def_storage_pve9_0);
             } else if (g_file_test(RRDDIR "/pve2-storage", G_FILE_TEST_IS_DIR)) {
                 g_free(filename);
-                filename = g_strdup(filename_pve2);
+                filename = filename_pve2;
+                filename_pve2 = NULL;
 
                 char *dir = g_path_get_dirname(filename);
                 checked_mkdir(dir, 0755);
-- 
2.47.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH cluster 1/5] pmxcfs: status: add one more missing g_free

[Maximiliano Sandoval]

Lukas Wagner <l.wagner@proxmox.com> writes:

> Otherwise we leak the memory that was already allocated for `filename`.
>
> Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
> ---
>  src/pmxcfs/status.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/src/pmxcfs/status.c b/src/pmxcfs/status.c
> index a00e793..886d0bd 100644
> --- a/src/pmxcfs/status.c
> +++ b/src/pmxcfs/status.c
> @@ -1369,6 +1369,8 @@ static void update_rrd_data(const char *key, gconstpointer data, size_t len) {
>          } else if (g_file_test(filename_pve2, G_FILE_TEST_EXISTS)) {
>              // old file exists, use it
>              use_pve2_file = 1;
> +
> +            g_free(filename);
>              filename = g_strdup_printf("%s", filename_pve2);
>          } else {
>              // neither file exists, check for directories to decide and create file

I tested this series against a 9.0 (beta) cluster and I didn't see
anything off while writing data to the pmxcfs.

I also did a light review of the code and it looks good.

Tested-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
Reviewed-by: Maximiliano Sandoval <m.sandoval@proxmox.com>

-- 
Maximiliano


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH cluster 1/5] pmxcfs: status: add one more missing g_free

[Thomas Lamprecht]

On Fri, 01 Aug 2025 11:54:27 +0200, Lukas Wagner wrote:
> Otherwise we leak the memory that was already allocated for `filename`.
> 
> 

Applied, thanks!

But can you please add a cover-letter for any actual patch series (i.e., not
just a single patch), as that provides some value even if it's empty, as it
makes it very clear if trailers like T-b or R-b are intended for the full
series or just a single patch out of it. b4 also uses the same logic to
determine for which commits to apply the R-b and T-b too, so here I had to
manually add Maximiliano's R-b and T-b to patch 2 to 5, as from b4's POV they
where only supplied for patch 1/5.

A diffstat over all patches is a side benefit, a short sentence as overview
might be nice too, but for things like these here it can be even fine to not
have that.

Not a big thing, but would make my (and others using b4) life a bit easier and
better ensure that tests and reviews get encoded into git, providing some
recognition of that important work.

[1/5] pmxcfs: status: add one more missing g_free
      commit: 8239d5c3657a0e3fe751689bce920b7c2c87e531
[2/5] pmxcfs: status: avoid unnecessary string allocations
      commit: 426cb07ed853db202aaa219b06be80e7bbb4c74c
[3/5] pmxcfs: status: call g_strdup instead of g_strdup_printf when no formatting is needed
      commit: c66917260e8bdf2a76878baee0ba91c6f4469207
[4/5] pmxcfs: status: remove string literal params for g_strdup_printf
      commit: 0795244f98b6e5e34491b9a1b3967d5bf935afd8
[5/5] pmxcfs: status: avoid g_strdup when not needed
      commit: 054995ba3d43e2bfe1e8fcd3d63549e3579e6ef9


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH cluster 1/5] pmxcfs: status: add one more missing g_free

[Lukas Wagner]

On Fri Aug 1, 2025 at 11:54 AM CEST, Lukas Wagner wrote:
> Otherwise we leak the memory that was already allocated for `filename`.
>
> Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
> ---
>  src/pmxcfs/status.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/src/pmxcfs/status.c b/src/pmxcfs/status.c
> index a00e793..886d0bd 100644
> --- a/src/pmxcfs/status.c
> +++ b/src/pmxcfs/status.c
> @@ -1369,6 +1369,8 @@ static void update_rrd_data(const char *key, gconstpointer data, size_t len) {
>          } else if (g_file_test(filename_pve2, G_FILE_TEST_EXISTS)) {
>              // old file exists, use it
>              use_pve2_file = 1;
> +
> +            g_free(filename);
>              filename = g_strdup_printf("%s", filename_pve2);
>          } else {
>              // neither file exists, check for directories to decide and create file


FWIW, did a quick rebuild of pve-cluster with -fsanitize=address set in the
CFLAGS/LDFLAGS. While I might not have hit all possible code paths
during my test run, at least the "main" one with the new RRD format came back
without any reported leaks!


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH cluster 1/5] pmxcfs: status: add one more missing g_free

[Lukas Wagner]

On Fri Aug 1, 2025 at 2:52 PM CEST, Thomas Lamprecht wrote:
> Applied, thanks!
>
> But can you please add a cover-letter for any actual patch series (i.e., not
> just a single patch), as that provides some value even if it's empty, as it
> makes it very clear if trailers like T-b or R-b are intended for the full
> series or just a single patch out of it. b4 also uses the same logic to
> determine for which commits to apply the R-b and T-b too, so here I had to
> manually add Maximiliano's R-b and T-b to patch 2 to 5, as from b4's POV they
> where only supplied for patch 1/5.
>
> A diffstat over all patches is a side benefit, a short sentence as overview
> might be nice too, but for things like these here it can be even fine to not
> have that.
>
> Not a big thing, but would make my (and others using b4) life a bit easier and
> better ensure that tests and reviews get encoded into git, providing some
> recognition of that important work.
>

Okay, will keep this in mind for the future. Thanks for the hint and
explanation!


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel