dkms icon indicating copy to clipboard operation
dkms copied to clipboard

common.postinst: binary-only ldtarball fails because load_tarball returns non-zero status

Open LuKePicci opened this issue 2 years ago • 6 comments

https://github.com/dell/dkms/blob/749b219744ccb3bbc6219aa48db653bbc9ebbebb/dkms_common.postinst#L141

The line above expects ldtarball to exit with status 0 on success but 1 is returned. I have a binary-only tarball made by mktarball which loads and works just fine but I can't make an rpm out of it using common.postinst because it seems that load_tarball function returns non-zero status even if tarball is loaded successfully.

If the exit status of 1 is correct then the above line should be changed in order to accept that situation without failing.

LuKePicci avatar Nov 08 '23 10:11 LuKePicci

Abstracting ourselves from all the code and implementation details, can you outline a simple reproducer - the more noob friendly the better - the expected behaviour and the actual result you're seeing.

Thanks.

evelikov avatar Nov 08 '23 13:11 evelikov

Sure, you're welcome.

We have a dkms driver which works fine compiling from sources (fedora 38, x86_64). We would like to avoid the compilation step on a specific device with fixed kernel and distro (fedora 38 as well, x86_64).

We used dkms mktarball with binaries only options to create a prebuilt tar.gz which works fine if we load it using dkms ldtarball and then dkms install -m Mname -v Mversion.

That said, we are now working on packaging such prebuilt driver as RPM package for which we have the following command in the %posttrans hook

%posttrans
/usr/lib/dkms/common.postinst %{module} %{version} $RPM_BUILD_ROOT/usr/modules %{buildarch}

According to the code of common.postinst, when $3 is defined (TARBALL_ROOT, valued /usr/modules at runtime) it is expected to load the driver from a prebuilt tarball with specific file name %{module}-%{version}.dkms.tar.gz and return success status to the rpm package installer.

However, the rpm install fails because common.postinst exits with error from the above line of code. During install, we see the ldtarball command is being executed with right arguments and is printing out normal informational messages to the console but its final exit status makes the if statement at that sae line to take the main branch and thus entering the error condition returned to the rpm package installer.

To reproduce, load and build any dkms module, use mktarball with binaries-only option to create a prebuilt tarball, remove the module from dkms system and then try using the above %posttrans command to install it again. Use echo $? to check the exit status code, you will see it is not 0, as the common.postinst expects.

LuKePicci avatar Nov 08 '23 15:11 LuKePicci

If you have done the analysis and can open a MR that would be great. I would request that it includes a test case so we don't regress things.

Alternatively a step-by-step reproducer, as requested earlier is warranted.

evelikov avatar Nov 09 '23 18:11 evelikov

Hint: You could use the minimal kernel module from test/dkms_test-1.0/ for the step-by-step reproducer.

anbe42 avatar Nov 24 '23 18:11 anbe42

Great I'll try to make repro steps then. I can also make a PR for the fix but before making a blind fix I would need to ask whoever authored the load_tarball function if it is expected for that function to return 1 in similar invocations. Then I would patch load_tarball rather then common.postinst

LuKePicci avatar Nov 24 '23 19:11 LuKePicci

If you are going to make a PR, you should probably start with adding some tests for mktarball and ldtarball to run_tests.sh. These should cover at least the three use cases source/binary/both. For a start it should be sufficient to look at the "success" cases (whether or not they return 0), tests covering all the possible error paths can come later.

From a sanely behaving mktarball and ldtarball subcommand I'd expect a return value of 0 on success and !0 otherwise. I'm not sure which error case it tries to signal currently with the return of 1.

(Using sane return values is not something dkms did in the past. But it is getting better.)

anbe42 avatar Nov 24 '23 20:11 anbe42