[Libosinfo] [PATCH 6/6] Apply datamap to config parameters when generating install script

[Zeeshan Ali (Khattak)]

On Wed, Jan 9, 2013 at 2:37 PM, Daniel P. Berrange <berrange at redhat.com> wrote:
> From: "Daniel P. Berrange" <berrange at redhat.com>
>
> When creating the XML to use in the install script XSL transform,
> apply any datamap associated with the config parameters.

Looks good. Some minor concerns below. I assume you tested it?

> ---
>  osinfo/osinfo_install_script.c | 72 ++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 66 insertions(+), 6 deletions(-)
>
> diff --git a/osinfo/osinfo_install_script.c b/osinfo/osinfo_install_script.c
> index b82c0f9..47be667 100644
> --- a/osinfo/osinfo_install_script.c
> +++ b/osinfo/osinfo_install_script.c
> @@ -603,7 +603,59 @@ static xsltStylesheetPtr osinfo_install_script_load_template(const gchar *uri,
>      return xslt;
>  }
>
> -static xmlNodePtr osinfo_install_script_generate_entity_config(OsinfoInstallConfig *config,
> +
> +static OsinfoDatamap *
> +osinfo_install_script_get_param_datamap(OsinfoInstallScript *script,
> +                                        const gchar *param_name)
> +{
> +    OsinfoEntity *entity;
> +    OsinfoInstallConfigParam *param;
> +
> +    if (!script->priv->config_params)
> +        return NULL;
> +
> +    entity = osinfo_list_find_by_id(OSINFO_LIST(script->priv->config_params),
> +                                    param_name);
> +    if (entity == NULL) {
> +        g_debug("%s is not a known parameter for this config", param_name);
> +        return NULL;
> +    }
> +
> +    param = OSINFO_INSTALL_CONFIG_PARAM(entity);
> +    return osinfo_install_config_param_get_value_map(param);
> +}
> +
> +
> +static GList *
> +osinfo_install_script_get_param_value_list(OsinfoInstallScript *script,
> +                                           OsinfoInstallConfig *config,
> +                                           const gchar *key)
> +{
> +    GList *values;
> +    GList *it;
> +    OsinfoDatamap *map;
> +
> +    values = osinfo_entity_get_param_value_list(OSINFO_ENTITY(config), key);
> +    if (values == NULL)
> +        return NULL;
> +
> +    map = osinfo_install_script_get_param_datamap(script, key);
> +    if (map != NULL) {
> +        for (it = values; it != NULL; it = it->next) {
> +            const char *transformed_value;
> +            transformed_value = osinfo_datamap_lookup(map, it->data);
> +            if (transformed_value == NULL) {
> +                continue;
> +            }
> +            it->data = (gpointer)transformed_value;

Is the cast really needed?

> +        }
> +    }
> +
> +    return values;
> +}
> +
> +
> +static xmlNodePtr osinfo_install_script_generate_entity_config(OsinfoInstallScript *script,

The name was already a bit misleading IMHO but now that it doesn't
take a 'config', its seems even more misleading. Could we name it to
osinfo_install_script_generate_entity_xml instead?

>                                                                 OsinfoEntity *entity,
>                                                                 const gchar *name,
>                                                                 GError **error)
> @@ -636,9 +688,17 @@ static xmlNodePtr osinfo_install_script_generate_entity_config(OsinfoInstallConf
>
>      tmp1 = keys = osinfo_entity_get_param_keys(entity);
>      while (tmp1) {
> -        GList *values = osinfo_entity_get_param_value_list(entity, tmp1->data);
> -        GList *tmp2 = values;
> +        GList *values;
> +        GList *tmp2;
> +
> +        if (OSINFO_IS_INSTALL_CONFIG(entity))
> +            values = osinfo_install_script_get_param_value_list(script,
> +                                                                OSINFO_INSTALL_CONFIG(entity),
> +                                                                tmp1->data);
> +        else
> +            values = osinfo_entity_get_param_value_list(entity, tmp1->data);
>
> +        tmp2 = values;
>          while (tmp2) {
>              if (!(data = xmlNewDocNode(NULL, NULL, (const xmlChar*)tmp1->data,
>                                         (const xmlChar*)tmp2->data))) {
> @@ -686,7 +746,7 @@ static xmlDocPtr osinfo_install_script_generate_config_xml(OsinfoInstallScript *
>                           NULL);
>      xmlDocSetRootElement(doc, root);
>
> -    if (!(node = osinfo_install_script_generate_entity_config(config,
> +    if (!(node = osinfo_install_script_generate_entity_config(script,
>                                                                OSINFO_ENTITY(script),
>                                                                "script",
>                                                                error)))
> @@ -697,7 +757,7 @@ static xmlDocPtr osinfo_install_script_generate_config_xml(OsinfoInstallScript *
>          goto error;
>      }
>
> -    if (!(node = osinfo_install_script_generate_entity_config(config,
> +    if (!(node = osinfo_install_script_generate_entity_config(script,
>                                                                OSINFO_ENTITY(os),
>                                                                "os",
>                                                                error)))
> @@ -708,7 +768,7 @@ static xmlDocPtr osinfo_install_script_generate_config_xml(OsinfoInstallScript *
>          goto error;
>      }
>
> -    if (!(node = osinfo_install_script_generate_entity_config(config,
> +    if (!(node = osinfo_install_script_generate_entity_config(script,
>                                                                OSINFO_ENTITY(config),
>                                                                "config",
>                                                                error)))

-- 
Regards,

Zeeshan Ali (Khattak)
FSF member#5124