Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
interfaces/udev: add udev support code #636
Conversation
niemeyer
reviewed
Mar 10, 2016
| + return err | ||
| + } | ||
| + err = exec.Command("udevadm", "trigger").Run() | ||
| + return err |
niemeyer
Mar 10, 2016
Contributor
Rather than Run, I suggest using CombinedOutput and doing a nice fmt.Errorf including it (probably needs to be multiline). Otherwise the err by itself will likely be very boring and hard to decipher, and the valueable output will be on the terminal, whatever that happens to be.
Same applies to the apparmor one, if you're doing that. Overlooked that there.
zyga
Mar 10, 2016
Contributor
Good catch, I didn't consider this. I'll play with those tools and see what output they have on failures and adjust this accordingly.
jdstrand
Mar 10, 2016
Contributor
I'm not a udev expert. In general this looks ok but I did notice in the udevadm man page the following:
-R, --reload
Signal systemd-udevd to reload the rules files and other databases
like the kernel module index. Reloading rules and databases does not
apply any changes to already existing devices; the new configuration
will only be applied to new events.
I mention this because of the 'existing devices' part. I'm not sure of the implications of that-- I suggest you ask pitti to comment.
zyga
added some commits
Mar 10, 2016
mvo5
reviewed
Mar 11, 2016
| +// Tests for ReloadRules() | ||
| + | ||
| +func (s *uDevSuite) TestReloadUDevRulesRunsUDevAdm(c *C) { | ||
| + cmd := testutil.MockCommand(c, "udevadm", 0) |
mvo5
Mar 11, 2016
Collaborator
Given that you always mock udevadm, I wonder if it would make sense to put the mock/restore into SetUpTest/TearDownTest ?
mvo5
reviewed
Mar 11, 2016
| +func (s *uDevSuite) TestReloadUDevRulesReportsErrorsFromReloadRules(c *C) { | ||
| + cmd := testutil.MockCommand(c, "udevadm", 0) | ||
| + defer cmd.Restore() | ||
| + cmd.SetDynamicBehavior(1, func(n int) (string, int) { |
mvo5
Mar 11, 2016
Collaborator
I wonder if cmd.SetCustom(if "$1" == "control"; then exit 1; fi)` would be easier to read? And have a SetCustom func instead of SetDynamicBehavior? Because of YAGNI and for the tests at hand it seems like "$1" is the important bit. But again, might be personal preference.
mvo5
reviewed
Mar 11, 2016
| +func (s *uDevSuite) TestReloadUDevRulesReportsErrorsFromTrigger(c *C) { | ||
| + cmd := testutil.MockCommand(c, "udevadm", 0) | ||
| + defer cmd.Restore() | ||
| + cmd.SetDynamicBehavior(2, func(n int) (string, int) { |
mvo5
Mar 11, 2016
Collaborator
See above, something like cmd.SetCustom(if "$1" == "trigger"; then echo "failure 2"; exit 2; fi) ? Or maybe something else, to me SetDynamicBehavior feels both relatively complex to use and also relatively limited (must know amount of calls instead of being able to check for other things like arguments passed). But again, maybe just a personal preference thing.
niemeyer
reviewed
Mar 14, 2016
| +func ReloadRules() error { | ||
| + output, err := exec.Command("udevadm", "control", "--reload-rules").CombinedOutput() | ||
| + if err != nil { | ||
| + return fmt.Errorf("Cannot reload udev rules: %s\nudev output:\n%s", err, string(output)) |
niemeyer
reviewed
Mar 14, 2016
| + } | ||
| + output, err = exec.Command("udevadm", "trigger").CombinedOutput() | ||
| + if err != nil { | ||
| + return fmt.Errorf("Cannot run udev triggers: %s\nudev output:\n%s", err, string(output)) |
niemeyer
reviewed
Mar 14, 2016
| +} | ||
| + | ||
| +func (s *uDevSuite) TestReloadUDevRulesReportsErrorsFromReloadRules(c *C) { | ||
| + s.cmd.SetDynamicBehavior(1, func(n int) (string, int) { |
niemeyer
Mar 14, 2016
Contributor
Sorry for bikesheding even further on this, but I don't understand what this test is doing. The helper, which is supposed to make things simple, is actually obscuring what's going on past the point of being understandable. This was supposed to be a trivial command, right? exec, and this is what happens.. what is "dynamic behavior", 1, with n == 0 or 1 for a mocked command!?
|
Okay, we've agreed to drop the |
zyga commentedMar 10, 2016
This is stacked on #634
This branch adds a function for reloading the udev database.
Signed-off-by: Zygmunt Krynicki zygmunt.krynicki@canonical.com