[libvirt PATCHv3 for 9.4.0] conf: node_device: use separate variables for parsing integers

Ján Tomko posted 1 patch 10 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/378453b18e2f9338537e72cd1e6ec8ee33fda2a0.1685558912.git.jtomko@redhat.com
src/conf/node_device_conf.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
[libvirt PATCHv3 for 9.4.0] conf: node_device: use separate variables for parsing integers
Posted by Ján Tomko 10 months ago
In virNodeDeviceGetSCSIHostCaps, there is a pattern of reusing
a tmp value and stealing the pointer.

But in two case it is not stolen. Use separate variables for them
to avoid mixing autofree with manual free() calls.

This fixes the memory leak of the "max_npiv_vports" string.
(The other gets freed anyway because it was the last use of "tmp"
 in the function")

Fixes: 8a0cb5f73ade3900546718eabe70cb064c6bd22c
Signed-off-by: Ján Tomko <jtomko@redhat.com>
---
v3: fix "declaration after statement" warning
 src/conf/node_device_conf.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
index fcee9c027c..172223225f 100644
--- a/src/conf/node_device_conf.c
+++ b/src/conf/node_device_conf.c
@@ -2857,29 +2857,32 @@ virNodeDeviceGetSCSIHostCaps(virNodeDevCapSCSIHost *scsi_host)
     }
 
     if (virVHBAIsVportCapable(NULL, scsi_host->host)) {
+        g_autofree char *max_vports = NULL;
+        g_autofree char *vports = NULL;
+
         scsi_host->flags |= VIR_NODE_DEV_CAP_FLAG_HBA_VPORT_OPS;
 
-        if (!(tmp = virVHBAGetConfig(NULL, scsi_host->host,
+        if (!(max_vports = virVHBAGetConfig(NULL, scsi_host->host,
                                      "max_npiv_vports"))) {
             VIR_WARN("Failed to read max_npiv_vports for host%d",
                      scsi_host->host);
             goto cleanup;
         }
 
-        if (virStrToLong_i(tmp, NULL, 10, &scsi_host->max_vports) < 0) {
-            VIR_WARN("Failed to parse value of max_npiv_vports '%s'", tmp);
+        if (virStrToLong_i(max_vports, NULL, 10, &scsi_host->max_vports) < 0) {
+            VIR_WARN("Failed to parse value of max_npiv_vports '%s'", max_vports);
             goto cleanup;
         }
 
-        if (!(tmp = virVHBAGetConfig(NULL, scsi_host->host,
+        if (!(vports = virVHBAGetConfig(NULL, scsi_host->host,
                                       "npiv_vports_inuse"))) {
             VIR_WARN("Failed to read npiv_vports_inuse for host%d",
                      scsi_host->host);
             goto cleanup;
         }
 
-        if (virStrToLong_i(tmp, NULL, 10, &scsi_host->vports) < 0) {
-            VIR_WARN("Failed to parse value of npiv_vports_inuse '%s'", tmp);
+        if (virStrToLong_i(vports, NULL, 10, &scsi_host->vports) < 0) {
+            VIR_WARN("Failed to parse value of npiv_vports_inuse '%s'", vports);
             goto cleanup;
         }
     }
-- 
2.40.1

Re: [libvirt PATCHv3 for 9.4.0] conf: node_device: use separate variables for parsing integers
Posted by Michal Prívozník 10 months ago
On 5/31/23 20:50, Ján Tomko wrote:
> In virNodeDeviceGetSCSIHostCaps, there is a pattern of reusing
> a tmp value and stealing the pointer.
> 
> But in two case it is not stolen. Use separate variables for them
> to avoid mixing autofree with manual free() calls.
> 
> This fixes the memory leak of the "max_npiv_vports" string.
> (The other gets freed anyway because it was the last use of "tmp"
>  in the function")
> 
> Fixes: 8a0cb5f73ade3900546718eabe70cb064c6bd22c
> Signed-off-by: Ján Tomko <jtomko@redhat.com>
> ---
> v3: fix "declaration after statement" warning
>  src/conf/node_device_conf.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
> index fcee9c027c..172223225f 100644
> --- a/src/conf/node_device_conf.c
> +++ b/src/conf/node_device_conf.c
> @@ -2857,29 +2857,32 @@ virNodeDeviceGetSCSIHostCaps(virNodeDevCapSCSIHost *scsi_host)
>      }
>  
>      if (virVHBAIsVportCapable(NULL, scsi_host->host)) {
> +        g_autofree char *max_vports = NULL;
> +        g_autofree char *vports = NULL;
> +
>          scsi_host->flags |= VIR_NODE_DEV_CAP_FLAG_HBA_VPORT_OPS;
>  
> -        if (!(tmp = virVHBAGetConfig(NULL, scsi_host->host,
> +        if (!(max_vports = virVHBAGetConfig(NULL, scsi_host->host,
>                                       "max_npiv_vports"))) {

Any reason to not align this second argument?

>              VIR_WARN("Failed to read max_npiv_vports for host%d",
>                       scsi_host->host);
>              goto cleanup;
>          }
>  
> -        if (virStrToLong_i(tmp, NULL, 10, &scsi_host->max_vports) < 0) {
> -            VIR_WARN("Failed to parse value of max_npiv_vports '%s'", tmp);
> +        if (virStrToLong_i(max_vports, NULL, 10, &scsi_host->max_vports) < 0) {
> +            VIR_WARN("Failed to parse value of max_npiv_vports '%s'", max_vports);
>              goto cleanup;
>          }
>  
> -        if (!(tmp = virVHBAGetConfig(NULL, scsi_host->host,
> +        if (!(vports = virVHBAGetConfig(NULL, scsi_host->host,
>                                        "npiv_vports_inuse"))) {

Same question here.

>              VIR_WARN("Failed to read npiv_vports_inuse for host%d",
>                       scsi_host->host);
>              goto cleanup;
>          }
>  
> -        if (virStrToLong_i(tmp, NULL, 10, &scsi_host->vports) < 0) {
> -            VIR_WARN("Failed to parse value of npiv_vports_inuse '%s'", tmp);
> +        if (virStrToLong_i(vports, NULL, 10, &scsi_host->vports) < 0) {
> +            VIR_WARN("Failed to parse value of npiv_vports_inuse '%s'", vports);
>              goto cleanup;
>          }
>      }

Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

Michal