Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enabling hibernation-setup-tool, hibernate and resume services #7

Open
wants to merge 29 commits into
base: main
Choose a base branch
from

Conversation

ishaansehgal99
Copy link
Contributor

@ishaansehgal99 ishaansehgal99 commented Jul 6, 2022

Currently systemd service hibernation-setup-tool relies on a make file instructions to create folders to store the service and executable.

Lines from Makefile:

	install -m 0755 -d $(DESTDIR)/usr/sbin
	install -m 0755 -d $(DESTDIR)/lib/systemd/system/
	install -m 0755 hibernation-setup-tool $(DESTDIR)/usr/sbin
	install -m 0644 hibernation-setup-tool.service $(DESTDIR)/lib/systemd/system

These make instructions currently do not get executed by the extension. This should get executed by the tool for:

  1. Modularity, future scalability, and idempotency
  2. Incase folders get moved, deleted or altered in anyway, this guarantees on every rerun folders are present
  3. If someone accidentally does not rely on this makefile rule to run the tool
  4. Easier to understand and debug issues if creation is done in tool (this was the case for me)
  5. Not best practice to include logic like this in makefile when can be avoided

In addition hibernation-setup-tool service is not enabled. It is simply ran once by extension. If we are to enable this service to run on reboot, we must set systemctl enable on the service, which is done here.

Finally, if a user wants to set a custom destination directory we support that here with optional arguments. Currently arguments passed are not parsed but grabbed arbitrarily from argv, this isn't good for tool security, readability, robustness and extensibility (as new arguments are to be added). So to add this new argument (destination directory) and for future arguments I added argument parsing. PR for argument parsing: #13

In addition to the above, the code has now implemented sleep hook service files: hibernate.service and resume.service. And enabled these services. These services now run pre and post hooks, before hibernate and after resume. The pre and post hooks tool code these services rely on have been fixed. One so that they run without failure, and two so that they log whether hibernate/resume was successful or not to syslog. PR for fixing hooks: #11

@ishaansehgal99 ishaansehgal99 marked this pull request as ready for review July 7, 2022 18:08

static bool ensure_systemd_service_enabled(char *dest_dir){
const char *execfn = (const char *)getauxval(AT_EXECFN);
const char *usr_sbin_default = "/usr/sbin", *systemd_dir_default = "/lib/systemd/system";

Choose a reason for hiding this comment

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

also usr/lib/systemd/system based on the distro?

Copy link
Contributor Author

@ishaansehgal99 ishaansehgal99 Aug 5, 2022

Choose a reason for hiding this comment

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

On Ubuntu /usr/lib and /lib and different directories. On Ubuntu, /usr/lib is a user-based directory to store things that require access to libraries. The /lib folder is the actual place for essential standard libraries. We need to put our services in this directory in order to access the targets we need (hibernate.target) and for systemd to recognize us as a legitimate service.

After testing on Debian, Rhel and CentOS, the /usr/lib and /lib directories are identical for each of them. On each of them lib is a symbolic link pointing to /usr/lib. In other words, /lib and /usr/lib go to the same place.

/lib is preferred because it includes ubuntu and so works on all distros.

Additional testing: On Debian tool executes without any issues.

However, Rhel has the additional challenges of not having enough space on root "/" partition no matter how much space it's created with. This causes the free space check to prevent tool from running, and if that check is removed, it causes insufficient space error upon allocating hibfile.

@ishaansehgal99 ishaansehgal99 mentioned this pull request Jul 26, 2022
@ishaansehgal99 ishaansehgal99 changed the title Ensuring hibernate-setup-tool service runs on restart Enabling hibernation-setup-tool, hibernate and resume services Jul 27, 2022
Copy link

@dcui dcui left a comment

Choose a reason for hiding this comment

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

I found no obvious issue, so this looks good to me.
@pavanrachapudy: please take a closer look. :-)

hibernation-setup-tool.c Outdated Show resolved Hide resolved
log_info("This is fine, but couldn't remove %s: %s", real_path, strerror(errno));
}
cold_booted = true;
notify_vm_host(HOST_VM_NOTIFY_FAILED_RESUME_FROM_HIBERNATION);

Choose a reason for hiding this comment

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

Just for my understanding - are we coming to conclusion that it is cold boot scenario even if there is some issue in creating the link?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We use the symbolic link (/etc/hibernation-setup-tool.last_hibernation) to find the temporary file. If, after resuming from hibernate, we cannot read from that symbolic link we assume a cold boot. Keep in mind this symbolic link gets created just before hibernating (in the pre hook). So it should exist in the post hook, no room for user intervention.

return true;
}

static bool enable_systemd_service(const char *dir_parent, char *service_file_name, char *systemd_dir) {

Choose a reason for hiding this comment

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

rename the method since it is doing much more than just enabling a service?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed this to link_and_enable_systemd_service. I think that should be a bit more descriptive. This function is essentially hard linking the service file into /lib/systemd/system and then running enable on it.

@@ -0,0 +1,10 @@
[Unit]
Description=User resume actions
After=hibernate.target

Choose a reason for hiding this comment

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

Does this mean it gets triggered only after resume or after cold boot as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This service only gets triggered after resume. I have tested this very thoroughly.

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