[PATCH v2 0/4] qemu-ga: Fix some potential issues find by coverity

[PATCH v2 0/4] qemu-ga: Fix some potential issues find by coverity

[Dehan Meng]

v2:
Split v1 up to separate commits for each logically independent change

Dehan Meng (4):
  sscanf return values are checked to ensure correct parsing.
  Proper initialization of n to 0 for getline to function correctly.
  Avoiding freeing line prematurely. It's now only freed at the end of
    the function.
  For correcting code style: Variable declarations moved to the
    beginning of blocks Followed the coding style of using snake_case
    for variable names. And merged redundant route and networkroute
    variables.

Signed-off-by: Dehan Meng <demeng@redhat.com>

 qga/commands-linux.c | 128 ++++++++++++++++++++-----------------------
 1 file changed, 59 insertions(+), 69 deletions(-)

-- 
2.40.1

[PATCH v2 1/4] sscanf return values are checked to ensure correct parsing.

[Dehan Meng]

Signed-off-by: Dehan Meng <demeng@redhat.com>
---
 qga/commands-linux.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/qga/commands-linux.c b/qga/commands-linux.c
index 51d5e3d927..2c2b5f4ff2 100644
--- a/qga/commands-linux.c
+++ b/qga/commands-linux.c
@@ -2103,7 +2103,9 @@ static char *hexToIPAddress(const void *hexValue, int is_ipv6)
         int i;
 
         for (i = 0; i < 16; i++) {
-            sscanf(&hexStr[i * 2], "%02hhx", &in6.s6_addr[i]);
+            if (sscanf(&hex_str[i * 2], "%02hhx", &in6.s6_addr[i]) != 1) {
+                return NULL;
+            }
         }
         inet_ntop(AF_INET6, &in6, addr, INET6_ADDRSTRLEN);
 
-- 
2.40.1

[PATCH v2 2/4] Proper initialization of n to 0 for getline to function correctly.

[Dehan Meng]

Signed-off-by: Dehan Meng <demeng@redhat.com>
---
 qga/commands-linux.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qga/commands-linux.c b/qga/commands-linux.c
index 2c2b5f4ff2..b905f33a57 100644
--- a/qga/commands-linux.c
+++ b/qga/commands-linux.c
@@ -2126,7 +2126,7 @@ GuestNetworkRouteList *qmp_guest_network_get_route(Error **errp)
     GuestNetworkRouteList *head = NULL, **tail = &head;
     const char *routeFiles[] = {"/proc/net/route", "/proc/net/ipv6_route"};
     FILE *fp;
-    size_t n;
+    size_t n = 0;
     char *line = NULL;
     int firstLine;
     int is_ipv6;
-- 
2.40.1

[PATCH v2 3/4] Avoiding freeing line prematurely. It's now only freed at the end of the function.

[Dehan Meng]

Signed-off-by: Dehan Meng <demeng@redhat.com>
---
 qga/commands-linux.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/qga/commands-linux.c b/qga/commands-linux.c
index b905f33a57..4f0e38be81 100644
--- a/qga/commands-linux.c
+++ b/qga/commands-linux.c
@@ -2137,8 +2137,7 @@ GuestNetworkRouteList *qmp_guest_network_get_route(Error **errp)
         is_ipv6 = (i == 1);
         fp = fopen(routeFiles[i], "r");
         if (fp == NULL) {
-            error_setg_errno(errp, errno, "open(\"%s\")", routeFiles[i]);
-            free(line);
+            error_setg_errno(errp, errno, "open(\"%s\")", route_files[i]);
             continue;
         }
 
@@ -2218,9 +2217,8 @@ GuestNetworkRouteList *qmp_guest_network_get_route(Error **errp)
             QAPI_LIST_APPEND(tail, route);
         }
 
-        free(line);
         fclose(fp);
     }
-
+    free(line);
     return head;
 }
-- 
2.40.1

[PATCH v2 4/4] For correcting code style: Variable declarations moved to the beginning of blocks Followed the coding style of using snake_case for variable names. And merged redundant route and networkroute variables.

[Dehan Meng]

Signed-off-by: Dehan Meng <demeng@redhat.com>
---
 qga/commands-linux.c | 116 ++++++++++++++++++++-----------------------
 1 file changed, 53 insertions(+), 63 deletions(-)

diff --git a/qga/commands-linux.c b/qga/commands-linux.c
index 4f0e38be81..c6cca630ef 100644
--- a/qga/commands-linux.c
+++ b/qga/commands-linux.c
@@ -2094,12 +2094,12 @@ GuestCpuStatsList *qmp_guest_get_cpustats(Error **errp)
     return head;
 }
 
-static char *hexToIPAddress(const void *hexValue, int is_ipv6)
+static char *hex_to_ip_address(const void *hex_value, int is_ipv6)
 {
     if (is_ipv6) {
         char addr[INET6_ADDRSTRLEN];
         struct in6_addr in6;
-        const char *hexStr = (const char *)hexValue;
+        const char *hex_str = (const char *)hex_value;
         int i;
 
         for (i = 0; i < 16; i++) {
@@ -2111,11 +2111,11 @@ static char *hexToIPAddress(const void *hexValue, int is_ipv6)
 
         return g_strdup(addr);
     } else {
-        unsigned int hexInt = *(unsigned int *)hexValue;
-        unsigned int byte1 = (hexInt >> 24) & 0xFF;
-        unsigned int byte2 = (hexInt >> 16) & 0xFF;
-        unsigned int byte3 = (hexInt >> 8) & 0xFF;
-        unsigned int byte4 = hexInt & 0xFF;
+        unsigned int hex_int = *(unsigned int *)hex_value;
+        unsigned int byte1 = (hex_int >> 24) & 0xFF;
+        unsigned int byte2 = (hex_int >> 16) & 0xFF;
+        unsigned int byte3 = (hex_int >> 8) & 0xFF;
+        unsigned int byte4 = hex_int & 0xFF;
 
         return g_strdup_printf("%u.%u.%u.%u", byte4, byte3, byte2, byte1);
     }
@@ -2131,6 +2131,7 @@ GuestNetworkRouteList *qmp_guest_network_get_route(Error **errp)
     int firstLine;
     int is_ipv6;
     int i;
+    char iface[IFNAMSIZ];
 
     for (i = 0; i < 2; i++) {
         firstLine = 1;
@@ -2146,72 +2147,61 @@ GuestNetworkRouteList *qmp_guest_network_get_route(Error **errp)
                 firstLine = 0;
                 continue;
             }
-            GuestNetworkRoute *route = NULL;
-            GuestNetworkRoute *networkroute;
-            char Iface[IFNAMSIZ];
-            if (is_ipv6) {
-                char Destination[33], Source[33], NextHop[33];
-                int DesPrefixlen, SrcPrefixlen, Metric, RefCnt, Use, Flags;
 
-                /* Parse the line and extract the values */
+            GuestNetworkRoute *route = g_new0(GuestNetworkRoute, 1);
+
+            if (is_ipv6) {
+                char destination[33], source[33], next_hop[33];
+                int des_prefixlen, src_prefixlen, metric, refcnt, use, flags;
                 if (sscanf(line, "%32s %x %32s %x %32s %x %x %x %x %s",
-                           Destination, &DesPrefixlen, Source,
-                           &SrcPrefixlen, NextHop, &Metric, &RefCnt,
-                           &Use, &Flags, Iface) != 10) {
+                           destination, &des_prefixlen, source,
+                           &src_prefixlen, next_hop, &metric, &refcnt,
+                           &use, &flags, iface) != 10) {
                     continue;
                 }
 
-                route = g_new0(GuestNetworkRoute, 1);
-                networkroute = route;
-                networkroute->iface = g_strdup(Iface);
-                networkroute->destination = hexToIPAddress(Destination, 1);
-                networkroute->metric = Metric;
-                networkroute->source = hexToIPAddress(Source, 1);
-                networkroute->desprefixlen = g_strdup_printf(
-                    "%d", DesPrefixlen
-                );
-                networkroute->srcprefixlen = g_strdup_printf(
-                    "%d", SrcPrefixlen
-                );
-                networkroute->nexthop = hexToIPAddress(NextHop, 1);
-                networkroute->has_flags = true;
-                networkroute->flags = Flags;
-                networkroute->has_refcnt = true;
-                networkroute->refcnt = RefCnt;
-                networkroute->has_use = true;
-                networkroute->use = Use;
-                networkroute->version = 6;
-            } else {
-                unsigned int Destination, Gateway, Mask, Flags;
-                int RefCnt, Use, Metric, MTU, Window, IRTT;
+                route->iface = g_strdup(iface);
+                route->destination = hex_to_ip_address(destination, 1);
+                route->source = hex_to_ip_address(source, 1);
+                route->nexthop = hex_to_ip_address(next_hop, 1);
+                route->desprefixlen = g_strdup_printf("%d", des_prefixlen);
+                route->srcprefixlen = g_strdup_printf("%d", src_prefixlen);
+                route->metric = metric;
+                route->has_flags = true;
+                route->flags = flags;
+                route->has_refcnt = true;
+                route->refcnt = refcnt;
+                route->has_use = true;
+                route->use = use;
+                route->version = 6;
 
-                /* Parse the line and extract the values */
+            } else {
+                unsigned int destination, gateway, mask, flags;
+                int refcnt, use, metric, mtu, window, irtt;
                 if (sscanf(line, "%s %X %X %x %d %d %d %X %d %d %d",
-                           Iface, &Destination, &Gateway, &Flags, &RefCnt,
-                           &Use, &Metric, &Mask, &MTU, &Window, &IRTT) != 11) {
+                           iface, &destination, &gateway, &flags, &refcnt,
+                           &use, &metric, &mask, &mtu, &window, &irtt) != 11) {
                     continue;
                 }
 
-                route = g_new0(GuestNetworkRoute, 1);
-                networkroute = route;
-                networkroute->iface = g_strdup(Iface);
-                networkroute->destination = hexToIPAddress(&Destination, 0);
-                networkroute->gateway = hexToIPAddress(&Gateway, 0);
-                networkroute->mask = hexToIPAddress(&Mask, 0);
-                networkroute->metric = Metric;
-                networkroute->has_flags = true;
-                networkroute->flags = Flags;
-                networkroute->has_refcnt = true;
-                networkroute->refcnt = RefCnt;
-                networkroute->has_use = true;
-                networkroute->use = Use;
-                networkroute->has_mtu = true;
-                networkroute->mtu = MTU;
-                networkroute->has_window = true;
-                networkroute->window = Window;
-                networkroute->has_irtt = true;
-                networkroute->irtt = IRTT;
-                networkroute->version = 4;
+                route->iface = g_strdup(iface);
+                route->destination = hex_to_ip_address(&destination, 0);
+                route->gateway = hex_to_ip_address(&gateway, 0);
+                route->mask = hex_to_ip_address(&mask, 0);
+                route->metric = metric;
+                route->has_flags = true;
+                route->flags = flags;
+                route->has_refcnt = true;
+                route->refcnt = refcnt;
+                route->has_use = true;
+                route->use = use;
+                route->has_mtu = true;
+                route->mtu = mtu;
+                route->has_window = true;
+                route->window = window;
+                route->has_irtt = true;
+                route->irtt = irtt;
+                route->version = 4;
             }
 
             QAPI_LIST_APPEND(tail, route);
-- 
2.40.1

Re: [PATCH v2 0/4] qemu-ga: Fix some potential issues find by coverity

[Konstantin Kostiuk]

[-- Attachment #1: Type: text/plain, Size: 1173 bytes --]

On Fri, Oct 11, 2024 at 6:19 AM Dehan Meng <demeng@redhat.com> wrote:

> v2:
> Split v1 up to separate commits for each logically independent change
>
> Dehan Meng (4):
>   sscanf return values are checked to ensure correct parsing.
>   Proper initialization of n to 0 for getline to function correctly.
>   Avoiding freeing line prematurely. It's now only freed at the end of
>     the function.
>   For correcting code style: Variable declarations moved to the
>     beginning of blocks Followed the coding style of using snake_case
>     for variable names. And merged redundant route and networkroute
>     variables.
>
>
First of all, don't use so long commit title. You can add more info in the
commit
message if needed. Commits 3 and 4 have very long titles.

Do not set . at the end of the commit title.

As in git, we do not see cover-messages, please add `qemu-ga:` suffix for
each commit

Best Regards,
Konstantin Kostiuk.


> Signed-off-by: Dehan Meng <demeng@redhat.com>
>
>  qga/commands-linux.c | 128 ++++++++++++++++++++-----------------------
>  1 file changed, 59 insertions(+), 69 deletions(-)
>
> --
> 2.40.1
>
>

[-- Attachment #2: Type: text/html, Size: 1937 bytes --]

Re: [PATCH v2 1/4] sscanf return values are checked to ensure correct parsing.

[Daniel P. Berrangé]

On Fri, Oct 11, 2024 at 11:19:34AM +0800, Dehan Meng wrote:
> Signed-off-by: Dehan Meng <demeng@redhat.com>
> ---
>  qga/commands-linux.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/qga/commands-linux.c b/qga/commands-linux.c
> index 51d5e3d927..2c2b5f4ff2 100644
> --- a/qga/commands-linux.c
> +++ b/qga/commands-linux.c
> @@ -2103,7 +2103,9 @@ static char *hexToIPAddress(const void *hexValue, int is_ipv6)
>          int i;
>  
>          for (i = 0; i < 16; i++) {
> -            sscanf(&hexStr[i * 2], "%02hhx", &in6.s6_addr[i]);
> +            if (sscanf(&hex_str[i * 2], "%02hhx", &in6.s6_addr[i]) != 1) {
> +                return NULL;
> +            }
>          }
>          inet_ntop(AF_INET6, &in6, addr, INET6_ADDRSTRLEN);

None of the callers of this function are expecting it to return NULL.

eg this code:

                networkroute->destination = hexToIPAddress(Destination, 1);
                networkroute->metric = Metric;
                networkroute->source = hexToIPAddress(Source, 1);
                networkroute->desprefixlen = g_strdup_printf(
                    "%d", DesPrefixlen
                );
                networkroute->srcprefixlen = g_strdup_printf(
                    "%d", SrcPrefixlen
                );
                networkroute->nexthop = hexToIPAddress(NextHop, 1);

The QAPI schema allows 'source' and 'nexthop' to be optional so those
two are fnie.

The 'destination' field is marked as mandatory thoug, so must not
be NULL.

IOW, in the calls we need to check for NULL, and skip adding the
entire route object if 'destniation' is NULL.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v2 2/4] Proper initialization of n to 0 for getline to function correctly.

[Daniel P. Berrangé]

On Fri, Oct 11, 2024 at 11:19:35AM +0800, Dehan Meng wrote:
> Signed-off-by: Dehan Meng <demeng@redhat.com>
> ---
>  qga/commands-linux.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

> 
> diff --git a/qga/commands-linux.c b/qga/commands-linux.c
> index 2c2b5f4ff2..b905f33a57 100644
> --- a/qga/commands-linux.c
> +++ b/qga/commands-linux.c
> @@ -2126,7 +2126,7 @@ GuestNetworkRouteList *qmp_guest_network_get_route(Error **errp)
>      GuestNetworkRouteList *head = NULL, **tail = &head;
>      const char *routeFiles[] = {"/proc/net/route", "/proc/net/ipv6_route"};
>      FILE *fp;
> -    size_t n;
> +    size_t n = 0;
>      char *line = NULL;
>      int firstLine;
>      int is_ipv6;
> -- 
> 2.40.1
> 
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v2 3/4] Avoiding freeing line prematurely. It's now only freed at the end of the function.

[Daniel P. Berrangé]

The subject line is too long - put the second sentance on a line of
its own in the commit message, separated from the 1st line by a blank
line.

On Fri, Oct 11, 2024 at 11:19:36AM +0800, Dehan Meng wrote:
> Signed-off-by: Dehan Meng <demeng@redhat.com>
> ---
>  qga/commands-linux.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)

> 
> diff --git a/qga/commands-linux.c b/qga/commands-linux.c
> index b905f33a57..4f0e38be81 100644
> --- a/qga/commands-linux.c
> +++ b/qga/commands-linux.c
> @@ -2137,8 +2137,7 @@ GuestNetworkRouteList *qmp_guest_network_get_route(Error **errp)
>          is_ipv6 = (i == 1);
>          fp = fopen(routeFiles[i], "r");
>          if (fp == NULL) {
> -            error_setg_errno(errp, errno, "open(\"%s\")", routeFiles[i]);
> -            free(line);
> +            error_setg_errno(errp, errno, "open(\"%s\")", route_files[i]);
>              continue;
>          }
>  
> @@ -2218,9 +2217,8 @@ GuestNetworkRouteList *qmp_guest_network_get_route(Error **errp)
>              QAPI_LIST_APPEND(tail, route);
>          }
>  
> -        free(line);
>          fclose(fp);
>      }
> -
> +    free(line);
>      return head;
>  }
> -- 
> 2.40.1
> 
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v2 4/4] For correcting code style: Variable declarations moved to the beginning of blocks Followed the coding style of using snake_case for variable names. And merged redundant route and networkroute variables.

[Daniel P. Berrangé]

Same comment about $SUBJECT being too long. You need a blank line after
the 1st line in the commit message.

On Fri, Oct 11, 2024 at 11:19:37AM +0800, Dehan Meng wrote:
> Signed-off-by: Dehan Meng <demeng@redhat.com>
> ---
>  qga/commands-linux.c | 116 ++++++++++++++++++++-----------------------
>  1 file changed, 53 insertions(+), 63 deletions(-)
> 
> diff --git a/qga/commands-linux.c b/qga/commands-linux.c
> index 4f0e38be81..c6cca630ef 100644
> --- a/qga/commands-linux.c
> +++ b/qga/commands-linux.c
> @@ -2094,12 +2094,12 @@ GuestCpuStatsList *qmp_guest_get_cpustats(Error **errp)
>      return head;
>  }
>  
> -static char *hexToIPAddress(const void *hexValue, int is_ipv6)
> +static char *hex_to_ip_address(const void *hex_value, int is_ipv6)
>  {
>      if (is_ipv6) {
>          char addr[INET6_ADDRSTRLEN];
>          struct in6_addr in6;
> -        const char *hexStr = (const char *)hexValue;
> +        const char *hex_str = (const char *)hex_value;
>          int i;
>  
>          for (i = 0; i < 16; i++) {
> @@ -2111,11 +2111,11 @@ static char *hexToIPAddress(const void *hexValue, int is_ipv6)
>  
>          return g_strdup(addr);
>      } else {
> -        unsigned int hexInt = *(unsigned int *)hexValue;
> -        unsigned int byte1 = (hexInt >> 24) & 0xFF;
> -        unsigned int byte2 = (hexInt >> 16) & 0xFF;
> -        unsigned int byte3 = (hexInt >> 8) & 0xFF;
> -        unsigned int byte4 = hexInt & 0xFF;
> +        unsigned int hex_int = *(unsigned int *)hex_value;
> +        unsigned int byte1 = (hex_int >> 24) & 0xFF;
> +        unsigned int byte2 = (hex_int >> 16) & 0xFF;
> +        unsigned int byte3 = (hex_int >> 8) & 0xFF;
> +        unsigned int byte4 = hex_int & 0xFF;
>  
>          return g_strdup_printf("%u.%u.%u.%u", byte4, byte3, byte2, byte1);
>      }
> @@ -2131,6 +2131,7 @@ GuestNetworkRouteList *qmp_guest_network_get_route(Error **errp)
>      int firstLine;
>      int is_ipv6;
>      int i;
> +    char iface[IFNAMSIZ];
>  
>      for (i = 0; i < 2; i++) {
>          firstLine = 1;
> @@ -2146,72 +2147,61 @@ GuestNetworkRouteList *qmp_guest_network_get_route(Error **errp)
>                  firstLine = 0;
>                  continue;
>              }
> -            GuestNetworkRoute *route = NULL;
> -            GuestNetworkRoute *networkroute;
> -            char Iface[IFNAMSIZ];
> -            if (is_ipv6) {
> -                char Destination[33], Source[33], NextHop[33];
> -                int DesPrefixlen, SrcPrefixlen, Metric, RefCnt, Use, Flags;
>  
> -                /* Parse the line and extract the values */
> +            GuestNetworkRoute *route = g_new0(GuestNetworkRoute, 1);
> +
> +            if (is_ipv6) {
> +                char destination[33], source[33], next_hop[33];
> +                int des_prefixlen, src_prefixlen, metric, refcnt, use, flags;
>                  if (sscanf(line, "%32s %x %32s %x %32s %x %x %x %x %s",
> -                           Destination, &DesPrefixlen, Source,
> -                           &SrcPrefixlen, NextHop, &Metric, &RefCnt,
> -                           &Use, &Flags, Iface) != 10) {
> +                           destination, &des_prefixlen, source,
> +                           &src_prefixlen, next_hop, &metric, &refcnt,
> +                           &use, &flags, iface) != 10) {
>                      continue;
>                  }
>  
> -                route = g_new0(GuestNetworkRoute, 1);

By moving this up to the time of declaration of 'route', you've introduced
a memory leak when the above 'sscanf' line triggers the 'continue' branch.

> -                networkroute = route;
> -                networkroute->iface = g_strdup(Iface);
> -                networkroute->destination = hexToIPAddress(Destination, 1);
> -                networkroute->metric = Metric;
> -                networkroute->source = hexToIPAddress(Source, 1);
> -                networkroute->desprefixlen = g_strdup_printf(
> -                    "%d", DesPrefixlen
> -                );
> -                networkroute->srcprefixlen = g_strdup_printf(
> -                    "%d", SrcPrefixlen
> -                );
> -                networkroute->nexthop = hexToIPAddress(NextHop, 1);
> -                networkroute->has_flags = true;
> -                networkroute->flags = Flags;
> -                networkroute->has_refcnt = true;
> -                networkroute->refcnt = RefCnt;
> -                networkroute->has_use = true;
> -                networkroute->use = Use;
> -                networkroute->version = 6;
> -            } else {
> -                unsigned int Destination, Gateway, Mask, Flags;
> -                int RefCnt, Use, Metric, MTU, Window, IRTT;
> +                route->iface = g_strdup(iface);
> +                route->destination = hex_to_ip_address(destination, 1);
> +                route->source = hex_to_ip_address(source, 1);
> +                route->nexthop = hex_to_ip_address(next_hop, 1);
> +                route->desprefixlen = g_strdup_printf("%d", des_prefixlen);
> +                route->srcprefixlen = g_strdup_printf("%d", src_prefixlen);
> +                route->metric = metric;
> +                route->has_flags = true;
> +                route->flags = flags;
> +                route->has_refcnt = true;
> +                route->refcnt = refcnt;
> +                route->has_use = true;
> +                route->use = use;
> +                route->version = 6;
>  
> -                /* Parse the line and extract the values */
> +            } else {
> +                unsigned int destination, gateway, mask, flags;
> +                int refcnt, use, metric, mtu, window, irtt;
>                  if (sscanf(line, "%s %X %X %x %d %d %d %X %d %d %d",
> -                           Iface, &Destination, &Gateway, &Flags, &RefCnt,
> -                           &Use, &Metric, &Mask, &MTU, &Window, &IRTT) != 11) {
> +                           iface, &destination, &gateway, &flags, &refcnt,
> +                           &use, &metric, &mask, &mtu, &window, &irtt) != 11) {
>                      continue;
>                  }
>  
> -                route = g_new0(GuestNetworkRoute, 1);

Again, introduced a memory leak for the same reason.

> -                networkroute = route;
> -                networkroute->iface = g_strdup(Iface);
> -                networkroute->destination = hexToIPAddress(&Destination, 0);
> -                networkroute->gateway = hexToIPAddress(&Gateway, 0);
> -                networkroute->mask = hexToIPAddress(&Mask, 0);
> -                networkroute->metric = Metric;
> -                networkroute->has_flags = true;
> -                networkroute->flags = Flags;
> -                networkroute->has_refcnt = true;
> -                networkroute->refcnt = RefCnt;
> -                networkroute->has_use = true;
> -                networkroute->use = Use;
> -                networkroute->has_mtu = true;
> -                networkroute->mtu = MTU;
> -                networkroute->has_window = true;
> -                networkroute->window = Window;
> -                networkroute->has_irtt = true;
> -                networkroute->irtt = IRTT;
> -                networkroute->version = 4;
> +                route->iface = g_strdup(iface);
> +                route->destination = hex_to_ip_address(&destination, 0);
> +                route->gateway = hex_to_ip_address(&gateway, 0);
> +                route->mask = hex_to_ip_address(&mask, 0);
> +                route->metric = metric;
> +                route->has_flags = true;
> +                route->flags = flags;
> +                route->has_refcnt = true;
> +                route->refcnt = refcnt;
> +                route->has_use = true;
> +                route->use = use;
> +                route->has_mtu = true;
> +                route->mtu = mtu;
> +                route->has_window = true;
> +                route->window = window;
> +                route->has_irtt = true;
> +                route->irtt = irtt;
> +                route->version = 4;
>              }
>  
>              QAPI_LIST_APPEND(tail, route);
> -- 
> 2.40.1
> 
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

[PATCH v2 4/4] For correcting code style: Variable declarations moved to the beginning of blocks Followed the coding style of using snake_case for variable names. And merged redundant route and networkroute variables.

[Dehan Meng]

Signed-off-by: Dehan Meng <demeng@redhat.com>
---
 qga/commands-linux.c | 116 ++++++++++++++++++++-----------------------
 1 file changed, 53 insertions(+), 63 deletions(-)

diff --git a/qga/commands-linux.c b/qga/commands-linux.c
index 4f0e38be81..c6cca630ef 100644
--- a/qga/commands-linux.c
+++ b/qga/commands-linux.c
@@ -2094,12 +2094,12 @@ GuestCpuStatsList *qmp_guest_get_cpustats(Error **errp)
     return head;
 }
 
-static char *hexToIPAddress(const void *hexValue, int is_ipv6)
+static char *hex_to_ip_address(const void *hex_value, int is_ipv6)
 {
     if (is_ipv6) {
         char addr[INET6_ADDRSTRLEN];
         struct in6_addr in6;
-        const char *hexStr = (const char *)hexValue;
+        const char *hex_str = (const char *)hex_value;
         int i;
 
         for (i = 0; i < 16; i++) {
@@ -2111,11 +2111,11 @@ static char *hexToIPAddress(const void *hexValue, int is_ipv6)
 
         return g_strdup(addr);
     } else {
-        unsigned int hexInt = *(unsigned int *)hexValue;
-        unsigned int byte1 = (hexInt >> 24) & 0xFF;
-        unsigned int byte2 = (hexInt >> 16) & 0xFF;
-        unsigned int byte3 = (hexInt >> 8) & 0xFF;
-        unsigned int byte4 = hexInt & 0xFF;
+        unsigned int hex_int = *(unsigned int *)hex_value;
+        unsigned int byte1 = (hex_int >> 24) & 0xFF;
+        unsigned int byte2 = (hex_int >> 16) & 0xFF;
+        unsigned int byte3 = (hex_int >> 8) & 0xFF;
+        unsigned int byte4 = hex_int & 0xFF;
 
         return g_strdup_printf("%u.%u.%u.%u", byte4, byte3, byte2, byte1);
     }
@@ -2131,6 +2131,7 @@ GuestNetworkRouteList *qmp_guest_network_get_route(Error **errp)
     int firstLine;
     int is_ipv6;
     int i;
+    char iface[IFNAMSIZ];
 
     for (i = 0; i < 2; i++) {
         firstLine = 1;
@@ -2146,72 +2147,61 @@ GuestNetworkRouteList *qmp_guest_network_get_route(Error **errp)
                 firstLine = 0;
                 continue;
             }
-            GuestNetworkRoute *route = NULL;
-            GuestNetworkRoute *networkroute;
-            char Iface[IFNAMSIZ];
-            if (is_ipv6) {
-                char Destination[33], Source[33], NextHop[33];
-                int DesPrefixlen, SrcPrefixlen, Metric, RefCnt, Use, Flags;
 
-                /* Parse the line and extract the values */
+            GuestNetworkRoute *route = g_new0(GuestNetworkRoute, 1);
+
+            if (is_ipv6) {
+                char destination[33], source[33], next_hop[33];
+                int des_prefixlen, src_prefixlen, metric, refcnt, use, flags;
                 if (sscanf(line, "%32s %x %32s %x %32s %x %x %x %x %s",
-                           Destination, &DesPrefixlen, Source,
-                           &SrcPrefixlen, NextHop, &Metric, &RefCnt,
-                           &Use, &Flags, Iface) != 10) {
+                           destination, &des_prefixlen, source,
+                           &src_prefixlen, next_hop, &metric, &refcnt,
+                           &use, &flags, iface) != 10) {
                     continue;
                 }
 
-                route = g_new0(GuestNetworkRoute, 1);
-                networkroute = route;
-                networkroute->iface = g_strdup(Iface);
-                networkroute->destination = hexToIPAddress(Destination, 1);
-                networkroute->metric = Metric;
-                networkroute->source = hexToIPAddress(Source, 1);
-                networkroute->desprefixlen = g_strdup_printf(
-                    "%d", DesPrefixlen
-                );
-                networkroute->srcprefixlen = g_strdup_printf(
-                    "%d", SrcPrefixlen
-                );
-                networkroute->nexthop = hexToIPAddress(NextHop, 1);
-                networkroute->has_flags = true;
-                networkroute->flags = Flags;
-                networkroute->has_refcnt = true;
-                networkroute->refcnt = RefCnt;
-                networkroute->has_use = true;
-                networkroute->use = Use;
-                networkroute->version = 6;
-            } else {
-                unsigned int Destination, Gateway, Mask, Flags;
-                int RefCnt, Use, Metric, MTU, Window, IRTT;
+                route->iface = g_strdup(iface);
+                route->destination = hex_to_ip_address(destination, 1);
+                route->source = hex_to_ip_address(source, 1);
+                route->nexthop = hex_to_ip_address(next_hop, 1);
+                route->desprefixlen = g_strdup_printf("%d", des_prefixlen);
+                route->srcprefixlen = g_strdup_printf("%d", src_prefixlen);
+                route->metric = metric;
+                route->has_flags = true;
+                route->flags = flags;
+                route->has_refcnt = true;
+                route->refcnt = refcnt;
+                route->has_use = true;
+                route->use = use;
+                route->version = 6;
 
-                /* Parse the line and extract the values */
+            } else {
+                unsigned int destination, gateway, mask, flags;
+                int refcnt, use, metric, mtu, window, irtt;
                 if (sscanf(line, "%s %X %X %x %d %d %d %X %d %d %d",
-                           Iface, &Destination, &Gateway, &Flags, &RefCnt,
-                           &Use, &Metric, &Mask, &MTU, &Window, &IRTT) != 11) {
+                           iface, &destination, &gateway, &flags, &refcnt,
+                           &use, &metric, &mask, &mtu, &window, &irtt) != 11) {
                     continue;
                 }
 
-                route = g_new0(GuestNetworkRoute, 1);
-                networkroute = route;
-                networkroute->iface = g_strdup(Iface);
-                networkroute->destination = hexToIPAddress(&Destination, 0);
-                networkroute->gateway = hexToIPAddress(&Gateway, 0);
-                networkroute->mask = hexToIPAddress(&Mask, 0);
-                networkroute->metric = Metric;
-                networkroute->has_flags = true;
-                networkroute->flags = Flags;
-                networkroute->has_refcnt = true;
-                networkroute->refcnt = RefCnt;
-                networkroute->has_use = true;
-                networkroute->use = Use;
-                networkroute->has_mtu = true;
-                networkroute->mtu = MTU;
-                networkroute->has_window = true;
-                networkroute->window = Window;
-                networkroute->has_irtt = true;
-                networkroute->irtt = IRTT;
-                networkroute->version = 4;
+                route->iface = g_strdup(iface);
+                route->destination = hex_to_ip_address(&destination, 0);
+                route->gateway = hex_to_ip_address(&gateway, 0);
+                route->mask = hex_to_ip_address(&mask, 0);
+                route->metric = metric;
+                route->has_flags = true;
+                route->flags = flags;
+                route->has_refcnt = true;
+                route->refcnt = refcnt;
+                route->has_use = true;
+                route->use = use;
+                route->has_mtu = true;
+                route->mtu = mtu;
+                route->has_window = true;
+                route->window = window;
+                route->has_irtt = true;
+                route->irtt = irtt;
+                route->version = 4;
             }
 
             QAPI_LIST_APPEND(tail, route);
-- 
2.40.1