On Thu, Apr 11, 2013 at 5:58 PM, Fabiano Fidêncio <fidencio@xxxxxxxxxx> wrote:
> ---
I think this should be divided in 2 patches: 1. Addition of
commandline to schema/API and 2. Adding it to scripts.
Also a bit more detail in the log please and title seems to suggest
that API was already there, just not implemented before this patch.
I'd suggest 'installer: Add commandline info' or something like that.
> data/install-scripts/fedora.xml | 60 +++++++++++++++++++++++++++++++++++++++++
> data/install-scripts/rhel.xml | 52 +++++++++++++++++++++++++++++++++++
> osinfo/libosinfo.syms | 1 +
> osinfo/osinfo_install_script.c | 44 ++++++++++++++++++++++++++++--
> osinfo/osinfo_install_script.h | 4 +++
> 5 files changed, 159 insertions(+), 2 deletions(-)
>
> diff --git a/data/install-scripts/fedora.xml b/data/install-scripts/fedora.xml
> index b5b20bb..7d9d41d 100644
> --- a/data/install-scripts/fedora.xml
> +++ b/data/install-scripts/fedora.xml
> @@ -37,6 +37,29 @@
> </xsl:choose>
> </xsl:template>
>
> + <xsl:template name="script-disk">
> + <xsl:choose>
> + <xsl:when test="config/script-disk != ''">
You want to declare 'script-disk' param above in 'config' node. Same
goes for the other profile.
> + <xsl:variable name="script-disk">
> + <xsl:value-of select="config/script-disk"/>
> + </xsl:variable>
> + <xsl:value-of select="substring-after($script-disk, '/dev/')"/>
> + </xsl:when>
> + <xsl:when test="os/version > 9">
> + <!-- target-disk uses virtio -->
> + <xsl:text>sda</xsl:text>
> + </xsl:when>
> + <xsl:when test="os/version > 6">
> + <!-- target-disk uses libata IDE -->
> + <xsl:text>sdb</xsl:text>
> + </xsl:when>
> + <xsl:otherwise>
> + <!-- target-disk uses IDE -->
> + <xsl:text>hdb</xsl:text>
> + </xsl:otherwise>
> + </xsl:choose>
> + </xsl:template>
I don't think we should go on such lengths to make wild guesses. It
probably makes sense in case of target-disk (as there are typically
not many choices) but not in this case. For all we know, script-disk
could be CDROM. I'd suggest this script simply requires script-disk
param.
> <xsl:template name="rootfs">
> <xsl:choose>
> <xsl:when test="os/version > 10">
> @@ -74,6 +97,13 @@
> </xsl:choose>
> </xsl:template>
>
> + <xsl:template match="/command-line">
> + <xsl:text>ks=hd:</xsl:text>
> + <xsl:call-template name="script-disk"/>
> + <xsl:text>:/</xsl:text>
> + <xsl:value-of select="script/expected-filename"/>
> + </xsl:template>
> +
> <xsl:template match="/install-script-config">
> # Install script for <xsl:value-of select="os/short-id"/> profile <xsl:value-of select="script/profile"/>
> install
> @@ -174,6 +204,29 @@ reboot
> </xsl:choose>
> </xsl:template>
>
> + <xsl:template name="script-disk">
> + <xsl:choose>
> + <xsl:when test="config/script-disk != ''">
> + <xsl:variable name="script-disk">
> + <xsl:value-of select="config/script-disk"/>
> + </xsl:variable>
> + <xsl:value-of select="substring-after($script-disk, '/dev/')"/>
> + </xsl:when>
> + <xsl:when test="os/version > 9">
> + <!-- target-disk uses virtio -->
> + <xsl:text>sda</xsl:text>
> + </xsl:when>
> + <xsl:when test="os/version > 6">
> + <!-- target-disk uses libata IDE -->
> + <xsl:text>sdb</xsl:text>
> + </xsl:when>
> + <xsl:otherwise>
> + <!-- target-disk uses IDE -->
> + <xsl:text>hdb</xsl:text>
> + </xsl:otherwise>
> + </xsl:choose>
> + </xsl:template>
> +
> <xsl:template name="rootfs">
> <xsl:choose>
> <xsl:when test="os/version > 10">
> @@ -211,6 +264,13 @@ reboot
> </xsl:choose>
> </xsl:template>
>
> + <xsl:template match="/command-line">
> + <xsl:text>ks=hd:</xsl:text>
> + <xsl:call-template name="script-disk"/>
> + <xsl:text>:/</xsl:text>
> + <xsl:value-of select="script/expected-filename"/>
> + </xsl:template>
> +
> <xsl:template match="/install-script-config">
> # Install script for <xsl:value-of select="os/short-id"/> profile <xsl:value-of select="script/profile"/>
> install
> diff --git a/data/install-scripts/rhel.xml b/data/install-scripts/rhel.xml
> index 37cf410..35be1c2 100644
> --- a/data/install-scripts/rhel.xml
> +++ b/data/install-scripts/rhel.xml
> @@ -33,6 +33,32 @@
> </xsl:choose>
> </xsl:template>
>
> + <xsl:template name="script-disk">
> + <xsl:choose>
> + <xsl:when test="config/script-disk != ''">
> + <xsl:variable name="script-disk">
> + <xsl:value-of select="config/script-disk"/>
> + </xsl:variable>
> + <xsl:value-of select="substring-after($script-disk, '/dev/')"/>
> + </xsl:when>
> + <xsl:when test="os/version > 4">
> + <!-- target-disk uses virtio -->
> + <xsl:text>sda</xsl:text>
> + </xsl:when>
> + <xsl:otherwise>
> + <!-- target-disk uses IDE -->
> + <xsl:text>sdb</xsl:text>
> + </xsl:otherwise>
> + </xsl:choose>
> + </xsl:template>
> +
> + <xsl:template match="/command-line">
> + <xsl:text>ks=hd:</xsl:text>
> + <xsl:call-template name="script-disk"/>
> + <xsl:text>:/</xsl:text>
> + <xsl:value-of select="script/expected-filename"/>
> + </xsl:template>
> +
> <xsl:template match="/install-script-config">
> # Install script for <xsl:value-of select="os/short-id"/> profile <xsl:value-of select="script/profile"/>
> install
> @@ -119,6 +145,32 @@ reboot
> </xsl:choose>
> </xsl:template>
>
> + <xsl:template name="script-disk">
> + <xsl:choose>
> + <xsl:when test="config/script-disk != ''">
> + <xsl:variable name="script-disk">
> + <xsl:value-of select="config/script-disk"/>
> + </xsl:variable>
> + <xsl:value-of select="substring-after($script-disk, '/dev/')"/>
> + </xsl:when>
> + <xsl:when test="os/version > 4">
> + <!-- target-disk uses virtio -->
> + <xsl:text>sda</xsl:text>
> + </xsl:when>
> + <xsl:otherwise>
> + <!-- target-disk uses IDE -->
> + <xsl:text>sdb</xsl:text>
> + </xsl:otherwise>
> + </xsl:choose>
> + </xsl:template>
> +
> + <xsl:template match="/command-line">
> + <xsl:text>ks=hd:</xsl:text>
> + <xsl:call-template name="script-disk"/>
> + <xsl:text>:/</xsl:text>
> + <xsl:value-of select="script/expected-filename"/>
> + </xsl:template>
> +
> <xsl:template match="/install-script-config">
> # Install script for <xsl:value-of select="os/short-id"/> profile <xsl:value-of select="script/profile"/>
> install
> diff --git a/osinfo/libosinfo.syms b/osinfo/libosinfo.syms
> index 8fcf327..d68d36c 100644
> --- a/osinfo/libosinfo.syms
> +++ b/osinfo/libosinfo.syms
> @@ -415,6 +415,7 @@ LIBOSINFO_0.2.6 {
>
> LIBOSINFO_0.2.7 {
> osinfo_platform_get_all_devices;
> + osinfo_install_script_get_command_line;
> } LIBOSINFO_0.2.6;
>
> /* Symbols in next release...
> diff --git a/osinfo/osinfo_install_script.c b/osinfo/osinfo_install_script.c
> index 3d5fe8d..78c31f6 100644
> --- a/osinfo/osinfo_install_script.c
> +++ b/osinfo/osinfo_install_script.c
> @@ -732,6 +732,7 @@ static xmlNodePtr osinfo_install_script_generate_entity_xml(OsinfoInstallScript
> static xmlDocPtr osinfo_install_script_generate_config_xml(OsinfoInstallScript *script,
> OsinfoOs *os,
> OsinfoInstallConfig *config,
> + const gchar *searched_node,
Not a name I'd choose for this parameter. How about just 'node_name'?
> GError **error)
> {
> xmlDocPtr doc = xmlNewDoc((xmlChar *)"1.0");
> @@ -740,7 +741,7 @@ static xmlDocPtr osinfo_install_script_generate_config_xml(OsinfoInstallScript *
>
> root = xmlNewDocNode(NULL,
> NULL,
> - (xmlChar*)"install-script-config",
> + (xmlChar*)searched_node,
> NULL);
> xmlDocSetRootElement(doc, root);
>
> @@ -820,13 +821,14 @@ static gboolean osinfo_install_script_apply_template(OsinfoInstallScript *script
> OsinfoOs *os,
> const gchar *templateUri,
> const gchar *template,
> + const gchar *node,
Same here. Its the name of the node, not the node itself.
> gchar **result,
> OsinfoInstallConfig *config,
> GError **error)
> {
> gboolean ret = FALSE;
> xsltStylesheetPtr templateXsl = osinfo_install_script_load_template(templateUri, template, error);
> - xmlDocPtr configXml = osinfo_install_script_generate_config_xml(script, os, config, error);
> + xmlDocPtr configXml = osinfo_install_script_generate_config_xml(script, os, config, node, error);
>
> if (!templateXsl || !configXml)
> goto cleanup;
> @@ -871,6 +873,7 @@ static void osinfo_install_script_template_loaded(GObject *src,
> data->os,
> uri,
> input,
> + "install-script-config",
> &output,
> data->config,
> &error)) {
> @@ -915,6 +918,7 @@ void osinfo_install_script_generate_async(OsinfoInstallScript *script,
> os,
> "<data>",
> templateData,
> + "install-script-config",
> &output,
> data->config,
> &error)) {
> @@ -1266,6 +1270,42 @@ int osinfo_install_script_get_post_install_drivers_signing_req(OsinfoInstallScri
> OSINFO_DEVICE_DRIVER_SIGNING_REQ_NONE);
> }
>
> +/**
> + * osinfo_install_script_get_command_line:
I think the name should be _generate_command_line for consistency with
the other generation methods and also to inform user that this is not
just a simple getter they can keep on calling.
> + * @script: the install script
> + * @os: the os entity
> + * @config: the install script config
> + *
> + * If install script needs pass a command line to kernel, this function
> + * retrieves the command line
the -> that. I'd add a bit more detail in here. Like mention of
'direct boot' and something like "Such scripts belong to OSs that
provide paths to kernel and initrd files that are needed for direct
boot.
> + *
> + * Returns: (transfer full): The command line string, or NULL otherwise.
> + */
> +gchar *osinfo_install_script_get_command_line(OsinfoInstallScript *script,
> + OsinfoOs *os,
> + OsinfoInstallConfig *config)
> +{
> + const gchar *templateData = osinfo_install_script_get_template_data(script);
> + gchar *output = NULL;
>
> + if (templateData) {
> + GError *error = NULL;
> + if (!osinfo_install_script_apply_template(script,
> + os,
> + "<data>",
> + templateData,
> + "command-line",
> + &output,
> + config,
> + &error)) {
> + g_prefix_error(&error, "%s", _("Failed to apply script template: "));
> + }
> + }
> +
> + return output;
> +}
> +
> +
> /*
> * Local variables:
> * indent-tabs-mode: nil
> diff --git a/osinfo/osinfo_install_script.h b/osinfo/osinfo_install_script.h
> index e94c88c..7d90703 100644
> --- a/osinfo/osinfo_install_script.h
> +++ b/osinfo/osinfo_install_script.h
> @@ -193,6 +193,10 @@ gboolean osinfo_install_script_get_can_post_install_drivers(OsinfoInstallScript
> int osinfo_install_script_get_pre_install_drivers_signing_req(OsinfoInstallScript *script);
> int osinfo_install_script_get_post_install_drivers_signing_req(OsinfoInstallScript *script);
>
> +gchar *osinfo_install_script_get_command_line(OsinfoInstallScript *script,
> + OsinfoOs *os,
> + OsinfoInstallConfig *config);
> +
> #endif /* __OSINFO_INSTALL_SCRIPT_H__ */
> /*
Looks good otherwise. Thanks so much for working on this!
--
Regards,
Zeeshan Ali (Khattak)
FSF member#5124
_______________________________________________
Libosinfo mailing list
Libosinfo@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libosinfo
