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

[Ivan Shapovalov]

On Monday 25 August 2014 at 19:49:52, Lennart Poettering wrote:	
> 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...)

That's because I've seen 80-wrapped lines in couple of other places.
Fixed for v4.

> 
> > +        }
> > +
> > +        /*
> > +         * 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..

Fixed as well.

> 
> 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... 

This is absolutely not fatal, and arguably not an error condition at all.

If a machine is simply rebooted, without hibernation image in place (but with
resume= kernel parameter), this code path will be hit.

We could return EXIT_FAILURE with something like "no image == failure to resume"
in mind, but this does not really worth attention as an error.

(Reaction to other errors handled in this file can be subject to discussion.

Errors like "wrong parameter, device not found" are essentially programmatic,
because the unit has correct dependencies in place.

Failing to write to /sys/power/resume can indicate that the resume has been
unsuccessful: corrupted image, filesystem mount timestamps are post hibernation,
and so on. I have not really triggered failure conditions so I don't know
whether a write error is really returned in this case. But if I were
kernel-space, I'd do it this way.)

> 
> > +}
> > 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
> 
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 213 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/systemd-devel/attachments/20140825/4c976dd1/attachment.sig>