[virt-tools-list] [libosinfo 07/11] OsinfoInstallScript: Use an OsinfoInstallConfigParamList

[Zeeshan Ali (Khattak)]

On Mon, Dec 10, 2012 at 6:46 PM, Christophe Fergeau <cfergeau at redhat.com> wrote:
> Currently the install script config parameters are stored in a
> raw GList. However, OsinfoInstallScript ends up reimplementing
> part of the OsinfoList API, and the raw GList also does not make
> it convenient to pass the list of config parameters around.
> Replace the internal GList with an OsinfoInstallConfigParamList,
> which has the side-effect of nicely simplifying the code.
> ---
>  osinfo/libosinfo.syms          |  1 +
>  osinfo/osinfo_install_script.c | 69 +++++++++++++++++++++---------------------
>  osinfo/osinfo_install_script.h |  1 +
>  3 files changed, 36 insertions(+), 35 deletions(-)
>
> diff --git a/osinfo/libosinfo.syms b/osinfo/libosinfo.syms
> index d96ac53..7ec1bbd 100644
> --- a/osinfo/libosinfo.syms
> +++ b/osinfo/libosinfo.syms
> @@ -374,6 +374,7 @@ LIBOSINFO_0.2.2 {
>         osinfo_install_script_get_avatar_format;
>         osinfo_install_script_get_can_pre_install_drivers;
>         osinfo_install_script_get_can_post_install_drivers;
> +       osinfo_install_script_get_config_paramlist;
>         osinfo_install_script_get_path_format;
>         osinfo_install_script_get_product_key_format;
>
> diff --git a/osinfo/osinfo_install_script.c b/osinfo/osinfo_install_script.c
> index cecad04..bb7ad29 100644
> --- a/osinfo/osinfo_install_script.c
> +++ b/osinfo/osinfo_install_script.c
> @@ -50,7 +50,7 @@ struct _OsinfoInstallScriptPrivate
>  {
>      gchar *output_prefix;
>      gchar *output_filename;
> -    GList *config_param_list;
> +    OsinfoInstallConfigParamList *config_params;
>      OsinfoAvatarFormat *avatar;
>  };
>
> @@ -164,8 +164,11 @@ osinfo_install_script_finalize (GObject *object)
>      OsinfoInstallScript *script = OSINFO_INSTALL_SCRIPT (object);
>      g_free(script->priv->output_prefix);
>      g_free(script->priv->output_filename);
> -    g_list_free_full(script->priv->config_param_list, g_object_unref);
> -    if (script->priv->avatar != NULL)
> +
> +    if (script->priv->config_params)
> +        g_object_unref(script->priv->config_params);
> +
> +    if (script->priv->avatar)
>          g_object_unref(script->priv->avatar);
>
>      /* Chain up to the parent class */
> @@ -258,35 +261,20 @@ void osinfo_install_script_add_config_param(OsinfoInstallScript *script, OsinfoI
>      g_return_if_fail(OSINFO_IS_INSTALL_SCRIPT(script));
>      g_return_if_fail(OSINFO_IS_INSTALL_CONFIG_PARAM(param));
>
> -    script->priv->config_param_list =
> -        g_list_prepend(script->priv->config_param_list, param);
> +    osinfo_list_add(OSINFO_LIST(script->priv->config_params),
> +                    OSINFO_ENTITY(param));
>  }
>
>  gboolean osinfo_install_script_has_config_param(const OsinfoInstallScript *script, const OsinfoInstallConfigParam *config_param)
>  {
> -    GList *l;
> -
> -    for (l = script->priv->config_param_list; l != NULL; l = l->next) {
> -        OsinfoInstallConfigParam *tmp = l->data;
> -
> -        if (g_strcmp0(osinfo_install_config_param_get_name(tmp),
> -                      osinfo_install_config_param_get_name(config_param)) == 0)
> -            return TRUE;
> -    }
> -    return FALSE;
> +    const char *name = osinfo_install_config_param_get_name(config_param);
> +    return osinfo_install_script_has_config_param_name(script, name);
>  }
>
>  gboolean osinfo_install_script_has_config_param_name(const OsinfoInstallScript *script, const gchar *name)
>  {
> -    GList *l;
> -
> -    for (l = script->priv->config_param_list; l != NULL; l = l->next) {
> -        OsinfoInstallConfigParam *tmp = l->data;
> -
> -        if (g_strcmp0(osinfo_install_config_param_get_name(tmp), name) == 0)
> -            return TRUE;
> -    }
> -    return FALSE;
> +    OsinfoList *l = OSINFO_LIST(script->priv->config_params);
> +    return (osinfo_list_find_by_id(l, name) != NULL);

Should this code be assuming that name and ID are the same thing for
ConfigParam?

>  }
>
>  /**
> @@ -300,7 +288,21 @@ gboolean osinfo_install_script_has_config_param_name(const OsinfoInstallScript *
>   */
>  GList *osinfo_install_script_get_config_param_list(const OsinfoInstallScript *script)
>  {
> -    return g_list_copy(script->priv->config_param_list);
> +    return osinfo_list_get_elements(OSINFO_LIST(script->priv->config_params));
> +}
> +
> +/**
> + * osinfo_install_script_get_config_paramlist:
> + *
> + * Get the list of valid config parameters for @script.
> + *
> + * Returns: (transfer none): the
> + * list of valid #OsinfoInstallConfigParam parameters. Free with
> + * g_list_free() when done. The elements are owned by libosinfo.

You want to update the doc about free function above.

> + */
> +OsinfoInstallConfigParamList *osinfo_install_script_get_config_paramlist(const OsinfoInstallScript *script)

This name seems a bit inconsistent with names for existing similar
functions. Since _get_config_param_list() is already taken, I suggest
_get_config_params.

-- 
Regards,

Zeeshan Ali (Khattak)
FSF member#5124