[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 3/3] qga/commands-posix: Fix listing ifaces for Solaris
From: |
Andrew Deason |
Subject: |
Re: [PATCH 3/3] qga/commands-posix: Fix listing ifaces for Solaris |
Date: |
Mon, 28 Mar 2022 12:44:30 -0500 |
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