[systemd-devel] [RFC] [PATCHv3 2/3] resume: add a tool to write a device node's major:minor to /sys/power/resume.

[Lennart Poettering]

On Mon, 25.08.14 02:16, Ivan Shapovalov (intelfx100 at 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-resume at .service.in b/units/systemd-resume at .service.in
> new file mode 100644
> index 0000000..c6aa0d2
> --- /dev/null
> +++ b/units/systemd-resume at .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 at .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