On Mon, 25.08.14 02:16, Ivan Shapovalov (intelfx...@gmail.com) wrote:
> +int main(int argc, char *argv[]) {
> + struct stat st;
> + const char *device;
> + _cleanup_free_ char *major_minor = NULL;
> + int r;
> +
> + if (argc != 2) {
> + log_error("This program expects one argument.");
> + return EXIT_FAILURE;
> + }
> +
> + log_set_target(LOG_TARGET_AUTO);
> + log_parse_environment();
> + log_open();
> +
> + umask(0022);
> +
> + device = argv[1];
> +
> + if (stat(device, &st) < 0) {
> + log_error("Failed to stat '%s': %m", device);
> + return EXIT_FAILURE;
> + }
> +
> + if (!S_ISBLK(st.st_mode)) {
> + log_error("Resume device '%s' is not a block device.",
> device);
> + return EXIT_FAILURE;
> + }
> +
> + if (asprintf(&major_minor, "%d:%d", major(st.st_rdev),
> minor(st.st_rdev)) < 0) {
> + log_oom();
> + return EXIT_FAILURE;
> + }
> +
> + r = write_string_file("/sys/power/resume", major_minor);
> + if (r < 0) {
> + log_error("Failed to write '%s' to /sys/power/resume: %s",
> + major_minor, strerror(-r));
> + return EXIT_FAILURE;
So, we explicitly do not break lines at 80ch... Nobody has such tiny
screens anymore. This doesn't really matter, but I 'd prefer if you
didn't break lines thaaaaat aggressively. I mean, don't overdo it with
super long lines either, but 140ch or so should be OK.
(this is just nitpicking, and not a necessity to fix before I merge it...)
> + }
> +
> + /*
> + * The write above shall not return.
> + *
> + * However, failed resume is a normal condition (may mean that there
> is
> + * no hibernation image).
> + */
> +
> + log_notice("Failed to resume from device '%s' (%s).",
> + device, major_minor);
same here..
Hmm, what's the logic here? Is this something where we should halt the
machine? How "fatal" shall we consider this error?
> + return EXIT_SUCCESS;
... especially, since you return "success" here...
I am just wondering whether log_notice() + EXIT_SUCCESS is the right
reaction here. Or maybe log_error() + freeze()? or maybe log_warning() +
EXIT_FAILURE?... Dunno. Just want to hear some rationale here...
> +}
> diff --git a/units/systemd-res...@.service.in
> b/units/systemd-res...@.service.in
> new file mode 100644
> index 0000000..c6aa0d2
> --- /dev/null
> +++ b/units/systemd-res...@.service.in
> @@ -0,0 +1,20 @@
> +# This file is part of systemd.
> +#
> +# systemd is free software; you can redistribute it and/or modify it
> +# under the terms of the GNU Lesser General Public License as published by
> +# the Free Software Foundation; either version 2.1 of the License, or
> +# (at your option) any later version.
> +
> +[Unit]
> +Description=Resume from hibernation using device %f
> +Documentation=man:systemd-resume@.service(8)
> +DefaultDependencies=no
> +BindsTo=%i.device
> +Wants=local-fs-pre.target
> +After=%i.device systemd-udevd.service
Ordering after systemd-udevd.service sounds unnecessary? I mean, no
devices will show up before udevd, but there's no need to do anything
about this.
> +Before=local-fs-pre.target systemd-remount-fs.service
> systemd-fsck-root.service usr.mount
The usr.mount should really not be listed.
> +ConditionPathExists=/etc/initrd-release
> +
> +[Service]
> +Type=oneshot
> +ExecStart=@rootlibexecdir@/systemd-resume %f
Looks great otherwise!
Lennart
--
Lennart Poettering, Red Hat
_______________________________________________
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
