apport-hackers team mailing list archive

[Julian Andres Klode <julian.klode@xxxxxxxxxxxxx>]

Julian Andres Klode has proposed merging lp:~juliank/apport/named-arguments into lp:apport.

Requested reviews:
  Apport upstream developers (apport-hackers)

For more details, see:
https://code.launchpad.net/~juliank/apport/named-arguments/+merge/337590

New command line parsing for bug 1732962
-- 
Your team Apport upstream developers is requested to review the proposed merge of lp:~juliank/apport/named-arguments into lp:apport.

=== modified file 'NEWS'
--- NEWS	2018-02-02 01:22:59 +0000
+++ NEWS	2018-02-12 17:27:35 +0000
@@ -26,6 +26,8 @@
     case so check for both.
   * data/apport: add an exception handler in case either name space can not be
     found.
+  * data/apport: Introduce support for non-positional arguments so we
+    can easily extend core_pattern in the future (LP: #1732962)
 
 2.20.8 (2017-11-15)
 -------------------

=== modified file 'data/apport'
--- data/apport	2018-02-02 01:22:59 +0000
+++ data/apport	2018-02-12 17:27:35 +0000
@@ -15,7 +15,7 @@
 
 import sys, os, os.path, subprocess, time, traceback, pwd, io
 import signal, inspect, grp, fcntl, socket, atexit, array, struct
-import errno
+import errno, argparse
 
 import apport, apport.fileutils
 
@@ -317,6 +317,55 @@
     return False
 
 
+def parse_arguments():
+    parser = argparse.ArgumentParser(epilog="""
+    Alternatively, the following command line is understood for legacy hosts:
+        <pid> <signal number> <core file ulimit> <dump mode> [global pid]
+    """)
+
+    # TODO: Use type=int
+    parser.add_argument("--pid", help="process id (%%p)")
+    parser.add_argument("--signal-number", help="signal number (%%s)")
+    parser.add_argument("--core-ulimit", help="core ulimit (%%c)")
+    parser.add_argument("--dump-mode", help="dump mode (%%d)")
+    parser.add_argument("--global-pid", nargs='?', help="pid in root namespace (%%P)")
+    parser.add_argument("--more", type=str, help="""Legacy interface for kernel use.
+        This option is used to core_pattern, in order to allow legacy
+        apport versions that only understand 4 or 5 positional arguments to
+        forward them to the host.
+
+        It is a simple list of the form key=value separated by comma. The
+        keys are the same as the arguments, just with - replaced by _.""")
+
+    options, rest = parser.parse_known_args()
+
+    if options.pid is not None:
+        for arg in rest:
+            error_log("Unknown argument: %s", arg)
+
+        if options.more is not None:
+            for k, v in [o.split("=") for o in options.more.split(",")]:
+                # TODO: Values should be int
+                setattr(options, k, v)
+
+    elif len(rest) in (4,5):
+        # Translate legacy command line
+        options.pid = rest[1]
+        options.signal_number = rest[2]
+        options.core_ulimit = rest[3]
+        options.dump_mode = rest[4]
+        try:
+            options.global_pid = rest[5]
+        except IndexError:
+            options.global_pid = None
+    else:
+        parser.print_usage()
+        sys.exit(1)
+
+    del options.more
+    return options
+
+
 #################################################################
 #
 # main
@@ -365,16 +414,8 @@
         error_log('Received a bad number of arguments from forwarder, received %d, expected 5, aborting.' % len(sys.argv))
         sys.exit(1)
 
-# Normal startup
-if len(sys.argv) not in (5, 6):
-    try:
-        print('Usage: %s <pid> <signal number> <core file ulimit> <dump mode> [global pid]' % sys.argv[0])
-        print('The core dump is read from stdin.')
-    except IOError:
-        # sys.stderr might not actually exist, expecially not when being called
-        # from the kernel
-        pass
-    sys.exit(1)
+
+options = parse_arguments()
 
 init_error_log()
 
@@ -382,8 +423,8 @@
 # then compare it with the local PID. If they don't match, it's an
 # indication that the crash originated from another PID namespace.
 # Simply log an entry in the host error log and exit 0.
-if len(sys.argv) == 6:
-    host_pid = int(sys.argv[5])
+if options.global_pid is not None:
+    host_pid = int(options.global_pid)
 
     if not is_same_ns(host_pid, "pid") and not is_same_ns(host_pid, "mnt"):
         # If the crash came from a container, don't attempt to handle
@@ -393,7 +434,7 @@
         # forward the process information there.
         if not os.path.exists('/proc/%d/root/run/apport.socket' % host_pid):
             error_log('host pid %s crashed in a container without apport support' %
-                      sys.argv[5])
+                      options.global_pid)
             sys.exit(0)
 
         sock = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM)
@@ -401,7 +442,7 @@
             sock.connect('/proc/%d/root/run/apport.socket' % host_pid)
         except Exception as e:
             error_log('host pid %s crashed in a container with a broken apport' %
-                      sys.argv[5])
+                      options.global_pid)
             sys.exit(0)
 
         # Send all arguments except for the first (exec path) and last (global pid)
@@ -428,7 +469,7 @@
         # sandbox will use container namespaces as a security measure but are
         # still otherwise host processes. When that's the case, we need to keep
         # handling those crashes locally using the global pid.
-        sys.argv[1] = str(host_pid)
+        options.pid = str(host_pid)
     elif not is_same_ns(host_pid, "mnt"):
         error_log('host pid %s crashed in a separate mount namespace, ignoring' % host_pid)
         sys.exit(0)
@@ -438,7 +479,10 @@
 try:
     setup_signals()
 
-    (pid, signum, core_ulimit, dump_mode) = sys.argv[1:5]
+    pid = options.pid
+    signum = options.signal_number
+    core_ulimit = options.core_ulimit
+    dump_mode = options.dump_mode
 
     get_pid_info(pid)
 

=== modified file 'etc/init.d/apport'
--- etc/init.d/apport	2017-11-15 20:07:23 +0000
+++ etc/init.d/apport	2018-02-12 17:27:35 +0000
@@ -50,7 +50,7 @@
 		rm -f /var/lib/pm-utils/resume-hang.log
 	fi
 
-	echo "|$AGENT %p %s %c %d %P" > /proc/sys/kernel/core_pattern
+	echo "|$AGENT --pid=%p --more signal_number=%s,core_ulimit=%c,dump_mode=%d --global-pid %P" > /proc/sys/kernel/core_pattern
 	echo 2 > /proc/sys/fs/suid_dumpable
 }