[virt-tools-list] [PATCH 1/3] Change install script output (tool)

[Zeeshan Ali (Khattak)]

On Thu, Aug 9, 2012 at 6:14 AM, Fabiano Fidêncio <fabiano at fidencio.org> wrote:
> On Wed, Aug 8, 2012 at 2:39 PM, Zeeshan Ali (Khattak)
> <zeeshanak at gnome.org> wrote:
>> On Thu, Aug 2, 2012 at 7:39 PM, Fabiano Fidêncio <fabiano at fidencio.org> wrote:

>>> @@ -279,6 +282,7 @@ LIBOSINFO_0.2.0 {
>>>         osinfo_install_scriptlist_new_intersection;
>>>         osinfo_install_scriptlist_new_copy;
>>>         osinfo_install_scriptlist_get_type;
>>> +       osinfo_install_scriptlist_get_by_profile;
>>
>> Unrelated change?
>
> It is used by osinfo-install-script tool.
> Do you think that would be better put it in another commit?

Doesn't matter whats its used by, its not related to this change so
yes, a separate patch for that please.

>>> @@ -210,6 +224,19 @@ osinfo_install_script_class_init (OsinfoInstallScriptClass *klass)
>>>                                      PROP_PROFILE,
>>>                                      pspec);
>>>
>>> +    pspec = g_param_spec_string("output-filename",
>>> +                                "Output Filename",
>>> +                                "Output filename for the script",
>>
>> You forgot to change these strings.
>
> Change or remove? :)

Yes, remove. Missed the fact that you added the "output-prefix" prop above.

>>> @@ -308,6 +359,24 @@ static void osinfo_install_script_generate_free(struct OsinfoInstallScriptGenera
>>>      g_free(data);
>>>  }
>>>
>>> +struct OsinfoInstallScriptGenerateOutput {
>>
>> Declare as 'static' and a 'Data' suffix.
>
> Should I do the same for OsinfoInstallScriptGenerate as well?

Yes.

>>
>>> +    GSimpleAsyncResult *res;
>>> +    GCancellable *cancellable;
>>> +    GError *error;
>>> +    GFile *file;
>>> +    GFileOutputStream *stream;
>>> +    gchar *output;
>>> +    gssize output_len;
>>> +    gssize output_pos;
>>> +};
>>> +
>>> +static void osinfo_install_script_generate_output_free(struct OsinfoInstallScriptGenerateOutput *data)
>>> +{
>>> +    g_object_unref(data->stream);
>>> +    g_object_unref(data->res);
>>> +    g_free(data);
>>> +}
>>> +
>>>
>>>  static xsltStylesheetPtr osinfo_install_script_load_template(const gchar *uri,
>>>                                                               const gchar *template,
>>> @@ -621,9 +690,9 @@ void osinfo_install_script_generate_async(OsinfoInstallScript *script,
>>>      }
>>>  }
>>>
>>> -gchar *osinfo_install_script_generate_finish(OsinfoInstallScript *script,
>>> -                                             GAsyncResult *res,
>>> -                                             GError **error)
>>> +static gpointer osinfo_install_script_generate_finish_common(OsinfoInstallScript *script,
>>> +                                                             GAsyncResult *res,
>>> +                                                             GError **error)
>>>  {
>>>      GSimpleAsyncResult *simple = G_SIMPLE_ASYNC_RESULT(res);
>>>
>>> @@ -635,13 +704,61 @@ gchar *osinfo_install_script_generate_finish(OsinfoInstallScript *script,
>>>      return g_simple_async_result_get_op_res_gpointer(simple);
>>>  }
>>>
>>> +gchar *osinfo_install_script_generate_finish(OsinfoInstallScript *script,
>>> +                                             GAsyncResult *res,
>>> +                                             GError **error)
>>> +{
>>> +    return osinfo_install_script_generate_finish_common(script,
>>> +                                                        res,
>>> +                                                        error);
>>> +}
>>> +
>>> +GFile *osinfo_install_script_generate_output_finish(OsinfoInstallScript *script,
>>> +                                                    GAsyncResult *res,
>>> +                                                    GError **error)
>>
>> So the function with output suffix gives you a GFile while the one w/o
>> it gives you the path? I'd suggest only having one function named
>> 'osinfo_install_script_generate' that returns the GFile. Its very easy
>> to get name from GFile if app needs that.
>
> Sorry, probably I'm missing something.
> But we are, in the both cases, returning a GFile, no?

Huh? You are returning a string from osinfo_install_script_generate()
and osinfo_install_script_generate_finish().

-- 
Regards,

Zeeshan Ali (Khattak)
FSF member#5124