Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.34 as permitted sender) client-ip=23.128.96.34;
Message-ID: <c44b748307a074d0c250002cdcfe209b8cce93c9.camel@sipsolutions.net>
Subject: Re: [PATCH v2 1/8] scripts/gdb/symbols: add specific ko module load
 command
From:   Johannes Berg <johannes@sipsolutions.net>
To:     Kuan-Ying Lee <Kuan-Ying.Lee@mediatek.com>,
        Jan Kiszka <jan.kiszka@siemens.com>,
        Kieran Bingham <kbingham@kernel.org>,
        Matthias Brugger <matthias.bgg@gmail.com>,
        AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@collabora.com>
Cc:     chinwen.chang@mediatek.com, qun-wei.lin@mediatek.com,
        linux-mm@kvack.org, linux-modules@vger.kernel.org,
        casper.li@mediatek.com, akpm@linux-foundation.org,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        linux-mediatek@lists.infradead.org
Date:   Tue, 12 Sep 2023 11:41:29 +0200
In-Reply-To: <20230808083020.22254-2-Kuan-Ying.Lee@mediatek.com>
References: <20230808083020.22254-1-Kuan-Ying.Lee@mediatek.com>
         <20230808083020.22254-2-Kuan-Ying.Lee@mediatek.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.48.4 (3.48.4-1.fc38) 
MIME-Version: 1.0
Precedence: bulk

On Tue, 2023-08-08 at 16:30 +0800, Kuan-Ying Lee wrote:
> Add lx-symbols <ko_path> command to support add specific
> ko module.

I'm not sure how this was supposed to work? It should have updated the
documentation, but more importantly, it shouldn't have broken the
documented usage of this command:

      The kernel (vmlinux) is taken from the current working directly. Modu=
les (.ko)
      are scanned recursively, starting in the same directory. Optionally, =
the module
      search path can be extended by a space separated list of paths passed=
 to the
      lx-symbols command.

Note how that talks about a "space separated list of paths" for which
clearly a single path seems like it should be accepted?

> @@ -138,6 +139,19 @@ lx-symbols command."""
>          else:
>              gdb.write("no module object found for '{0}'\n".format(module=
_name))
> =20
> +    def load_ko_symbols(self, mod_path):
> +        self.loaded_modules =3D []
> +        module_list =3D modules.module_list()
> +
> +        for module in module_list:
> +            module_name =3D module['name'].string()
> +            module_pattern =3D ".*/{0}\.ko(?:.debug)?$".format(
> +                module_name.replace("_", r"[_\-]"))
> +            if re.match(module_pattern, mod_path) and os.path.exists(mod=
_path):
> +                self.load_module_symbols(module, mod_path)
> +                return
> +        raise gdb.GdbError("%s is not a valid .ko\n" % mod_path)
> +
>      def load_all_symbols(self):
>          gdb.write("loading vmlinux\n")
> =20
> @@ -176,6 +190,11 @@ lx-symbols command."""
>          self.module_files =3D []
>          self.module_files_updated =3D False
> =20
> +        argv =3D gdb.string_to_argv(arg)
> +        if len(argv) =3D=3D 1:
> +            self.load_ko_symbols(argv[0])
> +            return

But this obviously breaks it, since passing a single path will go into
the if, then complain "some/folder/ is not a valid .ko" and exit.


But I'm not even sure how you intended this to work _at all_, because in
the context before this if, we have:

         self.module_paths =3D [os.path.abspath(os.path.expanduser(p))
                              for p in arg.split()]
         self.module_paths.append(os.getcwd())

so you first add the (file!) to the list of paths, and then try to load
the file by finding modules in the paths, and filtering by the specified
file? That seems ... very roundabout, and can even only work if the file
can be found via os.getcwd(), so you could never specify an absolute
filename?

All that seems counter to what the patch was meant to do.

I suspect that really you need to individually check "is it a file or a
directory" before handling any of this, and if it's actually a file,
don't use it as a _filter_ as you do in load_ko_symbols() now but
directly use it as is with load_module_symbols()?


@Jan, can we revert this? I came up with the following trivial fix that
makes it at least not break the original use case of passing a single-
entry directory list, but it really doesn't seem right one way or the
other.


--- a/scripts/gdb/linux/symbols.py
+++ b/scripts/gdb/linux/symbols.py
@@ -191,7 +191,7 @@ lx-symbols command."""
         self.module_files_updated =3D False
=20
         argv =3D gdb.string_to_argv(arg)
-        if len(argv) =3D=3D 1:
+        if len(argv) =3D=3D 1 and os.path.isfile(argv[0]):
             self.load_ko_symbols(argv[0])
             return
=20


johannes