Skip to content

libinit: Implement ErrNoModulesFound#2501

Merged
probot-auto-merge[bot] merged 2 commits into
u-root:mainfrom
Foxboron:morten/moduleload-er
Nov 5, 2022
Merged

libinit: Implement ErrNoModulesFound#2501
probot-auto-merge[bot] merged 2 commits into
u-root:mainfrom
Foxboron:morten/moduleload-er

Conversation

@Foxboron
Copy link
Copy Markdown
Member

While rewriting stuff in #2482 I apparently overlooked the fact that we'd abort if any the functions fail. InstallModulesFromDir is always going to return an error unless there is /lib/modules/*.ko which means we won't be looking at the other places.

A quickfix is to just never return an error on module loading. Another option would be to make the error from InstallModulesFromDir more granular so we can differentiate between "we found no files in this directory" and "we failed to load modules from this directory".

I'm unsure what people would prefer?

@codecov
Copy link
Copy Markdown

codecov Bot commented Sep 23, 2022

Codecov Report

Base: 73.66% // Head: 73.83% // Increases project coverage by +0.16% 🎉

Coverage data is based on head (f7625bd) compared to base (35172fd).
Patch coverage: 0.00% of modified lines in pull request are covered.

❗ Current head f7625bd differs from pull request most recent head 2ed5a30. Consider uploading reports for the commit 2ed5a30 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2501      +/-   ##
==========================================
+ Coverage   73.66%   73.83%   +0.16%     
==========================================
  Files         405      405              
  Lines       41234    41228       -6     
==========================================
+ Hits        30377    30441      +64     
+ Misses      10857    10787      -70     
Impacted Files Coverage Δ
pkg/libinit/root_linux.go 27.21% <0.00%> (+0.18%) ⬆️
pkg/boot/syslinux/syslinux.go 93.36% <0.00%> (-0.07%) ⬇️
pkg/termios/var_linux.go 100.00% <0.00%> (ø)
pkg/acpi/rsdp.go 51.28% <0.00%> (+10.25%) ⬆️
pkg/acpi/raw.go 89.28% <0.00%> (+14.28%) ⬆️
pkg/acpi/bios.go 83.78% <0.00%> (+83.78%) ⬆️
pkg/acpi/sdt.go 86.66% <0.00%> (+86.66%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

10000TB
10000TB previously approved these changes Sep 23, 2022
@10000TB
Copy link
Copy Markdown
Member

10000TB commented Sep 23, 2022

I think you could log errors in a more readable way,

e.g. so if modules files are not present, it proceeds to find conf,
etc, and may eventually successfully load some modules.

In that case, you still print an error from not finding ko files.

Comment thread pkg/libinit/root_linux.go Outdated
@rminnich
Copy link
Copy Markdown
Member

rminnich commented Oct 8, 2022

Things are about to get better in error handling.
golang/go#53435

Lack of modules in a given directory should be distinguishable from a
error loading a module. Ignore such errors and continue to the other
module loaders.

Signed-off-by: Morten Linderud <morten.linderud@mullvad.net>
@Foxboron Foxboron force-pushed the morten/moduleload-er branch from f7625bd to 72f48de Compare November 3, 2022 11:15
@Foxboron Foxboron changed the title libinit: log errors instead of hard aborting module loading libinit: Implement ErrNoModulesFound Nov 3, 2022
@Foxboron
Copy link
Copy Markdown
Member Author

Foxboron commented Nov 3, 2022

Changed the implementation to implement ErrNoModulesFound. This also removes the annoying startup message :)

@rminnich rminnich added automerge Applying this label auto-merges the PR when ready Awaiting author Waiting for new changes or feedback for author. labels Nov 5, 2022
@probot-auto-merge probot-auto-merge Bot merged commit 9d33d9b into u-root:main Nov 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

automerge Applying this label auto-merges the PR when ready Awaiting author Waiting for new changes or feedback for author.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants