Skip to content

virtme-run --mods=auto is not working with kernels >=6.2 #82

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

matttbe
Copy link

@matttbe matttbe commented Dec 22, 2022

virtme-prep-kdir-mods: support kernel 6.2

Thank you for having created and maintained this very handy tool!

Since the kernel commit torvalds/linux@f65a486821cf ("kbuild: change module.order to list *.o instead of *.ko") that is now in Linus tree in preparation of the future v6.2, module.order file lists .o files instead of .ko ones.

virtme-prep-kdir-mods reads module.order file but it needs to create symlinks for the .ko files, not the .o ones.

@dbrouwer3563
Copy link

@matttbe thanks for this patch - I was having the same problem today so just tested your patch and it worked perfectly. Not sure if this is best practice, but I had to manually delete the .virtme_mods folder in my main linux directory to force the modules to show up.

@matttbe
Copy link
Author

matttbe commented Jan 4, 2023

@dbrouwer3563 thank you for having validated this patch!

Not sure if this is best practice, but I had to manually delete the .virtme_mods folder in my main linux directory to force the modules to show up.

I don't remember I had to do that but when trying to understand the issue, I might have deleted the folder when it was filled in with .o files. Hopefully this fix can get merged before too many users switch to v6.2 and populate the directory with wrong files :-)
(cc @amluto)

Comment on lines 28 to 29
# from v6.2, modules.order lists .o files, we need the .ko ones
i=$(echo "$i" | sed 's:\.o$:.ko:')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of fork/exec-ing a new sed process for every line, this could be done more efficiently by streaming the whole modules.order through a single process instead, e.g.:

sed 's:\.o$:.ko:' modules.order | while read -r i; do
   # existing loop body
done

Or alternately, the string substitution could be achieved via parameter expansion instead of sed, e.g.:

if [ "${i%.o}" != "$i" ]; then
    i="${i%.o}.ko"
fi

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I initially wanted to do the conversion with ${i/%.o/.ko} but sh is used here. I then added the sed there to imitate what is done the line below, more to have something simple.

I agree it is not optimal, I didn't think it would have been more important to have it more efficient. I can change of course, thank you for the suggestion.

Since the kernel commit f65a486821cf ("kbuild: change module.order to
list *.o instead of *.ko") that is now in Linus tree in preparation of
the future v6.2, module.order file lists .o files instead of .ko ones.

virtme-prep-kdir-mods reads module.order file but it needs to create
symlinks for the .ko files, not the .o ones.

Signed-off-by: Matthieu Baerts <[email protected]>
@marcosps
Copy link
Contributor

@amluto can you please take a look at this PR? Thanks!

Copy link

@tyhicks tyhicks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes look good to me. I've also tested them and can verify that they work after blowing away any existing .virtme_mods directories that were created after kernel commit f65a486821cf.

@arighi
Copy link
Contributor

arighi commented Apr 7, 2023

This has been merged/fixed in https://github.com/arighi/virtme

NOTE: we are trying to catch up with all the pending PR in this temporary fork. In the future we may merge everything back here if this project become active again, but for now please consider to follow also the development of arighi/virtme.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants