Currently, lsevtchn aborts its event channel enumeration when it hits
its first hypercall error, namely:
* When an event channel doesn't exist at the specified port
* When the event channel is owned by Xen

lsevtchn does not distinguish between different hypercall errors, which
results in lsevtchn missing potential relevant event channels with
higher port numbers.

Use the errno macro to distinguish between hypercall errors, and
continue event channel enumeration if the hypercall error is not
critical to enumeration.

Signed-off-by: Matthew Barnes <matthew.barnes@cloud.com>
---
 tools/xcutils/lsevtchn.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/tools/xcutils/lsevtchn.c b/tools/xcutils/lsevtchn.c
index d1710613ddc5..e4b3f88fe4ec 100644
--- a/tools/xcutils/lsevtchn.c
+++ b/tools/xcutils/lsevtchn.c
@@ -3,6 +3,7 @@
 #include <stdint.h>
 #include <string.h>
 #include <stdio.h>
+#include <errno.h>
 
 #include <xenctrl.h>
 
@@ -24,7 +25,18 @@ int main(int argc, char **argv)
         status.port = port;
         rc = xc_evtchn_status(xch, &status);
         if ( rc < 0 )
-            break;
+        {
+            if ( errno == ESRCH )
+            {
+                fprintf(stderr, "Domain ID '%d' does not correspond to valid domain.\n", domid);
+                break;
+            }
+
+            if ( errno == EINVAL )
+                break;
+
+            continue;
+        }
 
         if ( status.status == EVTCHNSTAT_closed )
             continue;
-- 
2.34.1

On Mon, Jul 15, 2024 at 04:36:03PM +0100, Matthew Barnes wrote:
> Currently, lsevtchn aborts its event channel enumeration when it hits
> its first hypercall error, namely:
> * When an event channel doesn't exist at the specified port
> * When the event channel is owned by Xen
> 
> lsevtchn does not distinguish between different hypercall errors, which
> results in lsevtchn missing potential relevant event channels with
> higher port numbers.
> 
> Use the errno macro to distinguish between hypercall errors, and
> continue event channel enumeration if the hypercall error is not
> critical to enumeration.
> 
> Signed-off-by: Matthew Barnes <matthew.barnes@cloud.com>
> ---
>  tools/xcutils/lsevtchn.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/xcutils/lsevtchn.c b/tools/xcutils/lsevtchn.c
> index d1710613ddc5..e4b3f88fe4ec 100644
> --- a/tools/xcutils/lsevtchn.c
> +++ b/tools/xcutils/lsevtchn.c
> @@ -24,7 +25,18 @@ int main(int argc, char **argv)
>          status.port = port;
>          rc = xc_evtchn_status(xch, &status);
>          if ( rc < 0 )
> -            break;
> +        {
> +            if ( errno == ESRCH )
> +            {
> +                fprintf(stderr, "Domain ID '%d' does not correspond to valid domain.\n", domid);
> +                break;
> +            }
> +
> +            if ( errno == EINVAL )
> +                break;
> +
> +            continue;

Usually, when there's an error, we want to deal with it properly, and
not blindly ignore them. Instead, could you check for error that are
non-fatal, like described in the patch description?

Also, the code change doesn't reflect the patch description. The
description says "continue if error not fatal", but the implementation
is "exit on few known fatal error".

One other potentially useful thing to do would be to print why
xc_evtchn_status() failed like you did for ESRCH, but with perror() (or
err() or warn()) which print a translation of the errno value into text.
Of course, only in case where we stop the enumeration.

Thanks,

-- 

Anthony Perard | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

On 08.05.2024 13:04, Matthew Barnes wrote:
> @@ -24,7 +25,18 @@ int main(int argc, char **argv)
>          status.port = port;
>          rc = xc_evtchn_status(xch, &status);
>          if ( rc < 0 )
> -            break;
> +        {
> +            if ( errno == ESRCH )
> +            {
> +                fprintf(stderr, "Domain ID '%d' does not correspond to valid domain.\n", domid);
> +                break;
> +            }
> +
> +            if ( errno == EINVAL )
> +                break;
> +
> +            continue;
> +        }

Hmm, I'm not sure "black listing" certain error codes is useful. I'd have
expected a "white listing" approach, special casing just EACCES and EPERM
(which iirc is what XSM would return). I'm also not convinced of the
error message text of the ESRCH case you special case: There are valid
domain IDs which still cannot be used with rcu_lock_domain_by_any_id(),
e.g. DOM_IO and DOM_XEN.

I'd be curious to hear what others think.

Andrew, ftaod - this is the patch I've mentioned in reply to your revert
touching evtchn_status(). And as mentioned there - lsevtchn should never
have outright bailed on _any_ error it gets back (i.e. even ones coming
from XSM).

Jan