[Libosinfo] [PATCH 6/6] Apply datamap to config parameters when generating install script

[Daniel P. Berrange]

On Wed, Jan 09, 2013 at 06:28:23PM +0200, Zeeshan Ali (Khattak) wrote:
> On Wed, Jan 9, 2013 at 2:37 PM, Daniel P. Berrange <berrange at redhat.com> wrote:
> > From: "Daniel P. Berrange" <berrange at redhat.com>
> >
> > When creating the XML to use in the install script XSL transform,
> > apply any datamap associated with the config parameters.
> 
> Looks good. Some minor concerns below. I assume you tested it?
> 
> > ---
> >  osinfo/osinfo_install_script.c | 72 ++++++++++++++++++++++++++++++++++++++----
> >  1 file changed, 66 insertions(+), 6 deletions(-)
> >
> > diff --git a/osinfo/osinfo_install_script.c b/osinfo/osinfo_install_script.c
> > index b82c0f9..47be667 100644
> > --- a/osinfo/osinfo_install_script.c
> > +++ b/osinfo/osinfo_install_script.c
> > @@ -603,7 +603,59 @@ static xsltStylesheetPtr osinfo_install_script_load_template(const gchar *uri,
> >      return xslt;
> >  }
> >
> > -static xmlNodePtr osinfo_install_script_generate_entity_config(OsinfoInstallConfig *config,
> > +
> > +static OsinfoDatamap *
> > +osinfo_install_script_get_param_datamap(OsinfoInstallScript *script,
> > +                                        const gchar *param_name)
> > +{
> > +    OsinfoEntity *entity;
> > +    OsinfoInstallConfigParam *param;
> > +
> > +    if (!script->priv->config_params)
> > +        return NULL;
> > +
> > +    entity = osinfo_list_find_by_id(OSINFO_LIST(script->priv->config_params),
> > +                                    param_name);
> > +    if (entity == NULL) {
> > +        g_debug("%s is not a known parameter for this config", param_name);
> > +        return NULL;
> > +    }
> > +
> > +    param = OSINFO_INSTALL_CONFIG_PARAM(entity);
> > +    return osinfo_install_config_param_get_value_map(param);
> > +}
> > +
> > +
> > +static GList *
> > +osinfo_install_script_get_param_value_list(OsinfoInstallScript *script,
> > +                                           OsinfoInstallConfig *config,
> > +                                           const gchar *key)
> > +{
> > +    GList *values;
> > +    GList *it;
> > +    OsinfoDatamap *map;
> > +
> > +    values = osinfo_entity_get_param_value_list(OSINFO_ENTITY(config), key);
> > +    if (values == NULL)
> > +        return NULL;
> > +
> > +    map = osinfo_install_script_get_param_datamap(script, key);
> > +    if (map != NULL) {
> > +        for (it = values; it != NULL; it = it->next) {
> > +            const char *transformed_value;
> > +            transformed_value = osinfo_datamap_lookup(map, it->data);
> > +            if (transformed_value == NULL) {
> > +                continue;
> > +            }
> > +            it->data = (gpointer)transformed_value;
> 
> Is the cast really needed?

Shouldn't be - anything casts to/from void * in C.

> > +        }
> > +    }
> > +
> > +    return values;
> > +}
> > +
> > +
> > +static xmlNodePtr osinfo_install_script_generate_entity_config(OsinfoInstallScript *script,
> 
> The name was already a bit misleading IMHO but now that it doesn't
> take a 'config', its seems even more misleading. Could we name it to
> osinfo_install_script_generate_entity_xml instead?

Ok


Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|