[virt-tools-list] [PATCH libosinfo 2/7] Add APIs for dealing with installer automation scripts

[Zeeshan Ali (Khattak)]

On Tue, Feb 28, 2012 at 5:26 PM, Daniel P. Berrange <berrange at redhat.com> wrote:
> From: "Daniel P. Berrange" <berrange at redhat.com>
>
> This introduces two new objects
>
>  - OsinfoInstallConfig - stores configuration parameters which get
>   substituted into the install script template.
>  - OsinfoInstallScript - provides a template and the mechanism for
>   turning it into an install script using XSLT

Good stuff! Hard to digest as its a big patch so I might be missing
something obvious here but doesn't this assume the install script to
be always in XML format (which is not true for actually most OSs)?

Some small comments below:

> + * Authors:
> + *   Daniel P. Berrange <berrange at redhat.com>
> + */

If you have been learning a lot from others' code, please don't forget
to attribute them.

> +void osinfo_install_config_set_reg_login(OsinfoInstallConfig *config,
> +                                         const gchar *name)
> +{
> +    osinfo_entity_set_param(OSINFO_ENTITY(config),
> +                            OSINFO_INSTALL_CONFIG_PROP_REG_LOGIN,
> +                            name);
> +}
> +
> +const gchar *osinfo_install_config_get_reg_login(OsinfoInstallConfig *config)
> +{
> +    return osinfo_entity_get_param_value(OSINFO_ENTITY(config),
> +                                         OSINFO_INSTALL_CONFIG_PROP_REG_LOGIN);
> +}
> +
> +
> +void osinfo_install_config_set_reg_password(OsinfoInstallConfig *config,
> +                                            const gchar *password)
> +{
> +    osinfo_entity_set_param(OSINFO_ENTITY(config),
> +                            OSINFO_INSTALL_CONFIG_PROP_REG_PASSWORD,
> +                            password);
> +}
> +
> +const gchar *osinfo_install_config_get_reg_password(OsinfoInstallConfig *config)
> +{
> +    return osinfo_entity_get_param_value(OSINFO_ENTITY(config),
> +                                         OSINFO_INSTALL_CONFIG_PROP_REG_PASSWORD);
> +}

Some short doc comments for these getters/setters would be nice,
especially the non-obvious ones above.

> +#define OSINFO_INSTALL_CONFIG_PROP_L10N_TIMEZONE  "l10n-timezone"
> +#define OSINFO_INSTALL_CONFIG_PROP_L10N_LANGUAGE  "l10n-language"
> +#define OSINFO_INSTALL_CONFIG_PROP_L10N_KEYBOARD  "l10n-keyboard"

Is the 'l10n'  really needed in the name? "Localization language"
sounds wrong/weird to me.

> +static gchar *osinfo_install_script_apply_xslt(xsltStylesheetPtr ss,

Would prefer to avoid abbreviating here: ss -> style_sheet.

-- 
Regards,

Zeeshan Ali (Khattak)
FSF member#5124