[PATCH] tools/timerlat: Enhance Error Handling and Readability for timerlat_load.py

[PATCH] tools/timerlat: Enhance Error Handling and Readability for timerlat_load.py

[furkanonder]

[-- Attachment #1.1: Type: text/plain, Size: 2002 bytes --]

The enhancements made to timerlat_load.py focus on improving error handling, readability, and overall user experience. These changes aim to make the script more robust and easier to maintain while providing clearer feedback to users.
Key modifications include:

Type Declaration in Argument Parsing:
Added type declarations for command-line arguments in the argument parser. This removes the need for manual type checks later in the code, improving clarity and safety.
Before:
parser.add_argument("cpu", help='CPU to run timerlat thread')
parser.add_argument("-p", "--prio", help='FIFO priority')
After:
parser.add_argument("cpu", type=int, help='CPU to run timerlat thread')
parser.add_argument("-p", "--prio", type=int, help='FIFO priority')

String Formatting:
Replaced string concatenation with an f-string to enhancing readability and conciseness.
Before:
timerlat_path = "/sys/kernel/tracing/osnoise/per_cpu/cpu" + args.cpu + "/timerlat_fd"
After:
timerlat_path = f"/sys/kernel/tracing/osnoise/per_cpu/cpu{args.cpu}/timerlat_fd"

Enhanced Exception Handling and Consistent Error Reporting:
Specific exceptions are now caught and printed with clearer messages, providing context for errors when opening file descriptors. Added exception
handling for the data file descriptor opening to ensure uniformity across the script.

Before:
$ sudo python timerlat_load.py 122
Error setting affinity
After:
$ sudo python timerlat_load.py 122
Error setting affinity: [Errno 22] Invalid argument

Before:
$ sudo python timerlat_load.py 1 -p 950
Error setting priority
After:
$ sudo python timerlat_load.py 1 -p 950
Error setting priority: [Errno 22] Invalid argument

Before:
$ python timerlat_load.py 1
Error opening timerlat fd, did you run timerlat -U?
After:
$ python timerlat_load.py 1
Permission denied. Please check your access rights.

Changes for the read Infinite Loop:
The original generic exception clause has been replaced with more specific exception types to provide clearer feedback on errors.

[-- Attachment #1.2: Type: text/html, Size: 21531 bytes --]

[-- Attachment #2: patch --]
[-- Type: application/octet-stream, Size: 2428 bytes --]

--- tools/tracing/rtla/sample/timerlat_load.py.orig	2024-09-22 02:37:14.767832391 +0300
+++ tools/tracing/rtla/sample/timerlat_load.py	2024-09-22 02:37:05.721215367 +0300
@@ -25,50 +25,54 @@ import sys
 import os
 
 parser = argparse.ArgumentParser(description='user-space timerlat thread in Python')
-parser.add_argument("cpu", help='CPU to run timerlat thread')
-parser.add_argument("-p", "--prio", help='FIFO priority')
-
+parser.add_argument("cpu", type=int, help='CPU to run timerlat thread')
+parser.add_argument("-p", "--prio", type=int, help='FIFO priority')
 args = parser.parse_args()
 
 try:
-    affinity_mask = { int(args.cpu) }
-except:
-    print("Invalid cpu: " + args.cpu)
-    exit(1)
-
-try:
-    os.sched_setaffinity(0, affinity_mask);
-except:
-    print("Error setting affinity")
-    exit(1)
+    affinity_mask = {args.cpu}
+    os.sched_setaffinity(0, affinity_mask)
+except Exception as e:
+    print(f"Error setting affinity: {e}")
+    sys.exit(1)
 
-if (args.prio):
+if args.prio:
     try:
-        param = os.sched_param(int(args.prio))
+        param = os.sched_param(args.prio)
         os.sched_setscheduler(0, os.SCHED_FIFO, param)
-    except:
-        print("Error setting priority")
-        exit(1)
+    except Exception as e:
+        print(f"Error setting priority: {e}")
+        sys.exit(1)
 
 try:
-    timerlat_path = "/sys/kernel/tracing/osnoise/per_cpu/cpu" + args.cpu + "/timerlat_fd"
+    timerlat_path = f"/sys/kernel/tracing/osnoise/per_cpu/cpu{args.cpu}/timerlat_fd"
     timerlat_fd = open(timerlat_path, 'r')
-except:
+except PermissionError:
+    print("Permission denied. Please check your access rights.")
+    sys.exit(1)
+except OSError:
     print("Error opening timerlat fd, did you run timerlat -U?")
-    exit(1)
+    sys.exit(1)
 
 try:
-    data_fd = open("/dev/full", 'r');
-except:
-    print("Error opening data fd")
+    data_fd = open("/dev/full", 'r')
+except Exception as e:
+    print(f"Error opening data fd: {e}")
+    sys.exit(1)
 
 while True:
     try:
         timerlat_fd.read(1)
-        data_fd.read(20*1024*1024)
-    except:
+        data_fd.read(20 * 1024 * 1024)
+    except KeyboardInterrupt:
         print("Leaving")
         break
+    except IOError as e:
+        print(f"I/O error occurred: {e}")
+        break
+    except Exception as e:
+        print(f"Unexpected error: {e}")
+        break
 
 timerlat_fd.close()
 data_fd.close()

Re: [PATCH] tools/timerlat: Enhance Error Handling and Readability for timerlat_load.py

[Steven Rostedt]

Hi,

Thank you for your patch. First, Daniel is unfortunately no longer the
maintainer of this code:

  https://lwn.net/Articles/979912/

This patch is not formatted correctly, so it can not even be reviewed.
Please take a look at Submitting Patches:

  https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html

-- Steve


On Sat, 21 Sep 2024 23:43:02 +0000
furkanonder <furkanonder@protonmail.com> wrote:

> The enhancements made to timerlat_load.py focus on improving error handling, readability, and overall user experience. These changes aim to make the script more robust and easier to maintain while providing clearer feedback to users.
> Key modifications include:
> 
> Type Declaration in Argument Parsing:
> Added type declarations for command-line arguments in the argument parser. This removes the need for manual type checks later in the code, improving clarity and safety.
> Before:
> parser.add_argument("cpu", help='CPU to run timerlat thread')
> parser.add_argument("-p", "--prio", help='FIFO priority')
> After:
> parser.add_argument("cpu", type=int, help='CPU to run timerlat thread')
> parser.add_argument("-p", "--prio", type=int, help='FIFO priority')
> 
> String Formatting:
> Replaced string concatenation with an f-string to enhancing readability and conciseness.
> Before:
> timerlat_path = "/sys/kernel/tracing/osnoise/per_cpu/cpu" + args.cpu + "/timerlat_fd"
> After:
> timerlat_path = f"/sys/kernel/tracing/osnoise/per_cpu/cpu{args.cpu}/timerlat_fd"
> 
> Enhanced Exception Handling and Consistent Error Reporting:
> Specific exceptions are now caught and printed with clearer messages, providing context for errors when opening file descriptors. Added exception
> handling for the data file descriptor opening to ensure uniformity across the script.
> 
> Before:
> $ sudo python timerlat_load.py 122
> Error setting affinity
> After:
> $ sudo python timerlat_load.py 122
> Error setting affinity: [Errno 22] Invalid argument
> 
> Before:
> $ sudo python timerlat_load.py 1 -p 950
> Error setting priority
> After:
> $ sudo python timerlat_load.py 1 -p 950
> Error setting priority: [Errno 22] Invalid argument
> 
> Before:
> $ python timerlat_load.py 1
> Error opening timerlat fd, did you run timerlat -U?
> After:
> $ python timerlat_load.py 1
> Permission denied. Please check your access rights.
> 
> Changes for the read Infinite Loop:
> The original generic exception clause has been replaced with more specific exception types to provide clearer feedback on errors.

Re: [PATCH] tools/timerlat: Enhance Error Handling and Readability for timerlat_load.py

[furkanonder]

I’m sorry; I included the patch as an attachment in the email without noticing.

--- tools/tracing/rtla/sample/timerlat_load.py.orig	2024-09-22 02:37:14.767832391 +0300
+++ tools/tracing/rtla/sample/timerlat_load.py	2024-09-22 02:37:05.721215367 +0300
@@ -25,50 +25,54 @@ import sys
 import os
 
 parser = argparse.ArgumentParser(description='user-space timerlat thread in Python')
-parser.add_argument("cpu", help='CPU to run timerlat thread')
-parser.add_argument("-p", "--prio", help='FIFO priority')
-
+parser.add_argument("cpu", type=int, help='CPU to run timerlat thread')
+parser.add_argument("-p", "--prio", type=int, help='FIFO priority')
 args = parser.parse_args()
 
 try:
-    affinity_mask = { int(args.cpu) }
-except:
-    print("Invalid cpu: " + args.cpu)
-    exit(1)
-
-try:
-    os.sched_setaffinity(0, affinity_mask);
-except:
-    print("Error setting affinity")
-    exit(1)
+    affinity_mask = {args.cpu}
+    os.sched_setaffinity(0, affinity_mask)
+except Exception as e:
+    print(f"Error setting affinity: {e}")
+    sys.exit(1)
 
-if (args.prio):
+if args.prio:
     try:
-        param = os.sched_param(int(args.prio))
+        param = os.sched_param(args.prio)
         os.sched_setscheduler(0, os.SCHED_FIFO, param)
-    except:
-        print("Error setting priority")
-        exit(1)
+    except Exception as e:
+        print(f"Error setting priority: {e}")
+        sys.exit(1)
 
 try:
-    timerlat_path = "/sys/kernel/tracing/osnoise/per_cpu/cpu" + args.cpu + "/timerlat_fd"
+    timerlat_path = f"/sys/kernel/tracing/osnoise/per_cpu/cpu{args.cpu}/timerlat_fd"
     timerlat_fd = open(timerlat_path, 'r')
-except:
+except PermissionError:
+    print("Permission denied. Please check your access rights.")
+    sys.exit(1)
+except OSError:
     print("Error opening timerlat fd, did you run timerlat -U?")
-    exit(1)
+    sys.exit(1)
 
 try:
-    data_fd = open("/dev/full", 'r');
-except:
-    print("Error opening data fd")
+    data_fd = open("/dev/full", 'r')
+except Exception as e:
+    print(f"Error opening data fd: {e}")
+    sys.exit(1)
 
 while True:
     try:
         timerlat_fd.read(1)
-        data_fd.read(20*1024*1024)
-    except:
+        data_fd.read(20 * 1024 * 1024)
+    except KeyboardInterrupt:
         print("Leaving")
         break
+    except IOError as e:
+        print(f"I/O error occurred: {e}")
+        break
+    except Exception as e:
+        print(f"Unexpected error: {e}")
+        break
 
 timerlat_fd.close()
 data_fd.close()


Sent with Proton Mail secure email.

On Thursday, September 26th, 2024 at 7:38 AM, Steven Rostedt <rostedt@goodmis.org> wrote:

> Hi,
> 
> Thank you for your patch. First, Daniel is unfortunately no longer the
> maintainer of this code:
> 
> https://lwn.net/Articles/979912/
> 
> This patch is not formatted correctly, so it can not even be reviewed.
> Please take a look at Submitting Patches:
> 
> https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html
> 
> -- Steve
> 
> 
> On Sat, 21 Sep 2024 23:43:02 +0000
> furkanonder furkanonder@protonmail.com wrote:
> 
> > The enhancements made to timerlat_load.py focus on improving error handling, readability, and overall user experience. These changes aim to make the script more robust and easier to maintain while providing clearer feedback to users.
> > Key modifications include:
> > 
> > Type Declaration in Argument Parsing:
> > Added type declarations for command-line arguments in the argument parser. This removes the need for manual type checks later in the code, improving clarity and safety.
> > Before:
> > parser.add_argument("cpu", help='CPU to run timerlat thread')
> > parser.add_argument("-p", "--prio", help='FIFO priority')
> > After:
> > parser.add_argument("cpu", type=int, help='CPU to run timerlat thread')
> > parser.add_argument("-p", "--prio", type=int, help='FIFO priority')
> > 
> > String Formatting:
> > Replaced string concatenation with an f-string to enhancing readability and conciseness.
> > Before:
> > timerlat_path = "/sys/kernel/tracing/osnoise/per_cpu/cpu" + args.cpu + "/timerlat_fd"
> > After:
> > timerlat_path = f"/sys/kernel/tracing/osnoise/per_cpu/cpu{args.cpu}/timerlat_fd"
> > 
> > Enhanced Exception Handling and Consistent Error Reporting:
> > Specific exceptions are now caught and printed with clearer messages, providing context for errors when opening file descriptors. Added exception
> > handling for the data file descriptor opening to ensure uniformity across the script.
> > 
> > Before:
> > $ sudo python timerlat_load.py 122
> > Error setting affinity
> > After:
> > $ sudo python timerlat_load.py 122
> > Error setting affinity: [Errno 22] Invalid argument
> > 
> > Before:
> > $ sudo python timerlat_load.py 1 -p 950
> > Error setting priority
> > After:
> > $ sudo python timerlat_load.py 1 -p 950
> > Error setting priority: [Errno 22] Invalid argument
> > 
> > Before:
> > $ python timerlat_load.py 1
> > Error opening timerlat fd, did you run timerlat -U?
> > After:
> > $ python timerlat_load.py 1
> > Permission denied. Please check your access rights.
> > 
> > Changes for the read Infinite Loop:
> > The original generic exception clause has been replaced with more specific exception types to provide clearer feedback on errors.

Re: [PATCH] tools/timerlat: Enhance Error Handling and Readability for timerlat_load.py

[Steven Rostedt]

On Sun, 29 Sep 2024 23:21:05 +0000
furkanonder <furkanonder@protonmail.com> wrote:

> I’m sorry; I included the patch as an attachment in the email without noticing.

Yes, but still, you need to have the patch with the change log and the
signed-off-by. Not as a separate email.

Try using "git send-email", that can probably help.

-- Steve