Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Add support for reboot parameter #3353
Conversation
| +{ | ||
| + FILE *f; | ||
| + | ||
| + *arg = '\0'; |
zyga
May 19, 2017
•
Edited 1 time
-
zyga
May 19, 2017
Contributor
-
May 19, 2017
Ugh
This doesn't look at max_size. Perhaps I'm skewed towards security sensitive code (from snap-confine) but I prefer defensive coding.
| + | ||
| + *arg = '\0'; | ||
| + | ||
| + f = fopen("/run/systemd/reboot-param", "r"); |
zyga
May 19, 2017
Contributor
Where is this file name coming from? Is this our invention? If so, can we call it something like snapd.reboot-mode
alfonsosanchezbeato
May 19, 2017
Contributor
Nope, it is the same way used by systemd:
https://github.com/systemd/systemd/blob/master/src/systemctl/systemctl.c#L8262
morphis
May 19, 2017
Contributor
Can we get this properly documented with a link to the systemd source (a tagged version)? Also needs to be added in the go portion of this PR.
| @@ -29,4 +29,8 @@ void die(const char *msg); | ||
| __attribute__ ((format(printf, 1, 2))) | ||
| void kmsg(const char *fmt, ...); | ||
| +// Reads a possible argument for reboot syscall in /run/systemd/reboot-param, | ||
| +// which is the place where systemd stores it. | ||
| +int read_reboot_arg(char *arg, int max_size); |
zyga
May 19, 2017
Contributor
Please prefix this with sc_, the second argument should really be size_t
chipaca
May 19, 2017
Member
why the prefix? it's got nothing to do with snap-confine (and nothing else in system-shutdown-utils.h has that prefix)
alfonsosanchezbeato
May 19, 2017
Contributor
Re: size_t I always have mixed feelings wrt using natural integer size vs array sizes, it is a C language issue in the end. In fact, fgets uses "int", but in this case the advantage of using size_t is that you make sure that max_size is non-negative, so I will change the type.
zyga
May 19, 2017
Contributor
The prefix is used on all non-private functions simply because you can just move code around from place to place without thinking about it too hard. It is also free to do it now.
| #include "system-shutdown-utils.h" | ||
| #include "../libsnap-confine-private/string-utils.h" | ||
| int main(int argc, char *argv[]) | ||
| { | ||
| + char reboot_arg[LINE_MAX] = { 0 }; |
zyga
May 19, 2017
Contributor
I think we can reasonably say the size is something smaller, you don't need all of that space.
| errno = 0; | ||
| if (getpid() != 1) { | ||
| fprintf(stderr, | ||
| "This is a shutdown helper program; don't call it directly.\n"); | ||
| exit(1); | ||
| } | ||
| - kmsg("started."); | ||
| + kmsg("started. Hi there!"); |
| - reboot(cmd); | ||
| + // We are reading a file from /run and need to do this before unmounting | ||
| + if (cmd == RB_AUTOBOOT) | ||
| + read_reboot_arg(reboot_arg, sizeof reboot_arg); |
| + LINUX_REBOOT_MAGIC1, LINUX_REBOOT_MAGIC2, | ||
| + LINUX_REBOOT_CMD_RESTART2, reboot_arg); | ||
| + else | ||
| + reboot(cmd); |
zyga
May 19, 2017
Contributor
Can you add error handling to both cases please. It doesn't have to be very sophisticated but I'd like to see it.
| +func (a *androidboot) Reboot() error { | ||
| + // Write argument so we reboot to recovery partition | ||
| + param := []byte("recovery\n") | ||
| + if err := ioutil.WriteFile("/run/systemd/reboot-param", param, 0644); err != nil { |
pedronis
May 19, 2017
Contributor
if this is not systemd predefined shouldn't it go to /run/snapd/ instead?
alfonsosanchezbeato
May 19, 2017
•
Edited 1 time
-
alfonsosanchezbeato
May 19, 2017
Contributor
-
May 19, 2017
It is defined by systemd:
https://github.com/systemd/systemd/blob/master/src/systemctl/systemctl.c#L8262
pedronis
May 19, 2017
•
Edited 1 time
-
pedronis
May 19, 2017
Contributor
-
May 19, 2017
should this use osutil.AtomicWrite ? are we writing over a systemd undocumented file, that sounds odd? can't we use our own file ? given that we are consuming it from our own code?
alfonsosanchezbeato
May 19, 2017
Contributor
I prefer to use the same file because this way system-shutdown works properly not only for snapd, but also for the "reboot" command (there are those two ways to generate the file, so better to use the same interface, otherwise we would need to support two different ways of receiving the information in system-shutdown). Note also that we would not need the file if "shutdown +x -r" worked the same way as "reboot", which is a systemd inconsistency.
alfonsosanchezbeato
May 19, 2017
Contributor
Re: using AtomicWrite this is an ephemeral archive, so no, it does not make much sense imo.
| @@ -146,3 +155,13 @@ func MarkBootSuccessful(bootloader Bootloader) error { | ||
| return bootloader.SetBootVars(m) | ||
| } | ||
| + | ||
| +func reboot() error { | ||
| + cmd := exec.Command("shutdown", "+10", "-r", shutdownMsg) |
pedronis
May 19, 2017
Contributor
given we are doing this, can we instead of hard coding +10 here, pass in a afterMins params to Reboot(afterMins int)
| + bootloader, err := partition.FindBootloader() | ||
| + if err != nil { | ||
| + logger.Noticef("cannot get bootloader: %s", err) | ||
| + break |
morphis
May 19, 2017
Contributor
Shouldn't we have a proper fallback here when no bootloader is found and still call "shutdown"? Not sure if we can otherwise supports environments like docker or LXD.
pedronis
May 19, 2017
Contributor
that's a much bigger issue than this place in the code (there various places assuming that if we are core system we will get a reboot in some situations), if we have such issues we need to think through them, this is probably not the right PR for that
| @@ -75,3 +76,13 @@ func (a *androidboot) SetBootVars(values map[string]string) error { | ||
| } | ||
| return env.Save() | ||
| } | ||
| + | ||
| +func (a *androidboot) Reboot() error { |
alfonsosanchezbeato
May 19, 2017
Contributor
This one is tricky as it is writing in a global folder (/run/systemd) for which root permissions are needed, and I do not see an equivalent of MockCommand for this.
pedronis
May 19, 2017
Contributor
don't hardcode the folder here but in dirs/dirs.go and follow the patterns there, then you can use dirs.SetRootDir in tests to have an alternate root dir
alfonsosanchezbeato
May 19, 2017
Contributor
Not sure if we should do that, as this is not a snapd folder, but a systemd one.
pedronis
May 19, 2017
Contributor
I'm telling you to do that! there's a lot of system directories already listed there because of similar reasons, for example
LocaleDir = filepath.Join(rootdir, "/usr/share/locale")
otoh see my comment about being a bit strange to write over a systemd file
| @@ -146,3 +155,13 @@ func MarkBootSuccessful(bootloader Bootloader) error { | ||
| return bootloader.SetBootVars(m) | ||
| } | ||
| + | ||
| +func reboot() error { |
morphis
May 19, 2017
Contributor
Also I think we should give this a better name. like regularSystemReboot() so it name expresses a bit more that this is the std. way of doing a reboot.
alfonsosanchezbeato
May 19, 2017
Contributor
I thought about that, but how do you unit-test something that reboots the system (or fails if not root)?...
morphis
May 19, 2017
Contributor
By mocking the actual exec call :-)
We do this elsewhere in the snapd code base already. https://github.com/snapcore/snapd/blob/master/systemd/systemd_test.go is a good reference.
|
as i mentioned on IRC, i'd prefer to do all this on a lower level simply through systemd configuration so that every reboot gets us into recovery regardless ... that said, i dont mind if you add a super complex function to snapd like above instead, if it fulfills the same purpose as hardcoding "reboot recovery" on a systemd level for all reboot calls, i just think it is overkill. |
|
PR refreshed, thanks for all the comments from the different reviewers! |
| +func reboot(afterMins int) error { | ||
| + cmd := exec.Command("shutdown", fmt.Sprintf("+%d", afterMins), "-r", shutdownMsg) | ||
| + if out, err := cmd.CombinedOutput(); err != nil { | ||
| + logger.Noticef("%s", osutil.OutputErr(out, err)) |
pedronis
May 19, 2017
Contributor
this should return osutil.OutputErr directly and the logging moved back to daemon.go I think
| @@ -157,3 +161,23 @@ func (s *PartitionTestSuite) TestInstallBootloaderConfig(c *C) { | ||
| c.Assert(osutil.FileExists(fn), Equals, true) | ||
| } | ||
| } | ||
| + | ||
| +func (s *PartitionTestSuite) TestRebootNoError(c *C) { |
pedronis
May 19, 2017
Contributor
can we test the argument to shutdown as well here ? we should have examples how to do that
| + fclose(f); | ||
| + return -1; | ||
| + } | ||
| + arg[strcspn(arg, "\n")] = '\0'; |
alfonsosanchezbeato
May 23, 2017
Contributor
strchr() would need a check for NULL pointer on failure to find '\n', which is not needed by strcspn()
| } | ||
| + // Reboot in 10 minutes | ||
| + bootloader.RebootForUpdate(10) |
mvo5
May 22, 2017
Collaborator
I would prefer it bootloader.RebootForUpdate() takes a time.Time - this way it is more clear what the parameter means and the code is easier to read (even without a comment).
| + defer systemdCmd.Restore() | ||
| + | ||
| + err := reboot(10) | ||
| + c.Assert(err, IsNil) |
mvo5
May 22, 2017
Collaborator
I think we also want to c.Check(systemdCmd.Calls(), DeepEquals, [][]string{ {"shutdown", "+10"} }) (or similar) here just to ensure that a) shutdown got really called b) it was called with the right parameters.
| + defer systemdCmd.Restore() | ||
| + | ||
| + err := reboot(10) | ||
| + c.Assert(err, NotNil) |
mvo5
May 22, 2017
Collaborator
If we could use c.Assert(err, ErrorMatches, "the actual error") that would be great.
| @@ -81,3 +81,7 @@ func (g *grub) SetBootVars(values map[string]string) error { | ||
| } | ||
| return env.Save() | ||
| } | ||
| + | ||
| +func (g *grub) RebootForUpdate(afterMins int) error { | ||
| + return reboot(afterMins) |
mvo5
May 22, 2017
Collaborator
This (and the one for uboot) are untested currently and this will reduce the test-coverage. Not a big deal, we can do a followup branch but if we can get a test for this cheaply that would be great.
|
Please consider this branch on hold, as I have added a comment in the android support thread in the forum and the way to do this might change: https://forum.snapcraft.io/t/android-support-in-snapd/327/34 |
pedronis
added
the
Blocked
label
May 22, 2017
|
After some discussion in the forum thread, we are probably not going to use the snapd parts of this PR. I have repushed just the changes to system-shutdown, which are good to have anyway so we fulfill systemd interface for the reboot argument. @pedronis please consider the PR unblocked and ready for review again. |
codecov-io
commented
May 23, 2017
•
Edited 1 time
-
codecov-io
May 31, 2017
-
May 31, 2017
Codecov Report
@@ Coverage Diff @@
## master #3353 +/- ##
==========================================
- Coverage 77.56% 77.56% -0.01%
==========================================
Files 371 371
Lines 25519 25519
==========================================
- Hits 19794 19793 -1
- Misses 3975 3976 +1
Partials 1750 1750
Continue to review full report at Codecov.
|
pedronis
dismissed
their
stale review
May 24, 2017
branch has been reduced since
| @@ -56,6 +62,8 @@ int main(int argc, char *argv[]) | ||
| if (mkdir("/writable", 0755) < 0) { | ||
| die("cannot create directory /writable"); | ||
| } | ||
| + // We are reading a file from /run and need to do this before unmounting | ||
| + sc_read_reboot_arg(reboot_arg, sizeof reboot_arg); |
alfonsosanchezbeato
May 24, 2017
Contributor
Yes, because no action would be taken if there is a failure to read the file, we will simply have reboot_arg untouched, which is checked later when choosing how to reboot. If you want we can print something here, but just an info log, as the file not being present would be actually quite frequent.
| + ret = reboot(cmd); | ||
| + | ||
| + if (ret == -1) | ||
| + kmsg("Error calling reboot! : %s (%d)", strerror(errno), errno); |
zyga
May 24, 2017
Contributor
Nitpick, can you please reword this: kmsg(cannot reboot the system: %s", strerror(errno));
| @@ -93,7 +101,19 @@ int main(int argc, char *argv[]) | ||
| } | ||
| } | ||
| - reboot(cmd); | ||
| + // glibc reboot wrapper does not expose the optional reboot syscall | ||
| + // parameter |
chipaca
May 24, 2017
Member
you could also use reboot(2) from linux/reboot.h instead of sys/reboot.h, but not both of these at the same time.
alfonsosanchezbeato
May 24, 2017
Contributor
Not sure what you mean, /usr/include/linux/reboot.h only shows some macros, and does not have any include directive.
|
MP refreshed after addressing comments. |
pedronis
removed
the
Blocked
label
May 25, 2017
alfonsosanchezbeato
added some commits
May 18, 2017
alfonsosanchezbeato commentedMay 19, 2017
Add support for reboot parameter in system-shutdown program, and use it for the android bootloader.