On Tue, 24 Feb 2015, Karel Zak wrote:
> On Sun, Feb 22, 2015 at 02:42:08PM +0000, Sami Kerola wrote:
> > Signed-off-by: Sami Kerola <kerolasa@xxxxxx>
> > ---
> > sys-utils/rtcwake.c | 126 ++++++++++++++++++++++++++++++++--------------------
> > 1 file changed, 78 insertions(+), 48 deletions(-)
> >
> > diff --git a/sys-utils/rtcwake.c b/sys-utils/rtcwake.c
> > index 8b5f69c..372e620 100644
> > --- a/sys-utils/rtcwake.c
> > +++ b/sys-utils/rtcwake.c
> > @@ -52,7 +52,37 @@
> > #define RTC_PATH "/sys/class/rtc/%s/device/power/wakeup"
> > #define SYS_POWER_STATE_PATH "/sys/power/state"
> > #define DEFAULT_DEVICE "/dev/rtc0"
> > -#define DEFAULT_MODE "standby"
> > +
> > +enum rtc_modes { /* manual page --mode option explains these. */
> > + STANDBY_MODE = 0,
> > + MEM_MODE,
> > + FREEZE_MODE,
> > + DISK_MODE, /* end of Documentation/power/states.txt modes */
> > + OFF_MODE,
> > + NO_MODE,
> > + ON_MODE, /* smaller <- read the code */
> > + DISABLE_MODE, /* greater <- to understand */
> > + SHOW_MODE,
> > + ERROR_MODE /* invalid user input */
> > +};
>
> Please, don't use ERROR_MODE or so, just return -EINVAL from the
> parser.
>
> > +static int get_mode(const char *optarg)
> > +{
> > + if (!strcmp(optarg, mode_str[STANDBY_MODE]))
> > + return STANDBY_MODE;
> > + if (!strcmp(optarg, mode_str[MEM_MODE]))
> > + return MEM_MODE;
> > + if (!strcmp(optarg, mode_str[DISK_MODE]))
> > + return DISK_MODE;
> > + if (!strcmp(optarg, mode_str[ON_MODE]))
> > + return ON_MODE;
> > + if (!strcmp(optarg, mode_str[NO_MODE]))
> > + return NO_MODE;
> > + if (!strcmp(optarg, mode_str[OFF_MODE]))
> > + return OFF_MODE;
> > + if (!strcmp(optarg, mode_str[FREEZE_MODE]))
> > + return FREEZE_MODE;
> > + if (!strcmp(optarg, mode_str[DISABLE_MODE]))
> > + return DISABLE_MODE;
> > + if (!strcmp(optarg, mode_str[SHOW_MODE]))
> > + return SHOW_MODE;
>
> {
> for (i = 0; i < ARRAY_SIZE(mode_str); i++)
> if (strcmp(optarg, mode_str[i])
> return i;
>
> return -EINVAL;
> }
>
> > - if (!alarm && !seconds && strcmp(suspend,"disable") &&
> > - strcmp(suspend,"show")) {
> > -
> > + if (!alarm && !seconds && suspend < DISABLE_MODE) {
>
> this ("<") is pretty fragile, use
>
> && (suspend != DISABLE_MODE || suspend != SHOW_MODE)
>
> > warnx(_("must provide wake time (see -t and -s options)"));
> > usage(stderr);
>
> ...
>
> > - if (strcmp(suspend, "show") && strcmp(suspend, "disable")) {
> > - if (strcmp(suspend, "no") && strcmp(suspend, "on") &&
> > - strcmp(suspend, "off") && is_suspend_available(suspend) <= 0) {
> > - errx(EXIT_FAILURE, _("suspend to \"%s\" unavailable"), suspend);
> > + if (suspend < DISABLE_MODE) {
> > + if (suspend < OFF_MODE && is_suspend_available(suspend) <= 0) {
> > + errx(EXIT_FAILURE, _("suspend to \"%s\" unavailable"),
> > + mode_str[suspend]);
> }
>
> the same situation
Thanks Karel,
The get_mode() is simplified, and places where order of enum mattered
(fragile) are made to be explicit what modes are/aren't allowed.
https://github.com/kerolasa/lelux-utiliteetit/commit/bfdcfc8177bc9b7c724ee0adc779083a01b13399
--
Sami Kerola
http://www.iki.fi/kerolasa/
--
To unsubscribe from this list: send the line "unsubscribe util-linux" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
