On Mon, Oct 1, 2012 at 10:10 AM, Zeeshan Ali (Khattak)
<zeeshanak@xxxxxxxxx> wrote:
> On Mon, Oct 1, 2012 at 3:46 AM, Fabiano Fidêncio <fabiano@xxxxxxxxxxxx> wrote:
>> 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)
>
> Looks good otherwise. One thing:
>
>> diff --git a/osinfo/osinfo_install_script.c b/osinfo/osinfo_install_script.c
>> index bb2c2eb..8efe5f1 100644
>> --- a/osinfo/osinfo_install_script.c
>> +++ b/osinfo/osinfo_install_script.c
>> @@ -353,10 +353,41 @@ 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)
>> +/**
>> + * osinfo_install_script_get_expected_filename:
>> + *
>> + * Some operating systems (as Windows) expect that script filename has
>> + * particular name to work.
>> + *
>> + * Returns: (transfer none): the expected script filename
>> + */
>> +const gchar *osinfo_install_script_get_expected_filename(OsinfoInstallScript *script)
>> {
>> return osinfo_entity_get_param_value(OSINFO_ENTITY(script),
>> - OSINFO_INSTALL_SCRIPT_PROP_OUTPUT_FILENAME);
>> + OSINFO_INSTALL_SCRIPT_PROP_FILENAME);
>> +}
>> +
>> +/**
>> + * osinfo_install_script_get_output_filename:
>> + *
>> + * Some operating systems are able to use any script filename, allowing the
>> + * application to set the filename as desired. libosinfo provides this
>> + * functionality by set the expected filename's prefix using
>> + * osinfo_install_script_set_output_prefix() function.
>> + *
>> + * Returns: (transfer full): the output script filename
>> + */
>> +gchar *osinfo_install_script_get_output_filename(OsinfoInstallScript *script)
>> +{
>> + char *output_filename = osinfo_install_script_get_expected_filename(script);
>> +
>> + if (script->priv->output_prefix)
>> + output_filename = g_strjoin("-",
>> + script->priv->output_prefix,
>> + output_filename,
>> + NULL);
>> +
>> + return output_filename;
>> }
>
> If output_prefix is NULL, you are returning a const string (You must
> have gotten some warnings from gcc about this?) while this function is
> declaring to return a dynamically allocated one. It would be nice to
> keep this functions' return consistent with get_expected_filename() so
> I suggestion you keep an internal output_filename that you update
> whenever output_prefix is changed.
Oh. You're right!
I'll fix this and do explicit check for NULL (as suggested by Zesshan
on IRC) and resend with the Avatar Info stuff.
Thank you.
>
> --
> Regards,
>
> Zeeshan Ali (Khattak)
> FSF member#5124
Best Regards,
--
Fabiano Fidêncio
