[PATCH 5/9] add a mode commandline option

[Ray Strode]

Hi,

On Mon, Feb 23, 2009 at 3:35 PM,  <william.jon.mccann at gmail.com> wrote:
> From: William Jon McCann <jmccann at redhat.com>
>
> Initially supports modes: boot, shutdown.  This allows the
> progress cache to be loaded from the appropriate file.
Makes sense.


> +enum {
> +  PLY_MODE_BOOT,
> +  PLY_MODE_SHUTDOWN
> +} type;
This should be a named type... something like

typedef enum { ... } ply_mode_t;

>
>  typedef struct
>  {
> @@ -81,6 +87,7 @@ typedef struct
>   ply_buffer_t *entry_buffer;
>   ply_command_parser_t *command_parser;
>   long ptmx;
> +  int mode;
And this should use the above type instead of int.
[...]
> +static const char *
> +get_cache_file_for_mode (int mode)
here, too.
[...]
> +static const char *
> +get_log_file_for_mode (int mode)
> +{
> +  const char *filename;
> +
> +  switch (mode)
> +    {
> +    case PLY_MODE_BOOT:
> +      filename = PLYMOUTH_LOG_DIRECTORY "/boot.log";
> +      break;
> +    case PLY_MODE_SHUTDOWN:
> +      filename = PLYMOUTH_LOG_DIRECTORY "/shutdown.log";
> +      break;
I don't think it makes much sense to have a shutdown.log.  boot.log
currently only shows
the boot messages of the currently running system.  shutdown.log would
have to be
written out half way though shutdown (since the disks get
unmounted...) and it's not clear
what it's lifetime would be.  We also don't have a viewer for it like
we do for boot.log. So,
I'd say it's better to just drop it.
[...]
> +static const char *
> +get_log_spool_file_for_mode (int mode)
> +{
> +  const char *filename;
> +
> +  switch (mode)
> +    {
> +    case PLY_MODE_BOOT:
> +      filename = PLYMOUTH_SPOOL_DIRECTORY "/boot.log";
> +      break;
> +    case PLY_MODE_SHUTDOWN:
> +      filename = PLYMOUTH_SPOOL_DIRECTORY "/shutdown.log";
> +      break;
same here...
[...]
> @@ -1055,6 +1153,7 @@ main (int    argc,
>
>   ply_command_parser_add_options (state.command_parser,
>                                   "help", "This help message", PLY_COMMAND_OPTION_TYPE_FLAG,
> +                                  "mode", "Mode is one of: boot, shutdown", PLY_COMMAND_OPTION_TYPE_STRING,
>                                   "attach-to-session", "pty_master_fd", PLY_COMMAND_OPTION_TYPE_LONG,
>                                   NULL);
Might make sense to have either an PLY_COMMAND_OPTION_TYPE_ENUM or
PLY_COMMAND_OPTION_TYPE_CUSTOM (which took a helper function) to deal
with this.  Probably not worth the effort though.

--Ray