Skip to content
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

Fix %post and %postun generation in kmodtool #8866

Merged
merged 1 commit into from
Jun 10, 2019

Conversation

stormi
Copy link
Contributor

@stormi stormi commented Jun 7, 2019

During zfs-kmod RPM build, $(uname -r) gets unintentionally evaluated on
the build host, once and for all. It should be evaluated during the
execution of the scriptlets on the installation host. Escaping the $
character avoids evaluating it during build.

Signed-off-by: Samuel Verschelde stormi-xcp@ylix.fr

Motivation and Context

I'm maintaining the zfs packages for XCP-ng, a free virtualization solution forked from XenServer (https://xcp-ng.org/). I noticed that depmod -a was not executed after the module RPM was installed and this led me to discovering this bug in kmodtool as shipped by the project with the source RPMs.

The %post and %postun scriptlets generated by kmodtool are broken: either the build host has the same running kernel as the target kernel and then depmod -a will be executed each time a zfs-kmod-xxx package is installed, or the version is different (as in my example below) and depmod -a is never executed because the test always evaluates to false.

Description

Escape $(uname -r) so that it's not evaluated on the build host.

How Has This Been Tested?

RPM built in a docker container then checked with rpm -qp nameoffile --scripts and installed on an XCP-ng host (centos based with custom kernel). modprobe zfs worked without the need to run depmod -a manually whereas it didn't before the fix on similar hosts.

Output of rpm -qp nameoffile --scripts before the change:

$ rpm -qp kmod-zfs-4.19.0+1-0.8.0-1.xcpng8.0.x86_64.rpm --scripts                                                                 
postinstall scriptlet (using /bin/sh):
[[ "3.10.0-957.12.1.el7.x86_64" == "4.19.0+1"  ]] && /sbin/depmod -a > /dev/null || :
postuninstall scriptlet (using /bin/sh):
[[ "3.10.0-957.12.1.el7.x86_64" == "4.19.0+1"  ]] && /sbin/depmod -a > /dev/null || :

Output after the fix:

$ rpm -qp --scripts RPMS/x86_64/kmod-zfs-4.19.0+1-0.8.0-1.1.xcpng8.0.x86_64.rpm
postinstall scriptlet (using /bin/sh):
[[ "$(uname -r)" == "4.19.0+1"  ]] && /sbin/depmod -a > /dev/null || :
postuninstall scriptlet (using /bin/sh):
[[ "$(uname -r)" == "4.19.0+1"  ]] && /sbin/depmod -a > /dev/null || :

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation (a change to man pages or other documentation)

Checklist:

During zfs-kmod RPM build, $(uname -r) gets unintentionally evaluated on
the build host, once and for all. It should be evaluated during the
execution of the scriptlets on the installation host. Escaping the $
character avoids evaluating it during build.

Signed-off-by: Samuel Verschelde <stormi-xcp@ylix.fr>
stormi added a commit to xcp-ng-rpms/zfs-kmod that referenced this pull request Jun 7, 2019
Escape $(uname -r) because we don't want it to run on the build host. It
needs to run when the scriptlet itself is executed.

Also contributed upstream: openzfs/zfs#8866
@behlendorf behlendorf added Status: Code Review Needed Ready for review and testing Component: Packaging custom packages labels Jun 7, 2019
Copy link
Contributor

@behlendorf behlendorf left a comment

Choose a reason for hiding this comment

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

Thanks! Looks good, I also verified that this same fix was applied to the upstream kmodtool.

Copy link
Contributor

@ofaaland ofaaland left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Jun 7, 2019
@codecov
Copy link

codecov bot commented Jun 7, 2019

Codecov Report

Merging #8866 into master will increase coverage by 0.08%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8866      +/-   ##
==========================================
+ Coverage   78.65%   78.73%   +0.08%     
==========================================
  Files         382      382              
  Lines      117778   117778              
==========================================
+ Hits        92638    92735      +97     
+ Misses      25140    25043      -97
Flag Coverage Δ
#kernel 79.32% <ø> (-0.04%) ⬇️
#user 67.46% <ø> (+0.25%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 893a6d6...6ac7c90. Read the comment docs.

@behlendorf behlendorf merged commit 01d1e88 into openzfs:master Jun 10, 2019
behlendorf pushed a commit that referenced this pull request Jun 11, 2019
During zfs-kmod RPM build, $(uname -r) gets unintentionally evaluated on
the build host, once and for all. It should be evaluated during the
execution of the scriptlets on the installation host. Escaping the $
character avoids evaluating it during build.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Olaf Faaland <faaland1@llnl.gov>
Reviewed-by: Neal Gompa <ngompa@datto.com>
Signed-off-by: Samuel Verschelde <stormi-xcp@ylix.fr>
Closes #8866
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Packaging custom packages Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants