Merge pull request #224 from delphix/master

Merge branch 'master' into '6.0/stage'

Author: sdimitro

sdb/__init__.py

@@ -30,7 +30,8 @@
 #
 from sdb.error import (Error, CommandNotFoundError, CommandError,
                        CommandInvalidInputError, SymbolNotFoundError,
-                       CommandArgumentsError, CommandEvalSyntaxError)
+                       CommandArgumentsError, CommandEvalSyntaxError,
+                       ParserError)
 from sdb.target import (create_object, get_object, get_prog, get_typed_null,
                         get_type, get_pointer_type, get_target_flags,
                         get_symbol, type_canonical_name, type_canonicalize,
@@ -53,6 +54,7 @@
     'Error',
     'InputHandler',
     'Locator',
+    'ParserError',
     'PrettyPrinter',
     'SingleInputCommand',
     'SymbolNotFoundError',

sdb/command.py

@@ -215,13 +215,47 @@ def help(cls, name: str) -> None:
 
     input_type: Optional[str] = None
 
-    def __init__(self, args: str = "", name: str = "_") -> None:
+    def __init__(self,
+                 args: Optional[List[str]] = None,
+                 name: str = "_") -> None:
         self.name = name
         self.isfirst = False
         self.islast = False
 
         self.parser = type(self)._init_parser(name)
-        self.args = self.parser.parse_args(args.split())
+
+        #
+        # The if-else clauses below may seem like it can be avoided by:
+        #
+        #     [1] Passing the `args` function argument to parse_args() even if
+        #         it is None - the call won't blow up.
+        #
+        #  or [2] Setting the default value of `args` to be [] instead of None.
+        #
+        # Solution [1] doesn't work because parse_args() actually distinguishes
+        # between None and [] as parameters. If [] is passed it returns an
+        # argparse.Namespace() with default values for all the fields that the
+        # command specified in _init_parser(), which is what we want. If None
+        # is passed then argparse's default logic is to attempt to parse
+        # `_sys.argv[1:]` (reference code: cpython/Lib/argparse.py) which is
+        # the arguments passed to the sdb from the shell. This is far from what
+        # we want.
+        #
+        # Solution 2 is dangerous as default arguments in Python are mutable(!)
+        # and thus invoking a Command with arguments that doesn't specify the
+        # __init__() method can pass its arguments to a similar Command later
+        # in the pipeline even if the latter Command didn't specify any args.
+        # [docs.python-guide.org/writing/gotchas/#mutable-default-arguments]
+        #
+        # We still want to set self.args to an argparse.Namespace() with the
+        # fields specific to our self.parser, thus we are forced to call
+        # parse_args([]) for it, even if `args` is None. This way commands
+        # using arguments can always do self.args.<expected field> without
+        # having to check whether this field exist every time.
+        #
+        if args is None:
+            args = []
+        self.args = self.parser.parse_args(args)
 
     def __init_subclass__(cls, **kwargs: Any) -> None:
         """
@@ -365,7 +399,9 @@ def _init_parser(cls, name: str) -> argparse.ArgumentParser:
         parser.add_argument("type", nargs=argparse.REMAINDER)
         return parser
 
-    def __init__(self, args: str = "", name: str = "_") -> None:
+    def __init__(self,
+                 args: Optional[List[str]] = None,
+                 name: str = "_") -> None:
         super().__init__(args, name)
         if not self.args.type:
             self.parser.error("the following arguments are required: <type>")

sdb/commands/container_of.py

@@ -40,7 +40,7 @@ class ContainerOf(sdb.Command):
 
             sdb> addr init_task | cast void *
             (void *)0xffffffffa8217740
-            sdb> addr init_task | member comm | addr | container_of struct task_struct comm  | cast void *
+            sdb> addr init_task | member comm | addr | container_of task_struct comm  | cast void *
             (void *)0xffffffffa8217740
 
     """

sdb/commands/filter.py

@@ -17,7 +17,7 @@
 # pylint: disable=missing-docstring
 
 import argparse
-from typing import Iterable
+from typing import Iterable, List, Optional
 
 import drgn
 import sdb
@@ -30,19 +30,19 @@ class Filter(sdb.SingleInputCommand):
     EXAMPLES
         Print addresses greater than or equal to 4
 
-            sdb> addr 0 1 2 3 4 5 6 | filter obj >= 4
+            sdb> addr 0 1 2 3 4 5 6 | filter "obj >= 4"
             (void *)0x4
             (void *)0x5
             (void *)0x6
 
         Find the SPA object of the ZFS pool named "jax" and print its 'spa_name'
 
-            sdb> spa | filter obj.spa_name == "jax" | member spa_name
+            sdb> spa | filter 'obj.spa_name == "jax"' | member spa_name
             (char [256])"jax"
 
         Print the number of level 3 log statements in the kernel log buffer
 
-            sdb> dmesg | filter obj.level == 3 | count
+            sdb> dmesg | filter 'obj.level == 3' | count
             (unsigned long long)24
     """
     # pylint: disable=eval-used
@@ -52,19 +52,24 @@ class Filter(sdb.SingleInputCommand):
     @classmethod
     def _init_parser(cls, name: str) -> argparse.ArgumentParser:
         parser = super()._init_parser(name)
-        parser.add_argument("expr", nargs=argparse.REMAINDER)
+        parser.add_argument("expr", nargs=1)
         return parser
 
-    def __init__(self, args: str = "", name: str = "_") -> None:
+    @staticmethod
+    def _parse_expression(input_expr: str) -> List[str]:
+        pass
+
+    def __init__(self,
+                 args: Optional[List[str]] = None,
+                 name: str = "_") -> None:
         super().__init__(args, name)
-        if not self.args.expr:
-            self.parser.error("the following arguments are required: expr")
+        self.expr = self.args.expr[0].split()
 
         index = None
         operators = ["==", "!=", ">", "<", ">=", "<="]
         for operator in operators:
             try:
-                index = self.args.expr.index(operator)
+                index = self.expr.index(operator)
                 # Use the first comparison operator we find.
                 break
             except ValueError:
@@ -83,22 +88,22 @@ def __init__(self, args: str = "", name: str = "_") -> None:
             raise sdb.CommandInvalidInputError(
                 self.name, "left hand side of expression is missing")
 
-        if index == len(self.args.expr) - 1:
+        if index == len(self.expr) - 1:
             # If the index is found to be at the very end of the list,
             # this means there's no right hand side of the comparison to
             # compare the left hand side to. This is an error.
             raise sdb.CommandInvalidInputError(
                 self.name, "right hand side of expression is missing")
 
         try:
-            self.lhs_code = compile(" ".join(self.args.expr[:index]),
-                                    "<string>", "eval")
-            self.rhs_code = compile(" ".join(self.args.expr[index + 1:]),
-                                    "<string>", "eval")
+            self.lhs_code = compile(" ".join(self.expr[:index]), "<string>",
+                                    "eval")
+            self.rhs_code = compile(" ".join(self.expr[index + 1:]), "<string>",
+                                    "eval")
         except SyntaxError as err:
             raise sdb.CommandEvalSyntaxError(self.name, err)
 
-        self.compare = self.args.expr[index]
+        self.compare = self.expr[index]
 
     def _call_one(self, obj: drgn.Object) -> Iterable[drgn.Object]:
         try:

sdb/commands/internal/util.py

@@ -27,11 +27,8 @@ def get_valid_type_by_name(cmd: sdb.Command, tname: str) -> drgn.Type:
     corresponding drgn.Type object.
 
     This function is used primarily by commands that accept a type
-    name as an argument and exists mainly for 2 reasons:
-    [1] There is a limitation in the way the SDB lexer interacts with
-        argparse making it hard for us to parse type names more than
-        1 token wide (e.g. 'struct task_struct'). [bad reason]
-    [2] We save some typing for the user. [good reason]
+    name as an argument and exist only to save keystrokes for the
+    user.
     """
     if tname in ['struct', 'enum', 'union', 'class']:
         #
@@ -43,8 +40,9 @@ def get_valid_type_by_name(cmd: sdb.Command, tname: str) -> drgn.Type:
         # user-friendly and thus we just avoid that situation
         # by instructing the user to skip such keywords.
         #
-        raise sdb.CommandError(cmd.name,
-                               f"skip keyword '{tname}' and try again")
+        raise sdb.CommandError(
+            cmd.name,
+            f"skip keyword '{tname}' or quote your type \"{tname} <typename>\"")
 
     try:
         type_ = sdb.get_type(tname)

sdb/commands/linux/per_cpu.py

@@ -17,7 +17,7 @@
 # pylint: disable=missing-docstring
 
 import argparse
-from typing import Iterable
+from typing import Iterable, List, Optional
 
 import drgn
 import drgn.helpers.linux.cpumask as drgn_cpumask
@@ -51,7 +51,9 @@ def _init_parser(cls, name: str) -> argparse.ArgumentParser:
         parser.add_argument("cpus", nargs="*", type=int)
         return parser
 
-    def __init__(self, args: str = "", name: str = "_") -> None:
+    def __init__(self,
+                 args: Optional[List[str]] = None,
+                 name: str = "_") -> None:
         super().__init__(args, name)
         self.ncpus = len(
             list(drgn_cpumask.for_each_possible_cpu(sdb.get_prog())))

sdb/commands/linux/slabs.py

@@ -148,14 +148,6 @@ def no_input(self) -> Iterable[drgn.Object]:
     def __pp_parse_args(self) -> Tuple[str, List[str], Dict[str, Any]]:
         fields = self.DEFAULT_FIELDS
         if self.args.o:
-            #
-            # HACK: Until we have a proper lexer for SDB we can
-            #       only pass the comma-separated list as a
-            #       string (e.g. quoted). Until this is fixed
-            #       we make sure to unquote such strings.
-            #
-            if self.args.o[0] == '"' and self.args.o[-1] == '"':
-                self.args.o = self.args.o[1:-1]
             fields = self.args.o.split(",")
         elif self.args.v:
             fields = list(Slabs.FIELDS.keys())

sdb/commands/pyfilter.py

@@ -17,7 +17,7 @@
 # pylint: disable=missing-docstring
 
 import argparse
-from typing import Iterable
+from typing import Iterable, List, Optional
 
 import drgn
 import sdb
@@ -33,7 +33,9 @@ def _init_parser(cls, name: str) -> argparse.ArgumentParser:
         parser.add_argument("expr", nargs=argparse.REMAINDER)
         return parser
 
-    def __init__(self, args: str = "", name: str = "_") -> None:
+    def __init__(self,
+                 args: Optional[List[str]] = None,
+                 name: str = "_") -> None:
         super().__init__(args, name)
         if not self.args.expr:
             self.parser.error("the following arguments are required: expr")

sdb/commands/spl/spl_kmem_caches.py

@@ -148,14 +148,6 @@ def no_input(self) -> Iterable[drgn.Object]:
     def __pp_parse_args(self) -> Tuple[str, List[str], Dict[str, Any]]:
         fields = SplKmemCaches.DEFAULT_FIELDS
         if self.args.o:
-            #
-            # HACK: Until we have a proper lexer for SDB we can
-            #       only pass the comma-separated list as a
-            #       string (e.g. quoted). Until this is fixed
-            #       we make sure to unquote such strings.
-            #
-            if self.args.o[0] == '"' and self.args.o[-1] == '"':
-                self.args.o = self.args.o[1:-1]
             fields = self.args.o.split(",")
         elif self.args.v:
             fields = list(SplKmemCaches.FIELDS.keys())

sdb/commands/stacks.py

@@ -17,7 +17,7 @@
 # pylint: disable=missing-docstring
 
 import argparse
-from typing import Dict, Iterable, List, Tuple
+from typing import Dict, Iterable, List, Optional, Tuple
 from collections import defaultdict
 
 import drgn
@@ -143,7 +143,9 @@ class Stacks(sdb.Locator, sdb.PrettyPrinter):
     input_type = "struct task_struct *"
     output_type = "struct task_struct *"
 
-    def __init__(self, args: str = "", name: str = "_") -> None:
+    def __init__(self,
+                 args: Optional[List[str]] = None,
+                 name: str = "_") -> None:
         super().__init__(args, name)
         self.mod_start, self.mod_end = 0, 0
         self.func_start, self.func_end = 0, 0