On Mon, Dec 10, 2012 at 6:46 PM, Christophe Fergeau <cfergeau@xxxxxxxxxx> 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
_______________________________________________
virt-tools-list mailing list
virt-tools-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/virt-tools-list
