[virt-tools-list] [PATCH 00/10] Install Scripts fixes and improvements

[Zeeshan Ali (Khattak)]

Hi Fabiano,

On Tue, Jun 12, 2012 at 5:23 AM, Fabiano Fidêncio <fabiano at fidencio.org> wrote:
> These patches are a set of little corrections and some improvements in install-script stuffs (so, it's based on danpb's branch dedicated to install-script), allowing, then, Gnome Boxes to drop current schema to do unattended installations in favor to use this install-script API.
>
> As talked with danpb in #boxes, the whole serie is being send (some of them resend) to facilitate the review.

Good stuff! I finally managed to look into your work (and had a fresh
look at danpb's work at the same). It already looks and works pretty
good. I'm very happy with your progress, keep it up. While looking
into stuff, I noted down some observations. I was more interested in
the result so they are targeted at both your and danpb's patches:

* rebase was needed on top of current git master but it was simple:
https://gitorious.org/libosinfo/libosinfo/commits/fidencio-wip

* desktop profile was missing for fedora16, added that as part of
rebase along with fedora17.

*  generated files have empty lines (above and below) and trailing
whitespaces. Don't know if it'll cause any problem with any OS but
better be safe than sorry..

* osinfo-install-script
 * In --help output, we probably want to tell OS ID is needed as argument
 * We use GList everywhere in the code so no need to use GSList
 * Would be nice to have a commandline option to list all available
keys (config paramaters)
 * script_file_name_get
   * Better name: script_file_get_name
   * We don't want any os-specifics in apps and this function does
some very specific hard-coding.
   * It sets a global variable, while it can just return that value to
caller and that value could be passed around.
 * !(strcmp(..)) -> strcmp(..) == 0
 * This code:

   gsize len = sizeof(distro) + sizeof(".ks");
   gchar *output = g_malloc(len);
   g_snprintf(output, len, "%s.ks", distro);

   can be replaced by:

   gchar *output = g_strjoin(".", distro, "ks", NULL);

 * profileScripts -> profile_scripts
 * 'idoruri' param of find_os() is really hard to figure w/o '_' in the name.

* Seems we are doing a lot of repetition of 'installer' nodes in os
xml files. This is one place we can make use of inheritance.

* General comment on commit log messages:
   * Summary line is ideally < 50 chars but somewhere around 50 is ok
   * Description is indented (its not supposed to be) and it should be
< 74 cols (no exception in this case as there is no limit on number of
lines)

* osinfo_install_config_new
  * Document id param

* osinfo_os_get_install_script_list() should take a nullable 'profile'
argument. If given, filter for app.

-- 
Regards,

Zeeshan Ali (Khattak)
FSF member#5124