[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [Libosinfo] [libosinfo PATCH 1/2] tools, install-script: Add --config-file (-f) option



On Fri, Jul 5, 2019 at 5:51 PM Daniel P. Berrangé <berrange redhat com> wrote:
>
> On Fri, Jul 05, 2019 at 10:27:19AM +0200, Fabiano Fidêncio wrote:
> > Let's add a new option so users can set their config from a file,
> > instead of directly passing the values via command-line.
> >
> > Signed-off-by: Fabiano Fidêncio <fidencio redhat com>
> > ---
> >  tools/osinfo-install-script.c | 100 +++++++++++++++++++++++++++++++++-
> >  1 file changed, 97 insertions(+), 3 deletions(-)
> >
> > diff --git a/tools/osinfo-install-script.c b/tools/osinfo-install-script.c
> > index 15af48d..efa96ee 100644
> > --- a/tools/osinfo-install-script.c
> > +++ b/tools/osinfo-install-script.c
> > @@ -37,6 +37,34 @@ static gboolean list_profile = FALSE;
> >  static gboolean list_inj_method = FALSE;
> >  static gboolean quiet = FALSE;
> >
> > +static const gchar *configs[] = {
> > +    OSINFO_INSTALL_CONFIG_PROP_HARDWARE_ARCH,
> > +    OSINFO_INSTALL_CONFIG_PROP_L10N_TIMEZONE,
> > +    OSINFO_INSTALL_CONFIG_PROP_L10N_LANGUAGE,
> > +    OSINFO_INSTALL_CONFIG_PROP_L10N_KEYBOARD,
> > +    OSINFO_INSTALL_CONFIG_PROP_ADMIN_PASSWORD,
> > +    OSINFO_INSTALL_CONFIG_PROP_USER_PASSWORD,
> > +    OSINFO_INSTALL_CONFIG_PROP_USER_LOGIN,
> > +    OSINFO_INSTALL_CONFIG_PROP_USER_REALNAME,
> > +    OSINFO_INSTALL_CONFIG_PROP_USER_AUTOLOGIN,
> > +    OSINFO_INSTALL_CONFIG_PROP_USER_ADMIN,
> > +    OSINFO_INSTALL_CONFIG_PROP_REG_LOGIN,
> > +    OSINFO_INSTALL_CONFIG_PROP_REG_PASSWORD,
> > +    OSINFO_INSTALL_CONFIG_PROP_REG_PRODUCTKEY,
> > +    OSINFO_INSTALL_CONFIG_PROP_HOSTNAME,
> > +    OSINFO_INSTALL_CONFIG_PROP_TARGET_DISK,
> > +    OSINFO_INSTALL_CONFIG_PROP_SCRIPT_DISK,
> > +    OSINFO_INSTALL_CONFIG_PROP_AVATAR_LOCATION,
> > +    OSINFO_INSTALL_CONFIG_PROP_AVATAR_DISK,
> > +    OSINFO_INSTALL_CONFIG_PROP_PRE_INSTALL_DRIVERS_DISK,
> > +    OSINFO_INSTALL_CONFIG_PROP_PRE_INSTALL_DRIVERS_LOCATION,
> > +    OSINFO_INSTALL_CONFIG_PROP_POST_INSTALL_DRIVERS_DISK,
> > +    OSINFO_INSTALL_CONFIG_PROP_POST_INSTALL_DRIVERS_LOCATION,
> > +    OSINFO_INSTALL_CONFIG_PROP_DRIVER_SIGNING,
> > +    OSINFO_INSTALL_CONFIG_PROP_INSTALLATION_URL,
> > +    NULL
> > +};
>
> I wonder if its better to keep this valid list of keys in the
> OsinfoInstallConfig class. eg have an
>
>   osinfo_install_config_is_valid_parameter()
>
> function we can call

I thought about that, but then a few other things came to my mind:
- the correct place to have it would be in OsinfoInstallConfigParam
class, something like: osinfo_install_config_param_is_valid();
  - but then it'd require the user to have an OsinfoInstallConfigParam
object at the first place;
  - and the check should be done when creating the OsinfoInstallConfigParam;

Having a function in OsinfoInstallConfig seems weird to be, but if you
really prefer it I can go for it.
(I'm assuming we don't want to backport this patch, as introduce a new
function and depending on that would make the backport not viable).

>
> > +
> >  static OsinfoInstallConfig *config;
> >
> >  static gboolean handle_config(const gchar *option_name G_GNUC_UNUSED,
> > @@ -65,6 +93,47 @@ static gboolean handle_config(const gchar *option_name G_GNUC_UNUSED,
> >  }
> >
> >
> > +static gboolean handle_config_file(const gchar *option_name G_GNUC_UNUSED,
> > +                                   const gchar *value,
> > +                                   gpointer data G_GNUC_UNUSED,
> > +                                   GError **error)
> > +{
> > +    GKeyFile *key_file = NULL;
> > +    gchar *val = NULL;
> > +    gsize i;
> > +    gboolean ret = FALSE;
> > +
> > +    key_file = g_key_file_new();
> > +    if (!g_key_file_load_from_file(key_file, value, G_KEY_FILE_NONE, error))
> > +        goto error;
> > +
> > +    for (i = 0; configs[i] != NULL; i++) {
> > +        val = g_key_file_get_string(key_file, "install-script", configs[i], error);
> > +        if (val == NULL) {
> > +            if (g_error_matches(*error, G_KEY_FILE_ERROR,
> > +                                G_KEY_FILE_ERROR_KEY_NOT_FOUND)) {
> > +                g_clear_error(error);
> > +                continue;
> > +            }
> > +
> > +            goto error;
> > +        }
> > +
> > +        osinfo_entity_set_param(OSINFO_ENTITY(config),
> > +                                configs[i],
> > +                                val);
> > +        g_free(val);
> > +    }
>
> Then here we can just iterate over g_key_file_get_keys() checking if
> each is a valid parameter.
>
> > +
> > +    ret = TRUE;
> > +
> > +error:
> > +    g_key_file_unref(key_file);
> > +
> > +    return ret;
> > +}
>
> > +=head1 CONFIGURATION FILE FORMAT
> > +
> > +The configuration file must consist in a file which contains a
> > +`install-script` group and, under this group, C<key>=C<value>
> > +pairs, as shown below:
> > +
> > +[install-script]
> > +l10n-timezone=GMT
> > +l10n-keyboard=uk
> > +l10n-language=en_GB
> > +admin-password=123456
> > +user-login=berrange
> > +user-password=123456
> > +user-realname="Daniel P Berrange"
>
> Heh, my name is not quite correct

It's consistently wrong. It's better than correct in one place and
wrong in another, no? :-)

>
> > +
> >  =head1 EXAMPLE USAGE
> >
> > -The following usage generates a Fedora 16 kickstart script
> > +The following usages generates a Fedora 16 kickstart script
> > +
> > +  # osinfo-install-script \
> > +         --profile jeos \
> > +         --config-file /path/to/the/config/file \
> > +         fedora16
> >
> >    # osinfo-install-script \
> >           --profile jeos \
> >           --config l10n-timezone=GMT \
> >           --config l10n-keyboard=uk \
> >           --config l10n-language=en_GB \
> > -         --config admin-password=123456 \
> >           --config user-login=berrange \
> > -         --config user-password=123456 \
> >           --config user-realname="Daniel P Berrange" \
>
> but then it wasn't correct originally either :-)
>
> We should have a note here
>
>
>   "Note that use of --config-file is strongly recommended if the
>    user or admin passwords need to be set. Providing passwords
>    directly using --config is insecure as the password is visible
>    to all processes and users o nthe same host."

Indeed.

>
> Regards,
> Daniel
> --
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]