[virt-tools-list] [PATCH 1/4] differ expected and output name for a script

[Zeeshan Ali (Khattak)]

I think the words you are looking for are 'differenciate between' not 'differ.

On Sun, Aug 26, 2012 at 11:50 AM, Fabiano Fidêncio <fabiano at fidencio.org> wrote:
> We need to differ the expected name (name used for the installers, set
> in the XML representing each script) and the output-file name (name used
> to generate the script, may be the same than expected name; may be not,
> once we can set the prefix_output for a script)

Hmm.. how about:

"We need to differenciate between the expected filename and the output
filename. While former always remains the same (as some operating
systems expect it with a particular name), the latter is dependent on
the output prefix (set by application)."

Also, the difference is not very obvious for everyone so better add
some docs on top of both getters.

> ---
>  osinfo/libosinfo.syms          |  1 +
>  osinfo/osinfo_install_script.c | 19 +++++++++++++++++--
>  osinfo/osinfo_install_script.h |  4 +++-
>  osinfo/osinfo_loader.c         |  2 +-
>  4 files changed, 22 insertions(+), 4 deletions(-)
>
> diff --git a/osinfo/libosinfo.syms b/osinfo/libosinfo.syms
> index 6060b1d..c60f726 100644
> --- a/osinfo/libosinfo.syms
> +++ b/osinfo/libosinfo.syms
> @@ -281,6 +281,7 @@ LIBOSINFO_0.2.0 {
>         osinfo_install_script_get_template_uri;
>         osinfo_install_script_get_template_data;
>         osinfo_install_script_get_output_filename;
> +       osinfo_install_script_get_filename;

I'd name the property as 'expected-filename' (accordingly for the
struct field and this getter) and not just 'filename'.

>         osinfo_install_script_has_config_param;
>         osinfo_install_script_has_config_param_name;
>         osinfo_install_script_get_config_param_list;
> diff --git a/osinfo/osinfo_install_script.c b/osinfo/osinfo_install_script.c
> index bb2c2eb..bb4e0f3 100644
> --- a/osinfo/osinfo_install_script.c
> +++ b/osinfo/osinfo_install_script.c
> @@ -47,6 +47,7 @@ G_DEFINE_TYPE (OsinfoInstallScript, osinfo_install_script, OSINFO_TYPE_ENTITY);
>  struct _OsinfoInstallScriptPrivate
>  {
>      gchar *output_prefix;
> +    gchar *output_filename;

We don't need to keep this if we are going to create the value
on-deman only in the getter.

>      GList *config_param_list;
>  };
>
> @@ -353,10 +354,24 @@ const gchar *osinfo_install_script_get_output_prefix(OsinfoInstallScript *script
>      return script->priv->output_prefix;
>  }
>
> -const gchar *osinfo_install_script_get_output_filename(OsinfoInstallScript *script)
> +const gchar *osinfo_install_script_get_filename(OsinfoInstallScript *script)
>  {
>      return osinfo_entity_get_param_value(OSINFO_ENTITY(script),
> -                                         OSINFO_INSTALL_SCRIPT_PROP_OUTPUT_FILENAME);
> +                                         OSINFO_INSTALL_SCRIPT_PROP_FILENAME);
> +}
> +
> +const gchar *osinfo_install_script_get_output_filename(OsinfoInstallScript *script)
> +{
> +    if (script->priv->output_prefix) {
> +        g_free(script->priv->output_filename);
> +        script->priv->output_filename =
> +            g_strdup_printf("%s-%s",
> +                            script->priv->output_prefix,
> +                            osinfo_install_script_get_filename(script));

* As pointed out above, no need to keep output_filename.
* No biggie but IMHO g_strjoin would be more appropriate here.

> +        return script->priv->output_filename;
> +    }
> +
> +    return osinfo_install_script_get_filename(script);

Since you call _get_filename in all cases and its the return value if
there is no output, better call it once before the 'if' and use a
variable for it:

char *output_filename;

output_filename = g_strdup (osinfo_install_script_get_filename(script));

if (..)
   output_filename = g_strjoin ("-", script->priv->output_prefix,
output_filename, NULL);

return output_filename; // This will also mean that this function no
longer returns 'const'.

>  }
>
>  struct _OsinfoInstallScriptGenerateData {
> diff --git a/osinfo/osinfo_install_script.h b/osinfo/osinfo_install_script.h
> index cbfc517..a360098 100644
> --- a/osinfo/osinfo_install_script.h
> +++ b/osinfo/osinfo_install_script.h
> @@ -50,7 +50,7 @@ typedef struct _OsinfoInstallScriptPrivate OsinfoInstallScriptPrivate;
>  #define OSINFO_INSTALL_SCRIPT_PROP_TEMPLATE_DATA      "template-data"
>  #define OSINFO_INSTALL_SCRIPT_PROP_PROFILE            "profile"
>  #define OSINFO_INSTALL_SCRIPT_PROP_PRODUCT_KEY_FORMAT "product-key-format"
> -#define OSINFO_INSTALL_SCRIPT_PROP_OUTPUT_FILENAME    "output-filename"
> +#define OSINFO_INSTALL_SCRIPT_PROP_FILENAME           "filename"

Won't you want to update the database as well in the same patch?

>  #define OSINFO_INSTALL_SCRIPT_PROP_CONFIG_REQUIRED    "required"
>  #define OSINFO_INSTALL_SCRIPT_PROP_CONFIG_OPTIONAL    "optional"
>
> @@ -95,6 +95,8 @@ const gchar *osinfo_install_script_get_output_prefix(OsinfoInstallScript *script
>
>  const gchar *osinfo_install_script_get_output_filename(OsinfoInstallScript *script);
>
> +const gchar *osinfo_install_script_get_filename(OsinfoInstallScript *script);
> +
>  void osinfo_install_script_generate_async(OsinfoInstallScript *script,
>                                            OsinfoOs *os,
>                                            OsinfoInstallConfig *config,
> diff --git a/osinfo/osinfo_loader.c b/osinfo/osinfo_loader.c
> index c080fc4..404e5b9 100644
> --- a/osinfo/osinfo_loader.c
> +++ b/osinfo/osinfo_loader.c
> @@ -619,7 +619,7 @@ static void osinfo_loader_install_script(OsinfoLoader *loader,
>          goto error;
>      if (value)
>          osinfo_entity_set_param(OSINFO_ENTITY(installScript),
> -                                OSINFO_INSTALL_SCRIPT_PROP_OUTPUT_FILENAME,
> +                                OSINFO_INSTALL_SCRIPT_PROP_FILENAME,
>                                  value);
>      g_free(value);



-- 
Regards,

Zeeshan Ali (Khattak)
FSF member#5124