Skip to content

Random selection of fixes and cleanups#171

Merged
scaronni merged 9 commits intodkms-project:masterfrom
evelikov:sent/misc-fixes
Nov 4, 2021
Merged

Random selection of fixes and cleanups#171
scaronni merged 9 commits intodkms-project:masterfrom
evelikov:sent/misc-fixes

Conversation

@evelikov
Copy link
Collaborator

Grouping the lot in a single MR since they fit no particular topic:

  • remove multi-action machinery - manual says one action, code "supports" multiple
  • move input arguments validation earlier
  • use local -r variables - makes the code a bit easier on the eyes
  • update tar invocation - drop cd push/pop, auto-detection magic
  • always check for write permissions in make_tarball
  • add some early return/break statements - try to tame the crazy indentation, without rewriting the world

Based on the documentation dkms can do a simple action at a time.
Although the code seems to disagree.

Considering the complexity such multi-action might involve, to work
properly, simply remove that feature.

After staring at dozens of scripts using dkms, I'm yet to see one which
use this undocumented feature.

Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
Move all but the "add_module" check_module_args() call earlier.

As seen in the removal actions - we do not need to be root, only to
check the user has provided sane input params.

The add_module remains as-is since the args are optional.

Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
Function is used in a single, place only to call a variant of
module_is_added_or_die, followed up with an essentially duplicate
"is_added" check.

Just kill it all off, adding module_is_added_or_die() into the main
switch, like many other actions.

Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
Reuse the existing maybe_{build,install}_module to simplify the command.

Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
Just use the trap and kill off the manual rm instances. We've been doing
rather poor job with them anyway.

Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
Bit of a no-brainer - use local read-only variables so the codebase is
actually readable.

Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
Tar (GNU or BSD) has been able to detect the format for a while. Drop
the local auto-detection code. In addition, consistently use -C instead
of open-coding it.

Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
Currently we check for write permissions only if --archive is provided
and is pointing to an existing folder.

In any other case we don't bother, so we get cryptic error messages.

Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
Current codebase is mixing indentation levels - be that tabs or spaces,
to outright omitting a level.

This patch addresses a few cases of the latter, by breaking/returning
early. This way the developer does not need to read/parse through the
remaining code, since the codeflow is clearer.

Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
@evelikov
Copy link
Collaborator Author

Seems like we're trying to sign the modules twice atm - once in do_build and second time in sign_build. The latter cannot work in most cases, since it's looking for uncompressed kernel modules - where most distros compress them.

I'm inclined that we should nuke sign_build and inline prepare_build do_build and clean_build into build_module. Thus one can better reason about what happens plus we can drop some duplication.

@scaronni
Copy link
Member

scaronni commented Nov 2, 2021

Seems like we're trying to sign the modules twice atm - once in do_build and second time in sign_build. The latter cannot work in most cases, since it's looking for uncompressed kernel modules - where most distros compress them.

I'm inclined that we should nuke sign_build and inline prepare_build do_build and clean_build into build_module. Thus one can better reason about what happens plus we can drop some duplication.

Fine to me, would you do it in a separate merge request? I also created this issue as a reminder for myself, maybe you can attach to it with a separate merge request:

#162

Also, I don't have project access to change members, otherwise I would give you direct commit access. Thanks!

@scaronni scaronni merged commit 571fb44 into dkms-project:master Nov 4, 2021
@evelikov evelikov deleted the sent/misc-fixes branch November 6, 2021 17:38
@evelikov
Copy link
Collaborator Author

evelikov commented Nov 6, 2021

Yup, will do separate MR maybe tomorrow. Mind you I don't know much about module signing, so I won't be tackling #162 anytime soon.

Thanks for the vote of confidence - wrt commit access. I think it's better to use PRs, regardless if one has direct commit access.

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.

2 participants