Hi Fabiano, On Tue, Jun 12, 2012 at 5:23 AM, Fabiano Fidêncio <fabiano@xxxxxxxxxxxx> 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