[virt-tools-list] [PATCH 00/10] Install Scripts fixes and improvements
Zeeshan Ali (Khattak)
zeeshanak at gnome.org
Wed Jun 13 01:28:44 UTC 2012
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:
* 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..
* 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)
* 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
* Document id param
* osinfo_os_get_install_script_list() should take a nullable 'profile'
argument. If given, filter for app.
Zeeshan Ali (Khattak)
More information about the virt-tools-list