Re: [PATCH 1/1] kernel-module-split: fix conf file generation when KERNEL_SPLIT_MODULES=0

["Dixit Parmar" <dixitparmar19@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 12991 bytes --]

Hi Bruce, Thank you for your constructive feedback. Please find my inline replies.
FYI, I had submitted this v2 change before your comment on the previous version, where you suggested to wait :| Thank you for considering this.

On Fri, Mar 21, 2025 at 12:52 AM, Bruce Ashfield wrote:

> 
> In message: [OE-core] [PATCH 1/1] kernel-module-split: fix conf file
> generation when KERNEL_SPLIT_MODULES=0
> on 16/03/2025 Dixit Parmar via lists.openembedded.org wrote:
> 
> 
>> When KERNEL_SPLIT_MODULES=0 modprobe and autoload conf files are not
>> getting
>> generated for the kernel modules.
>> 
>> Separated out conf file handling mechanism as handle_conf_files() function
>> 
>> from hook frob_metadata() function. handle_conf_files() gets re-used from
>> the
>> existing hook function and add_conf_files function which got introduced
>> for
>> splitmods=0 flow in this patch.
> 
> Trevor's advice on the commit message was excellent, I won't repeat
> it here. I'll just simply add that the commit message should give us
> all the information we need about the issue (and what its symptoms
> are) and how this fixes it (not explaining the code, but describing
> the strategy)

Understood. I will update the commit message with all these details.

> 
> 
>> [YOCTO #15145]
>> 
>> Signed-off-by: Dixit Parmar <dixitparmar19@gmail.com>
>> Cc: George Thopas <george.thopas@gmail.com>
>> --
>> Can you show the conf files for the same kernel configuration
>> with and without the kernel_split_modules enabled ? That way
>> we know there's no change in existing behaviour.
>> 
>>> I have confirmed that in my testing. Can you suggest how I can
>> 
>> share that information here?
> 
> A good way would simply to put it in the commit message that you've
> built and booted with both modes and that the generated module
> conf files are the same in both cases. Showing a diff command
> that you ran, or how you determined they were the same would be enough
> as well.

Agreed.

> 
> 
>> We also should make a test for this in the OE selftests. We
>> are adding conditional code paths, so they should be tested
>> to ensure no regressions in either in the future.
>> 
>>> Never done that before. May be I can do it given some direction
>> 
>> as separate patch.
> 
> Any selftests should be in a separate patch, but they do need
> to come along with any new verions of this. Saying that "it will
> be send later" won't get the patches merged. Since everyone gets
> busy and these sorts of things tend to not happen.
> 
> If you search on the mailing list(s) you'll find examples of
> other self tests being submitted. Just follow what they are
> doing. I'd suggest that a self test needs to run both modes
> and do a check that the generated .conf files are correct
> (basically what you'd be showing in the commit message). Even
> better if a boot and showing that they are (auto)loaded.
> 
> That's the only way we can be sure this won't break in the
> future and be comfortable with the complexty being added.

Agree. I will look at the self-test implementations and develop the test for this too.

> 
> 
>> I'm curious about the above line. It is unclear to me why we'd
>> only have this postinst be relevant if none was previously set.
>> 
>>> Reverted.
>> 
>> Is there really a scenario where the directory won't exist ? Isn't
>> this just running in our own install phase ? So all prerequisites
>> and directories should be in place.
>> 
>>> Ideally no, we kept it for safer side, I have added log warning.
>> 
>> 
> 
> As Trevor mentioned in his reply, it is best to configure your
> mail client to reply inline. I had made that comment near the
> line of code in question, but with the time between my review,
> your reply and now this reply .. the context of what was being
> discussed is very difficult to remember.
> 
> If that directory doesn't exist, just error out and do not
> continue. This isn't something recoverable, and the build should
> be stopped.

Okay. Will handle it that way.

> 
> 
>> The walking and sorting seems quite heavy. Isn't this called from
>> do_split_packages indirectly ?
>> Do we really need to walk and gather the information ? Is this mainly for
>> the case of no-split
>> on the kernel modules ? If that is the case, isn't there a way to short
>> circuit the processing
>> on the split-package case ?
>> 
>>> Litterally I could not think of anything else here and not sure of there
>> 
>> are any short-circuit options. I have limited knowledge in this. I am open
>> to suggestions
>> if this is not the best solution at the moment.
> 
> Describe the approach in your commit message, this is the type of
> information that we need. To be sure that it isn't adding a lot of
> time to the build, add some timing information with and without
> your changes. This is the type of information we are looking for
> in a review.

Understood.

> 
> See a few more comments below.
> 
> 
>> ---
>> .../kernel-module-split.bbclass | 81 +++++++++++++++----
>> 1 file changed, 67 insertions(+), 14 deletions(-)
>> 
>> diff --git a/meta/classes-recipe/kernel-module-split.bbclass
>> b/meta/classes-recipe/kernel-module-split.bbclass
>> index 9487365eb7..b7ee3a8f9e 100644
>> --- a/meta/classes-recipe/kernel-module-split.bbclass
>> +++ b/meta/classes-recipe/kernel-module-split.bbclass
>> @@ -86,11 +86,7 @@ python split_kernel_module_packages () {
>> vals[m.group(1)] = m.group(2)
>> return vals
>> 
>> - def frob_metadata(file, pkg, pattern, format, basename):
>> - vals = extract_modinfo(file)
>> -
>> - dvar = d.getVar('PKGD')
>> -
>> + def handle_conf_files(d, basename, pkg):
>> # If autoloading is requested, output ${modulesloaddir}/<name>.conf and
>> append
>> # appropriate modprobe commands to the postinst
>> autoloadlist = (d.getVar("KERNEL_MODULE_AUTOLOAD") or "").split()
>> @@ -101,7 +97,7 @@ python split_kernel_module_packages () {
>> bb.warn("module_autoload_%s is defined but '%s' isn't included in
>> KERNEL_MODULE_AUTOLOAD, please add it there" % (basename, basename))
>> if basename in autoloadlist:
>> conf = '%s/%s.conf' % (d.getVar('modulesloaddir'), basename)
>> - name = '%s%s' % (dvar, conf)
>> + name = '%s%s' % (d.getVar('PKGD'), conf)
>> os.makedirs(os.path.dirname(name), exist_ok=True)
>> with open(name, 'w') as f:
>> if autoload:
>> @@ -123,7 +119,7 @@ python split_kernel_module_packages () {
>> modconf = d.getVar('module_conf_%s' % basename)
>> if modconf and basename in modconflist:
>> conf = '%s/%s.conf' % (d.getVar('modprobedir'), basename)
>> - name = '%s%s' % (dvar, conf)
>> + name = '%s%s' % (d.getVar('PKGD'), conf)
>> os.makedirs(os.path.dirname(name), exist_ok=True)
>> with open(name, 'w') as f:
>> f.write("%s\n" % modconf)
>> @@ -134,6 +130,63 @@ python split_kernel_module_packages () {
>> elif modconf:
>> bb.error("Please ensure module %s is listed in KERNEL_MODULE_PROBECONF
>> since module_conf_%s is set" % (basename, basename))
>> 
>> + def add_conf_files(d, root, file_regex, output_pattern):
>> + """
>> + Arguments:
>> + root -- the path in which to search
>> + file_regex -- regular expression to match searched files. Use
>> + parentheses () to mark the part of this expression
>> + that should be used to derive the module name (to be
>> + substituted where %s is used in other function
>> + arguments as noted below)
>> + output_pattern -- pattern to use for the package names. Must include %s.
>> 
>> + """
>> +
>> + dvar = d.getVar('PKGD')
>> + root = d.expand(root)
> 
> root is a passed parameter, you shouldn't need to call d.expand()
> on it. Are you expecting there to be something that can be expanded ?

> 
> 
>> + output_pattern = d.expand(output_pattern)
> 
> Likewise with output_pattern

Correct. It's not needed as I don't expect it to be expanded. I missed it.

> 
> 
>> +
>> + # check if the root directory doesn't exist for safe side, don't error
>> out later but silently do
>> + # no splitting.
>> + if not os.path.exists(dvar + root):
>> + bb.warn("kernel module root directory path does not exist")
>> + return []
> 
> as I mentioned above, I see this as a fatal error. Something has gone
> terribly wrong if this doesn't exist and we should error and exit.
> 
> 
>> +
>> + # get list of modules
>> + objs = []
>> + for walkroot, dirs, files in os.walk(dvar + root):
>> + for file in files:
>> + relpath = os.path.join(walkroot, file).replace(dvar + root + '/', '', 1)
>> 
>> + if relpath:
>> + objs.append(relpath)
> 
> The above block of code needs a comment. What exactly is it doing ?
> My quick read of it makes me think it is checking our staged / rootfs /
> sysroot
> for the modules, and then removing that part of the path so we'll
> presumably
> have the path as they will appear on the target when it boots ?
> 
> We need explanations in the code about the logic, otherwise, it is going
> to be unmaintainable.

Will add a comment.

> 
> 
>> +
>> + for o in sorted(objs):
>> + import re, stat
>> + if False:
>> + m = re.match(file_regex, o)
>> + else:
>> + m = re.match(file_regex, os.path.basename(o))
>> +
>> + if not m:
>> + continue
> 
> Why isn't this test just done when walking the module files ? I don't
> see the point in walking once, and not testing to see if they are
> modules during that walk.

Probably, I had planned to optimize it later and then missed it. will revise this.

> 
> 
>> +
>> + file = os.path.join(dvar + root, o)
>> + mode = os.lstat(file).st_mode
>> + if not (stat.S_ISREG(mode) or (allow_links and stat.S_ISLNK(mode)) or
>> (allow_dirs and stat.S_ISDIR(mode))):
>> + continue
>> +
>> + on = legitimize_package_name(m.group(1))
>> + pkg = output_pattern % on
>> +
>> + basename = m.group(1)
>> + handle_conf_files(d, basename, pkg)
>> +
>> + def frob_metadata(file, pkg, pattern, format, basename):
>> + vals = extract_modinfo(file)
>> + dvar = d.getVar('PKGD')
>> +
>> + handle_conf_files(d, basename, pkg)
>> +
>> if "description" in vals:
>> old_desc = d.getVar('DESCRIPTION:' + pkg) or ""
>> d.setVar('DESCRIPTION:' + pkg, old_desc + "; " + vals["description"])
>> @@ -167,19 +220,19 @@ python split_kernel_module_packages () {
>> postinst = d.getVar('pkg_postinst:modules')
>> postrm = d.getVar('pkg_postrm:modules')
>> 
>> + module_regex = r'^(.*)\.k?o(?:\.(gz|xz|zst))?$'
>> + module_pattern_prefix = d.getVar('KERNEL_MODULE_PACKAGE_PREFIX')
>> + module_pattern_suffix = d.getVar('KERNEL_MODULE_PACKAGE_SUFFIX')
>> + module_pattern = module_pattern_prefix + kernel_package_name +
>> '-module-%s' + module_pattern_suffix
>> +
>> if splitmods != '1':
>> d.appendVar('FILES:' + metapkg, '%s %s %s/modules' %
>> (d.getVar('modulesloaddir'), d.getVar('modprobedir'),
>> d.getVar("nonarch_base_libdir")))
>> d.appendVar('pkg_postinst:%s' % metapkg, postinst)
>> - d.prependVar('pkg_postrm:%s' % metapkg, postrm);
>> + d.prependVar('pkg_postrm:%s' % metapkg, postrm)
> 
> Why did you remove the semicolon above ? I admit it is strange to see
> it in the python routine, but it doesn't look related to your changes.

I thought the same way, I found it unusual in python and it did not have an impact so I thought to follow the standard and do the cleanup.

> 
> 
>> + add_conf_files(d, root='${nonarch_base_libdir}/modules',
>> file_regex=module_regex, output_pattern=module_pattern)
> 
> I think this would be better called "create_conf_files" or
> "generate_conf_files".
> 
> It was the semicolon and the name of the routine that had me asking about
> the
> routine being called from do_split_packages(). But I see that it in fact
> is
> not called during package splitting, but only called once (when splitting
> is
> not enabled) .. amd I ready that correctly now ?

Correct. Here there are two branches of execution based on whether the split is enabled or not. the execution reaches at do_split_package only in case when split is enabled, otherwise, for split disabled case we will directly call the conf file API.

Thanks,
Dixit

> 
> Bruce
> 
> 
>> return
>> 
>> - module_regex = r'^(.*)\.k?o(?:\.(gz|xz|zst))?$'
>> -
>> - module_pattern_prefix = d.getVar('KERNEL_MODULE_PACKAGE_PREFIX')
>> - module_pattern_suffix = d.getVar('KERNEL_MODULE_PACKAGE_SUFFIX')
>> - module_pattern = module_pattern_prefix + kernel_package_name +
>> '-module-%s' + module_pattern_suffix
>> -
>> modules = do_split_packages(d, root='${nonarch_base_libdir}/modules',
>> file_regex=module_regex, output_pattern=module_pattern, description='%s
>> kernel module', postinst=postinst, postrm=postrm, recursive=True,
>> hook=frob_metadata, extra_depends='%s-%s' % (kernel_package_name,
>> kernel_version))
>> if modules:
>> d.appendVar('RDEPENDS:' + metapkg, ' '+' '.join(modules))
>> --
>> 2.43.0
> 
>

[-- Attachment #2: Type: text/html, Size: 13820 bytes --]