Re: [PATCH 3/3] qga/commands-posix: Fix listing ifaces for Solaris

[Andrew Deason <adeason@sinenomine.net>]

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