[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [virt-tools-list] [PATCH 1/2] install script: generates the output in a file



On Wed, Aug 15, 2012 at 5:21 AM, Fabiano Fidêncio <fabiano fidencio org> wrote:
> On Mon, Aug 13, 2012 at 2:54 PM, Zeeshan Ali (Khattak)
> <zeeshanak gnome org> wrote:
>> On Mon, Aug 13, 2012 at 10:37 AM, Fabiano Fidêncio <fabiano fidencio org> wrote:
>>> Some functions were added to provide support for, instead of return a
>>> string, the osinfo-install-script tool writes that string in a file,
>>> that will be into a dir, passed as argument.
>>
>> Not really a native English speaker and commit a lot of mistakes
>> myself but the para above isnt very comprehensible. Would this be a
>> correct equivalent?
>>
>> Add variant of osinfo_install_script_generate() that outputs the
>> script into a file in the given directory. Also add API to specify a
>> custom prefix for generated file.
>>
>>> These functions are:
>>>     - osinfo_install_script_generate_output:
>>>         returns a GFile * instance containing the install script, itself.
>>>         This GFile * instance must be unref'd by the caller.
>>>
>>>     - osinfo_install_script_generate_output_async:
>>>         async version of the previous functions.
>>>
>>>     - osinfo_install_script_generate_output_finish:
>>>         used when the user uses async method.
>>>
>>>     - osinfo_install_script_{set,get}_output_prefix:
>>>         used to set/get a prefix that will be prepended in the output
>>>         filename. The string set is g_strdup'd and g_free'd internally,
>>>         avoiding that user cares about that.
>>
>> If you take the above description I provided than there is no need to
>> mention these details.
>>
>>> Moreover, the "filename" attribute was added in the "template" element
>>> in the install script data, and it will be used as the output filename
>>> for each script and will be more detailed below. Also, support to this
>>> field was provided in the XML schema and in the osinfo_loader as well.
>>>
>>> About the osinfo-install-script tool:
>>>     If the prefix argument is NULL, the output files will be written as
>>>     set in data/install-scripts/*.xml (in filename attribute from
>>>     template element):
>>>         - Linuxes: fedora.ks
>>>         - Windows 2k3r2 and older: windows.sif
>>>         - Windows 2k8 and newer: windows.xml
>>>     Otherwise, the prefix will be prepended in the filename as:
>>>     <prefix>-<filename>
>>>
>>>     If the dirname argument is NULL, the output files will be written in
>>>     the current directory.
>>>
>>> It will be used to create, easily, multiple scripts, as used in:
>>> http://bugzilla-attachments.gnome.org/attachment.cgi?id=214681
>>> ---
>>>  data/install-scripts/fedora.xml           |   2 +-
>>>  data/install-scripts/windows-sif.xml      |   2 +-
>>>  data/install-scripts/windows-unattend.xml |   2 +-
>>>  data/schemas/libosinfo.rng                |   1 +
>>>  osinfo/libosinfo.syms                     |   3 +
>>>  osinfo/osinfo_install_script.c            | 222 ++++++++++++++++++++++++++++--
>>>  osinfo/osinfo_install_script.h            |  24 ++++
>>>  osinfo/osinfo_loader.c                    |   9 ++
>>>  tools/osinfo-install-script.c             |  42 +++---
>>>  9 files changed, 276 insertions(+), 31 deletions(-)
>>>
>>> diff --git a/data/install-scripts/fedora.xml b/data/install-scripts/fedora.xml
>>> index b4ad72d..338a570 100644
>>> --- a/data/install-scripts/fedora.xml
>>> +++ b/data/install-scripts/fedora.xml
>>> @@ -1,7 +1,7 @@
>>>  <libosinfo version="0.0.1">
>>>    <install-script id='http://fedoraproject.org/scripts/fedora/jeos'>
>>>      <profile>jeos</profile>
>>> -    <template>
>>> +    <template filename="fedora.ks">
>>>        <xsl:stylesheet
>>>          xmlns:xsl="http://www.w3.org/1999/XSL/Transform";
>>>          version="1.0">
>>> diff --git a/data/install-scripts/windows-sif.xml b/data/install-scripts/windows-sif.xml
>>> index 29a0eae..46b24ae 100644
>>> --- a/data/install-scripts/windows-sif.xml
>>> +++ b/data/install-scripts/windows-sif.xml
>>> @@ -1,7 +1,7 @@
>>>  <libosinfo version="0.0.1">
>>>    <install-script id='http://microsoft.com/windows/sif'>
>>>      <profile>jeos</profile>
>>> -    <template>
>>> +    <template filename="windows.sif">
>>>        <xsl:stylesheet
>>>          xmlns:xsl="http://www.w3.org/1999/XSL/Transform";
>>>          version="1.0">
>>> diff --git a/data/install-scripts/windows-unattend.xml b/data/install-scripts/windows-unattend.xml
>>> index 9828b34..20e3e23 100644
>>> --- a/data/install-scripts/windows-unattend.xml
>>> +++ b/data/install-scripts/windows-unattend.xml
>>> @@ -1,7 +1,7 @@
>>>  <libosinfo version="0.0.1">
>>>    <install-script id='http://microsoft.com/windows/unattend'>
>>>      <profile>jeos</profile>
>>> -    <template>
>>> +    <template filename="windows.xml">
>>>        <xsl:stylesheet
>>>          xmlns:xsl="http://www.w3.org/1999/XSL/Transform";
>>>          version="1.0">
>>> diff --git a/data/schemas/libosinfo.rng b/data/schemas/libosinfo.rng
>>> index a5f527d..7c8d7f7 100644
>>> --- a/data/schemas/libosinfo.rng
>>> +++ b/data/schemas/libosinfo.rng
>>> @@ -414,6 +414,7 @@
>>>          <text/>
>>>        </element>
>>>        <element name='template'>
>>> +        <attribute name="filename"/>
>>>          <choice>
>>>            <group>
>>>              <attribute name="uri"/>
>>> diff --git a/osinfo/libosinfo.syms b/osinfo/libosinfo.syms
>>> index 2f90183..ddf736e 100644
>>> --- a/osinfo/libosinfo.syms
>>> +++ b/osinfo/libosinfo.syms
>>> @@ -264,6 +264,7 @@ LIBOSINFO_0.2.0 {
>>>         osinfo_install_config_set_user_administrator;
>>>         osinfo_install_config_set_user_autologin;
>>>         osinfo_install_config_set_hostname;
>>> +       osinfo_install_script_set_output_prefix;
>>>         osinfo_install_script_get_type;
>>>         osinfo_install_script_new;
>>>         osinfo_install_script_new_data;
>>> @@ -271,6 +272,8 @@ LIBOSINFO_0.2.0 {
>>>         osinfo_install_script_generate;
>>>         osinfo_install_script_generate_async;
>>>         osinfo_install_script_generate_finish;
>>> +       osinfo_install_script_generate_output;
>>> +       osinfo_install_script_generate_output_async;
>>>         osinfo_install_script_get_profile;
>>>         osinfo_install_script_get_uri;
>>>         osinfo_install_scriptlist_new;
>>> diff --git a/osinfo/osinfo_install_script.c b/osinfo/osinfo_install_script.c
>>> index 1f437a1..2d0b440 100644
>>> --- a/osinfo/osinfo_install_script.c
>>> +++ b/osinfo/osinfo_install_script.c
>>> @@ -46,7 +46,7 @@ G_DEFINE_TYPE (OsinfoInstallScript, osinfo_install_script, OSINFO_TYPE_ENTITY);
>>>
>>>  struct _OsinfoInstallScriptPrivate
>>>  {
>>> -    gboolean unused;
>>> +    gchar *output_prefix;
>>>  };
>>>
>>>  enum {
>>> @@ -141,6 +141,9 @@ osinfo_os_get_property(GObject    *object,
>>>  static void
>>>  osinfo_install_script_finalize (GObject *object)
>>>  {
>>> +    OsinfoInstallScript *script = OSINFO_INSTALL_SCRIPT (object);
>>> +    g_free(script->priv->output_prefix);
>>> +
>>>      /* Chain up to the parent class */
>>>      G_OBJECT_CLASS (osinfo_install_script_parent_class)->finalize (object);
>>>  }
>>> @@ -219,6 +222,7 @@ osinfo_install_script_init (OsinfoInstallScript *list)
>>>      OsinfoInstallScriptPrivate *priv;
>>>      list->priv = priv = OSINFO_INSTALL_SCRIPT_GET_PRIVATE(list);
>>>
>>> +    list->priv->output_prefix = NULL;
>>
>> This is redundant. This struct is zeroed for you.
>
> It's not an error, it's just be careful in excess. :)
> Where am I zeroing the struct? Should I remove this? Is not preferable
> to avoid an error if the user doesn't zero the struct?

There is no error possiblities here. All structs allocated by glib for
you are also zeroed out for you.

>>>  }
>>>
>>>
>>> @@ -291,7 +295,25 @@ const gchar *osinfo_install_script_get_product_key_format(OsinfoInstallScript *s
>>>                                           OSINFO_INSTALL_SCRIPT_PROP_PRODUCT_KEY_FORMAT);
>>>  }
>>>
>>> -struct OsinfoInstallScriptGenerate {
>>> +void osinfo_install_script_set_output_prefix(OsinfoInstallScript *script,
>>> +                                             const gchar *prefix)
>>> +{
>>> +    g_free(script->priv->output_prefix);
>>> +    script->priv->output_prefix = g_strdup(prefix);
>>> +}
>>> +
>>> +const gchar *osinfo_install_script_get_output_prefix(OsinfoInstallScript *script)
>>> +{
>>> +    return script->priv->output_prefix;
>>> +}
>>> +
>>> +static const gchar *osinfo_install_script_get_output_filename(OsinfoInstallScript *script)
>>> +{
>>> +    return osinfo_entity_get_param_value(OSINFO_ENTITY(script),
>>> +                                         OSINFO_INSTALL_SCRIPT_PROP_OUTPUT_FILENAME);
>>> +}
>>> +
>>> +struct OsinfoInstallScriptGenerateData {
>>>      GSimpleAsyncResult *res;
>>>      OsinfoOs *os;
>>>      OsinfoInstallConfig *config;
>>> @@ -299,7 +321,7 @@ struct OsinfoInstallScriptGenerate {
>>>  };
>>>
>>>
>>> -static void osinfo_install_script_generate_free(struct OsinfoInstallScriptGenerate *data)
>>> +static void osinfo_install_script_generate_free(struct OsinfoInstallScriptGenerateData *data)
>>
>> * This should be a separate change as it has nothing to do with this
>> patch's goal (and before this patch). Same goes for all other hunks
>> for this renaming..
>> * the free function needs to be renamed too:
>> osinfo_install_script_generate_data_free
>>
>>>  {
>>>      g_object_unref(data->os);
>>>      g_object_unref(data->config);
>>> @@ -308,6 +330,24 @@ static void osinfo_install_script_generate_free(struct OsinfoInstallScriptGenera
>>>      g_free(data);
>>>  }
>>>
>>> +struct OsinfoInstallScriptGenerateOutputData {
>>> +    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 OsinfoInstallScriptGenerateOutputData *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,
>>> @@ -530,7 +570,7 @@ static void osinfo_install_script_template_loaded(GObject *src,
>>>                                                    GAsyncResult *res,
>>>                                                    gpointer user_data)
>>>  {
>>> -    struct OsinfoInstallScriptGenerate *data = user_data;
>>> +    struct OsinfoInstallScriptGenerateData *data = user_data;
>>>      GError *error = NULL;
>>>      gchar *input = NULL;
>>>      gchar *output = NULL;
>>> @@ -579,7 +619,7 @@ void osinfo_install_script_generate_async(OsinfoInstallScript *script,
>>>                                            GAsyncReadyCallback callback,
>>>                                            gpointer user_data)
>>>  {
>>> -    struct OsinfoInstallScriptGenerate *data = g_new0(struct OsinfoInstallScriptGenerate, 1);
>>> +    struct OsinfoInstallScriptGenerateData *data = g_new0(struct OsinfoInstallScriptGenerateData, 1);
>>>      const gchar *templateData = osinfo_install_script_get_template_data(script);
>>>      const gchar *templateUri = osinfo_install_script_get_template_uri(script);
>>>
>>> @@ -621,9 +661,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 +675,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)
>>> +{
>>> +    return osinfo_install_script_generate_finish_common(script,
>>> +                                                        res,
>>> +                                                        error);
>>> +}
>>>
>>>  struct OsinfoInstallScriptGenerateSync {
>>>      GMainLoop *loop;
>>>      GError *error;
>>>      gchar *output;
>>> +    GFile *file;
>>>  };
>>>
>>> +static void osinfo_install_script_generate_output_done(GObject *src,
>>> +                                                       GAsyncResult *res,
>>> +                                                       gpointer user_data)
>>> +{
>>> +    struct OsinfoInstallScriptGenerateSync *data = user_data;
>>> +
>>> +    data->file =
>>> +        osinfo_install_script_generate_output_finish(OSINFO_INSTALL_SCRIPT(src),
>>> +                                                     res,
>>> +                                                     &data->error);
>>> +    g_main_loop_quit(data->loop);
>>> +}
>>> +
>>> +static void osinfo_install_script_generate_output_close_file(GObject *src,
>>> +                                                            GAsyncResult *res,
>>> +                                                            gpointer user_data)
>>> +{
>>> +    struct OsinfoInstallScriptGenerateOutputData *data = user_data;
>>> +
>>> +    g_output_stream_close_finish(G_OUTPUT_STREAM(src),
>>> +                                 res,
>>> +                                 &data->error);
>>> +
>>> +    g_simple_async_result_set_op_res_gpointer(data->res,
>>> +                                              data->file, NULL);
>>> +    g_simple_async_result_complete_in_idle(data->res);
>>> +
>>> +    osinfo_install_script_generate_output_free(data);
>>> +}
>>> +
>>>  static void osinfo_install_script_generate_done(GObject *src,
>>>                                                  GAsyncResult *res,
>>>                                                  gpointer user_data)
>>> @@ -655,7 +743,6 @@ static void osinfo_install_script_generate_done(GObject *src,
>>>      g_main_loop_quit(data->loop);
>>>  }
>>>
>>> -
>>>  gchar *osinfo_install_script_generate(OsinfoInstallScript *script,
>>>                                        OsinfoOs *os,
>>>                                        OsinfoInstallConfig *config,
>>> @@ -665,7 +752,7 @@ gchar *osinfo_install_script_generate(OsinfoInstallScript *script,
>>>      GMainLoop *loop = g_main_loop_new(g_main_context_get_thread_default(),
>>>                                        TRUE);
>>>      struct OsinfoInstallScriptGenerateSync data = {
>>> -        loop, NULL, NULL
>>> +        loop, NULL, NULL, NULL
>>
>> If all you need to do is to pass the main loop, you should pass that
>> as the data pointer and no need to have a separate struct.
>
> We are keeping the result (the GFile * or the gchar * that contains
> the script) in this struct as well.
> Should I remove?

Yeah I don't see any need as you get the result when you call the
finalize in this same function.

-- 
Regards,

Zeeshan Ali (Khattak)
FSF member#5124


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]