Skip to content
This repository was archived by the owner on Jan 31, 2019. It is now read-only.

Conversation

@kimrhh
Copy link
Contributor

@kimrhh kimrhh commented Jan 12, 2017

No description provided.

Kim Højgaard-Hansen added 2 commits January 12, 2017 10:59
This makes it possible to just depend on a dhcp-client and then let
the configuration decide which recipe to prefer. Busybox already
use the same provide.
if [ "$?" != "0" ] ; then
start-stop-daemon -K --pidfile $PIDFILE --exec $DAEMON
fi
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this work? You still have set -e, so if the first start-stop-daemon fails, you should never get around to testing $?. At least that how a quick test in bash behaves. If you make the do_stop part of a conditional chain, the set -e behaviour is suppressed, so it seems (again, tested in bash) that one can do "do_stop || true" in the restart) case. But then you don't need to change do_stop itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it does work, since the -o just "checking if work is to be done" according to the docs, which means we only really stop it if there is something to stop. It is surely not the most obvious way to solve this, but dropping set -e means we need to change it a lot more to handle errors

Copy link
Contributor

Choose a reason for hiding this comment

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

The docs say "-o,--oknodo Exit with status 0 if nothing is done", which I read as saying that the first start-stop-daemon does attempt to stop $DAEMON - it just also returns 0 if it doesn't succeed in doing so. And it seems that this means "/etc/init.d/dhcpcd stop" will always return 0, which I don't like (but I'm fine with making restart mean "restart or start"). But regardless of the semantics of -o, the last three lines are dead code, because if the first start-stop-daemon exits with 0, we do nothing, and if it exits non-zero, the script exits right there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you are right the three lines are dead code, but I don't think you are right that stop will always return 0 when we add the -o , there is surely a difference between "no work done" and "there was an error when stopping", but will verify tomorrow to be sure.

I don't think it is a great idea to throw away an error when stopping with || true

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, it would be absolutely ideal to have the action "stop" exit non-zero if an error occured or no daemon was running, while "restart" would ignore the latter case and proceed to the start part. However, the source doesn't suggest that's how it works:

	if (opt & CTX_STOP) {
		int i = do_stop();
		return (opt & OPT_OKNODO) ? 0 : (i <= 0);
	}

do_install[prefuncs] += "do_install_config"
PACKAGES =+ "${PN}-conf"
FILES_${PN}-conf = "${sysconfdir}/default/dhcpcd ${sysconfdir}/dhcpcd.conf"
RDEPENDS_${PN} += "${PN}-conf"
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: missing newline at end of file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed :)

Allow the default arguments for dhcpcd when started from the init script
to be set with a USE-flag, and split the configuration files into a seperate
package to allow the full configuration to be overriden from a pure configuration
recipe in e.g. a BSP manifest.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants