[PATCH] virNetDevOpenvswitchInterfaceStats: Add 'ifname' to error messages

Peter Krempa via Devel posted 1 patch 2 weeks, 3 days ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/b6257226f616f4aeab68bd1a8edad3397cc61bdc.1777358939.git.pkrempa@redhat.com
src/util/virnetdevopenvswitch.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
[PATCH] virNetDevOpenvswitchInterfaceStats: Add 'ifname' to error messages
Posted by Peter Krempa via Devel 2 weeks, 3 days ago
From: Peter Krempa <pkrempa@redhat.com>

Report the interface name which caused the error.

Resolves: https://redhat.atlassian.net/browse/RHEL-170993
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
---
 src/util/virnetdevopenvswitch.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/src/util/virnetdevopenvswitch.c b/src/util/virnetdevopenvswitch.c
index 868d6d26ba..d6457f35a0 100644
--- a/src/util/virnetdevopenvswitch.c
+++ b/src/util/virnetdevopenvswitch.c
@@ -403,8 +403,8 @@ virNetDevOpenvswitchInterfaceStats(const char *ifname,
         STREQ_NULLABLE(output, "")) {
         /* no ovs-vsctl or interface 'ifname' doesn't exists in ovs */
         virReportError(VIR_ERR_INTERNAL_ERROR,
-                       _("Interface not found: %1$s"),
-                       NULLSTR(errbuf));
+                       _("Interface '%1$s' not found: %2$s"),
+                       ifname, NULLSTR(errbuf));
         return -1;
     }

@@ -419,8 +419,9 @@ virNetDevOpenvswitchInterfaceStats(const char *ifname,
         stats->tx_packets == -1 &&
         stats->tx_errs == -1 &&
         stats->tx_drop == -1) {
-        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-                       _("Interface doesn't have any statistics"));
+        virReportError(VIR_ERR_INTERNAL_ERROR,
+                       _("Interface '%1$s' doesn't have any statistics"),
+                       ifname);
         return -1;
     }

-- 
2.53.0
Re: [PATCH] virNetDevOpenvswitchInterfaceStats: Add 'ifname' to error messages
Posted by Martin Kletzander via Devel 2 weeks, 3 days ago
On Tue, Apr 28, 2026 at 08:48:59AM +0200, Peter Krempa via Devel wrote:
>From: Peter Krempa <pkrempa@redhat.com>
>
>Report the interface name which caused the error.
>
>Resolves: https://redhat.atlassian.net/browse/RHEL-170993
>Signed-off-by: Peter Krempa <pkrempa@redhat.com>
>---
> src/util/virnetdevopenvswitch.c | 9 +++++----
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
>diff --git a/src/util/virnetdevopenvswitch.c b/src/util/virnetdevopenvswitch.c
>index 868d6d26ba..d6457f35a0 100644
>--- a/src/util/virnetdevopenvswitch.c
>+++ b/src/util/virnetdevopenvswitch.c
>@@ -403,8 +403,8 @@ virNetDevOpenvswitchInterfaceStats(const char *ifname,
>         STREQ_NULLABLE(output, "")) {
>         /* no ovs-vsctl or interface 'ifname' doesn't exists in ovs */
>         virReportError(VIR_ERR_INTERNAL_ERROR,
>-                       _("Interface not found: %1$s"),
>-                       NULLSTR(errbuf));
>+                       _("Interface '%1$s' not found: %2$s"),
>+                       ifname, NULLSTR(errbuf));

You could use the opportunity to differentiate between no interface and
ovs-vsctl missing, but I guess the latter is less likely, and it's
strictly better than before, so it's fine.

Reviewed-by: Martin Kletzander <mkletzan@redhat.com>

and safe for freeze IMNSHO

>         return -1;
>     }
>
>@@ -419,8 +419,9 @@ virNetDevOpenvswitchInterfaceStats(const char *ifname,
>         stats->tx_packets == -1 &&
>         stats->tx_errs == -1 &&
>         stats->tx_drop == -1) {
>-        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>-                       _("Interface doesn't have any statistics"));
>+        virReportError(VIR_ERR_INTERNAL_ERROR,
>+                       _("Interface '%1$s' doesn't have any statistics"),
>+                       ifname);
>         return -1;
>     }
>
>-- 
>2.53.0
>
Re: [PATCH] virNetDevOpenvswitchInterfaceStats: Add 'ifname' to error messages
Posted by Peter Krempa via Devel 2 weeks, 2 days ago
On Tue, Apr 28, 2026 at 10:57:12 +0200, Martin Kletzander wrote:
> On Tue, Apr 28, 2026 at 08:48:59AM +0200, Peter Krempa via Devel wrote:
> > From: Peter Krempa <pkrempa@redhat.com>
> > 
> > Report the interface name which caused the error.
> > 
> > Resolves: https://redhat.atlassian.net/browse/RHEL-170993
> > Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> > ---
> > src/util/virnetdevopenvswitch.c | 9 +++++----
> > 1 file changed, 5 insertions(+), 4 deletions(-)
> > 
> > diff --git a/src/util/virnetdevopenvswitch.c b/src/util/virnetdevopenvswitch.c
> > index 868d6d26ba..d6457f35a0 100644
> > --- a/src/util/virnetdevopenvswitch.c
> > +++ b/src/util/virnetdevopenvswitch.c
> > @@ -403,8 +403,8 @@ virNetDevOpenvswitchInterfaceStats(const char *ifname,
> >         STREQ_NULLABLE(output, "")) {
> >         /* no ovs-vsctl or interface 'ifname' doesn't exists in ovs */
> >         virReportError(VIR_ERR_INTERNAL_ERROR,
> > -                       _("Interface not found: %1$s"),
> > -                       NULLSTR(errbuf));
> > +                       _("Interface '%1$s' not found: %2$s"),
> > +                       ifname, NULLSTR(errbuf));
> 
> You could use the opportunity to differentiate between no interface and
> ovs-vsctl missing, but I guess the latter is less likely, and it's

Hmm, yeah. I didn't even notice that the virCommandRun call was involved
there too. Since virCommandRun does report own errors just separating it
would seem to be the correct thing, but unfortunately it's
virCommandRun(cmd, NULL), which reports error also on non-zero return
value, and I honestly have absolutely no idea if 'ovs-vsctl' can fail in
a way where we'd want to actually rewrite the error message.

And more importantly I don't think I care enough to find out :)

> strictly better than before, so it's fine.
> 
> Reviewed-by: Martin Kletzander <mkletzan@redhat.com>

So I'll use this in the as-is state ;)