Skip to content

Conversation

@yanosz
Copy link

@yanosz yanosz commented Feb 27, 2017

@yanosz yanosz force-pushed the master+flyspray_433 branch from 876f731 to a5019e5 Compare February 27, 2017 15:41
@mkresin
Copy link
Contributor

mkresin commented Feb 27, 2017

Your commit has formal issues. Please have a look at draft version of the github submission guidelines and the generic submission guidelines.

  • no signed of by line
  • no commit message
  • wrong/missing prefix in commit title

Please have a look at the LEDE commit history to have some examples on how to do it the right way.

When installing kmod-* packages, insert_modules modprobes all modules.
modprobe fails, if the module is already loaded and succeeds if the module
doesn't exists. Ignoring its return code appears reasonable.
This causes package installation to fail

Fix FS#433 - kmod-ebtables, kmod-br-netfilter opkg install failed
Related to Busybox bug 9676

Signed-off-by: Jan 'yanosz' Lühr <lede@yanosz.net>
@yanosz yanosz force-pushed the master+flyspray_433 branch from a5019e5 to 71b45b3 Compare February 27, 2017 16:15
@yanosz
Copy link
Author

yanosz commented Feb 27, 2017

hmm.. hope hope its ok, now

# Since busybox modprobe returns 0 on loading non-existing
# and 255 on loading already loaded modules
# its return-code is not useful
modprobe $m || true
Copy link
Contributor

Choose a reason for hiding this comment

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

better make it modprobe; [ "$?" = 0 -o "$?" = 255 ] and adapt the comment accordingly

Copy link
Author

@yanosz yanosz Mar 1, 2017

Choose a reason for hiding this comment

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

hmm... I don't agree.
This code doesn't ignore modprobe's return code. I think, we should do so, since its bogus

Thus I don't like to make "255" explicit and go for ignoring modprobe's return code at all.
I think, that there's a semantic difference.

If you are aware of specific return code, that should be taken into account, I'm happy with mapping that option to some error value.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, got it. thanks for the efforts ;)

@yousong
Copy link
Contributor

yousong commented Mar 3, 2017

Well, I just checked the issue again. It seems like the modprobe in question here is from the ubox package, not busybox. We should fix ubox kmodloader instead.

@yanosz , to verify, please check the output of ls -l $(which modprobe)

@mkresin
Copy link
Contributor

mkresin commented Mar 11, 2017

This patch shouldn't be required any longer. The issue was fixed with https://git.lede-project.org/21a4bd04062e16164f4f3e1c67005a062f470b0c, which went into LEDE with https://git.lede-project.org/c8f7031ba76824642035ed0525fd640e00f58926.

Beside of that, no response from the PR author.

@mkresin mkresin closed this Mar 11, 2017
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.

3 participants