The code for guest-network-get-interfaces needs a couple of small
adjustments for Solaris:

- The results from SIOCGIFHWADDR are documented as being in ifr_addr,
  not ifr_hwaddr (ifr_hwaddr doesn't exist on Solaris).

- The implementation of guest_get_network_stats is Linux-specific, so
  hide it under #ifdef CONFIG_LINUX. On non-Linux, we just won't
  provide network interface stats.

Signed-off-by: Andrew Deason <adeason@sinenomine.net>
---
 qga/commands-posix.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index bd0d67f674..c0b00fc488 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -2781,20 +2781,21 @@ guest_find_interface(GuestNetworkInterfaceList *head,
             return head->value;
         }
     }
 
     return NULL;
 }
 
 static int guest_get_network_stats(const char *name,
                        GuestNetworkInterfaceStat *stats)
 {
+#ifdef CONFIG_LINUX
     int name_len;
     char const *devinfo = "/proc/net/dev";
     FILE *fp;
     char *line = NULL, *colon;
     size_t n = 0;
     fp = fopen(devinfo, "r");
     if (!fp) {
         return -1;
     }
     name_len = strlen(name);
@@ -2836,20 +2837,21 @@ static int guest_get_network_stats(const char *name,
             stats->tx_errs = tx_errs;
             stats->tx_dropped = tx_dropped;
             fclose(fp);
             g_free(line);
             return 0;
         }
     }
     fclose(fp);
     g_free(line);
     g_debug("/proc/net/dev: Interface '%s' not found", name);
+#endif /* CONFIG_LINUX */
     return -1;
 }
 
 /*
  * Build information about guest interfaces
  */
 GuestNetworkInterfaceList *qmp_guest_network_get_interfaces(Error **errp)
 {
     GuestNetworkInterfaceList *head = NULL, **tail = &head;
     struct ifaddrs *ifap, *ifa;
@@ -2901,22 +2903,25 @@ GuestNetworkInterfaceList *qmp_guest_network_get_interfaces(Error **errp)
                 if (errno == EADDRNOTAVAIL) {
                     /* The interface doesn't have a hw addr (e.g. loopback). */
                     g_debug("failed to get MAC address of %s: %s",
                             ifa->ifa_name, strerror(errno));
                 } else{
                     g_warning("failed to get MAC address of %s: %s",
                               ifa->ifa_name, strerror(errno));
                 }
 
             } else {
+#ifdef CONFIG_SOLARIS
+                mac_addr = (unsigned char *) &ifr.ifr_addr.sa_data;
+#else
                 mac_addr = (unsigned char *) &ifr.ifr_hwaddr.sa_data;
-
+#endif
                 info->hardware_address =
                     g_strdup_printf("%02x:%02x:%02x:%02x:%02x:%02x",
                                     (int) mac_addr[0], (int) mac_addr[1],
                                     (int) mac_addr[2], (int) mac_addr[3],
                                     (int) mac_addr[4], (int) mac_addr[5]);
 
                 info->has_hardware_address = true;
             }
             close(sock);
         }
-- 
2.11.0

On 3/20/22 22:38, Andrew Deason wrote:
> The code for guest-network-get-interfaces needs a couple of small
> adjustments for Solaris:
> 
> - The results from SIOCGIFHWADDR are documented as being in ifr_addr,
>   not ifr_hwaddr (ifr_hwaddr doesn't exist on Solaris).
> 
> - The implementation of guest_get_network_stats is Linux-specific, so
>   hide it under #ifdef CONFIG_LINUX. On non-Linux, we just won't
>   provide network interface stats.
> 
> Signed-off-by: Andrew Deason <adeason@sinenomine.net>
> ---
>  qga/commands-posix.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index bd0d67f674..c0b00fc488 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -2781,20 +2781,21 @@ guest_find_interface(GuestNetworkInterfaceList *head,
>              return head->value;
>          }
>      }
>  
>      return NULL;
>  }
>  
>  static int guest_get_network_stats(const char *name,
>                         GuestNetworkInterfaceStat *stats)
>  {
> +#ifdef CONFIG_LINUX
>      int name_len;
>      char const *devinfo = "/proc/net/dev";
>      FILE *fp;
>      char *line = NULL, *colon;
>      size_t n = 0;
>      fp = fopen(devinfo, "r");
>      if (!fp) {
>          return -1;
>      }
>      name_len = strlen(name);
> @@ -2836,20 +2837,21 @@ static int guest_get_network_stats(const char *name,
>              stats->tx_errs = tx_errs;
>              stats->tx_dropped = tx_dropped;
>              fclose(fp);
>              g_free(line);
>              return 0;
>          }
>      }
>      fclose(fp);
>      g_free(line);
>      g_debug("/proc/net/dev: Interface '%s' not found", name);
> +#endif /* CONFIG_LINUX */

I wonder whether we should signal this somehow. I mean, have something
like this:

#else /* !CONFIG_LINUX */
  g_debug("Stats reporting available only for Linux");
#endif /* !CONFIG_LINUX */

>      return -1;
>  }

A counter argument is that if fopen() above fails then -1 is returned
without any error/debug message reported. And stats fetching is best
effort anyway.

Michal

On Mon, 21 Mar 2022 10:14:57 +0100
Michal Prívozník <mprivozn@redhat.com> wrote:

> On 3/20/22 22:38, Andrew Deason wrote:
> > The code for guest-network-get-interfaces needs a couple of small
> > adjustments for Solaris:
> > 
> > - The results from SIOCGIFHWADDR are documented as being in ifr_addr,
> >   not ifr_hwaddr (ifr_hwaddr doesn't exist on Solaris).
> > 
> > - The implementation of guest_get_network_stats is Linux-specific, so
> >   hide it under #ifdef CONFIG_LINUX. On non-Linux, we just won't
> >   provide network interface stats.
> > 
> > Signed-off-by: Andrew Deason <adeason@sinenomine.net>
> > ---
> >  qga/commands-posix.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> > index bd0d67f674..c0b00fc488 100644
> > --- a/qga/commands-posix.c
> > +++ b/qga/commands-posix.c
> > @@ -2781,20 +2781,21 @@ guest_find_interface(GuestNetworkInterfaceList *head,
> >              return head->value;
> >          }
> >      }
> >  
> >      return NULL;
> >  }
> >  
> >  static int guest_get_network_stats(const char *name,
> >                         GuestNetworkInterfaceStat *stats)
> >  {
> > +#ifdef CONFIG_LINUX
> >      int name_len;
> >      char const *devinfo = "/proc/net/dev";
> >      FILE *fp;
> >      char *line = NULL, *colon;
> >      size_t n = 0;
> >      fp = fopen(devinfo, "r");
> >      if (!fp) {
> >          return -1;
> >      }
> >      name_len = strlen(name);
> > @@ -2836,20 +2837,21 @@ static int guest_get_network_stats(const char *name,
> >              stats->tx_errs = tx_errs;
> >              stats->tx_dropped = tx_dropped;
> >              fclose(fp);
> >              g_free(line);
> >              return 0;
> >          }
> >      }
> >      fclose(fp);
> >      g_free(line);
> >      g_debug("/proc/net/dev: Interface '%s' not found", name);
> > +#endif /* CONFIG_LINUX */
> 
> I wonder whether we should signal this somehow. I mean, have something
> like this:
> 
> #else /* !CONFIG_LINUX */
>   g_debug("Stats reporting available only for Linux");
> #endif /* !CONFIG_LINUX */
> 
> >      return -1;
> >  }
> 
> A counter argument is that if fopen() above fails then -1 is returned
> without any error/debug message reported. And stats fetching is best
> effort anyway.

Ping for this stack. Should I just go ahead and add the above? Sorry if
I was expected to respond to this; I don't disagree but I also saw the
existing silent-failure code path so it doesn't seem like it matters.

I could add debug messages for both silent-failure code paths, maybe as
a separate commit afterwards?

-- 
Andrew Deason
adeason@sinenomine.net