| Message ID | 20250222071113.81675-1-dixitparmar19@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | kernel-module-split: fix conf file generation when KERNEL_SPLIT_MODULES=0 | expand |
In message: [OE-core] [PATCH] kernel-module-split: fix conf file generation when KERNEL_SPLIT_MODULES=0 on 22/02/2025 Dixit Parmar via lists.openembedded.org wrote: > When KERNEL_SPLIT_MODULES=0 modprob and autload conf files are not getting s/modprob/modprobe/ s/autload/autoload/ > 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. 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. 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. > > [YOCTO #15145] > > Signed-off-by: Dixit Parmar <dixitparmar19@gmail.com> > Cc: George Thopas <george.thopas@gmail.com> > --- > .../kernel-module-split.bbclass | 82 +++++++++++++++---- > 1 file changed, 67 insertions(+), 15 deletions(-) > > diff --git a/meta/classes-recipe/kernel-module-split.bbclass b/meta/classes-recipe/kernel-module-split.bbclass > index 9487365eb7..2fb3ab967f 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: > @@ -114,7 +110,7 @@ python split_kernel_module_packages () { > d.appendVar('CONFFILES:%s' % pkg, conf2append) > postinst = d.getVar('pkg_postinst:%s' % pkg) > if not postinst: > - bb.fatal("pkg_postinst:%s not defined" % pkg) > + postinst = d.getVar('pkg_postinst:modules') 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. > postinst += d.getVar('autoload_postinst_fragment') % (autoload or basename) > d.setVar('pkg_postinst:%s' % pkg, postinst) > > @@ -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,62 @@ 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) > + output_pattern = d.expand(output_pattern) > + > + # if the root directory doesn't exist, don't error out later but silently do > + # no splitting. > + if not os.path.exists(dvar + root): > + return [] 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. > + > + # 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) > + > + 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 > + > + 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) 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 ? Bruce > + > + 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 +219,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) > + add_conf_files(d, root='${nonarch_base_libdir}/modules', file_regex=module_regex, output_pattern=module_pattern) > 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 > > > -=-=-=-=-=-=-=-=-=-=-=- > Links: You receive all messages sent to this group. > View/Reply Online (#211817): https://lists.openembedded.org/g/openembedded-core/message/211817 > Mute This Topic: https://lists.openembedded.org/mt/111322503/1050810 > Group Owner: openembedded-core+owner@lists.openembedded.org > Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub [bruce.ashfield@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? 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. 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. 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.
Submitted V2 patch: https://lists.openembedded.org/g/openembedded-core/message/212995
On Sun 2025-03-16 @ 01:16:47 AM, Dixit Parmar via lists.openembedded.org wrote: > 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? What data are you expecting to find, and where are you expecting to find it, that prompted this fix? I assume that in one case there will be information/files populated in the image's /etc/modprobe.d and in the other case (the buggy case) that isn't happening? Or is there something else? Can you provide complete instructions and/or your configuration demonstrating the problem occurring before your fix, and the problem being solved by your fix? Simply enabling KERNEL_SPLIT_MODULES by itself won't show anything unless the user is trying to enable some module(s) too. I found that I had to go read the bugzilla report to better understand the issue and potentially how to show it occurring. Keep the reference to the bugzilla, but ideally the commit message would have all the necessary information to understand and reproduce the issue. > 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. > 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. > 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. It is customary that quoted text have the start-of-line ">" symbols (the more symbols, the further back in history the comment) and that replies not have any start-of-line character, as I'm doing here. Your email program should have an option to do this automatically.
On Mon, Mar 17, 2025 at 07:54 PM, Trevor Woerner wrote: > > On Sun 2025-03-16 @ 01:16:47 AM, Dixit Parmar via lists.openembedded.org > wrote: > >> 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? > > What data are you expecting to find, and where are you expecting to find > it, that prompted this fix? I assume that in one case there will be > information/files populated in the image's /etc/modprobe.d and in the > other > case (the buggy case) that isn't happening? Or is there something else? > > Can you provide complete instructions and/or your configuration > demonstrating > the problem occurring before your fix, and the problem being solved by > your > fix? Simply enabling KERNEL_SPLIT_MODULES by itself won't show anything > unless > the user is trying to enable some module(s) too. > > I found that I had to go read the bugzilla report to better understand the > > issue and potentially how to show it occurring. Keep the reference to the > bugzilla, but ideally the commit message would have all the necessary > information to understand and reproduce the issue. Yeah. I shall add the issue reproduction configuration, and overview of the fix in the next patchset v2. Do you see any other code changes that are required? so I can take it in the next set of changes. > > >> 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. >> 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. >> >> 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. > > It is customary that quoted text have the start-of-line ">" symbols (the > more > symbols, the further back in history the comment) and that replies not > have > any start-of-line character, as I'm doing here. Your email program should > have > an option to do this automatically. As you might have rightly assumed, I am a newbie so figuring out this stuff, I hope you understand. ~Dixit
On Wed, Mar 19, 2025 at 3:40 AM Dixit Parmar via lists.openembedded.org <dixitparmar19=gmail.com@lists.openembedded.org> wrote: > On Mon, Mar 17, 2025 at 07:54 PM, Trevor Woerner wrote: > > On Sun 2025-03-16 @ 01:16:47 AM, Dixit Parmar via lists.openembedded.org > wrote: > > 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? > > What data are you expecting to find, and where are you expecting to find > it, that prompted this fix? I assume that in one case there will be > information/files populated in the image's /etc/modprobe.d and in the other > case (the buggy case) that isn't happening? Or is there something else? > > Can you provide complete instructions and/or your configuration > demonstrating > the problem occurring before your fix, and the problem being solved by your > fix? Simply enabling KERNEL_SPLIT_MODULES by itself won't show anything > unless > the user is trying to enable some module(s) too. > > I found that I had to go read the bugzilla report to better understand the > issue and potentially how to show it occurring. Keep the reference to the > bugzilla, but ideally the commit message would have all the necessary > information to understand and reproduce the issue. > > Yeah. I shall add the issue reproduction configuration, and overview of > the fix in the next patchset v2. Do you see any other code changes that are > required? so I can take it in the next set of changes. > > Hold on the next version for a day or so. I've been traveling and haven't had time to reply to your other questions, but will get time either today or tomorrow. Bruce > 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. > 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. > > 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. > > It is customary that quoted text have the start-of-line ">" symbols (the > more > symbols, the further back in history the comment) and that replies not have > any start-of-line character, as I'm doing here. Your email program should > have > an option to do this automatically. > > As you might have rightly assumed, I am a newbie so figuring out this > stuff, I hope you understand. > > ~Dixit > > -=-=-=-=-=-=-=-=-=-=-=- > Links: You receive all messages sent to this group. > View/Reply Online (#213266): > https://lists.openembedded.org/g/openembedded-core/message/213266 > Mute This Topic: https://lists.openembedded.org/mt/111322503/1050810 > Group Owner: openembedded-core+owner@lists.openembedded.org > Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub [ > bruce.ashfield@gmail.com] > -=-=-=-=-=-=-=-=-=-=-=- > >
diff --git a/meta/classes-recipe/kernel-module-split.bbclass b/meta/classes-recipe/kernel-module-split.bbclass index 9487365eb7..2fb3ab967f 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: @@ -114,7 +110,7 @@ python split_kernel_module_packages () { d.appendVar('CONFFILES:%s' % pkg, conf2append) postinst = d.getVar('pkg_postinst:%s' % pkg) if not postinst: - bb.fatal("pkg_postinst:%s not defined" % pkg) + postinst = d.getVar('pkg_postinst:modules') postinst += d.getVar('autoload_postinst_fragment') % (autoload or basename) d.setVar('pkg_postinst:%s' % pkg, postinst) @@ -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,62 @@ 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) + output_pattern = d.expand(output_pattern) + + # if the root directory doesn't exist, don't error out later but silently do + # no splitting. + if not os.path.exists(dvar + root): + return [] + + # 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) + + 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 + + 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 +219,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) + add_conf_files(d, root='${nonarch_base_libdir}/modules', file_regex=module_regex, output_pattern=module_pattern) 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))
When KERNEL_SPLIT_MODULES=0 modprob and autload 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. [YOCTO #15145] Signed-off-by: Dixit Parmar <dixitparmar19@gmail.com> Cc: George Thopas <george.thopas@gmail.com> --- .../kernel-module-split.bbclass | 82 +++++++++++++++---- 1 file changed, 67 insertions(+), 15 deletions(-)