[libvirt] [libvirt-designer][PATCH v2 2/4] virtxml: Detect OS from given ISO

[Christophe Fergeau]

Hey,

Looks good, a few cosmetic comments below,

On Thu, Sep 13, 2012 at 04:04:29PM +0200, Michal Privoznik wrote:
> In some cases telling OS version is redundant as ISO image
> with specified OS is passed some arguments later as disk.
> Don't require --os then.
> ---
>  examples/virtxml.c |  110 +++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 files changed, 105 insertions(+), 5 deletions(-)
> 
> diff --git a/examples/virtxml.c b/examples/virtxml.c
> index b0c3a77..4ec9cd8 100644
> --- a/examples/virtxml.c
> +++ b/examples/virtxml.c
> @@ -293,6 +293,98 @@ add_iface_str(const gchar *option_name,
>      return TRUE;
>  }
>  
> +static OsinfoOs *
> +find_os(const gchar *os_str)
> +{
> +    OsinfoDb *db = get_default_osinfo_db();
> +    OsinfoOs *ret = NULL;
> +
> +    if (!db)
> +        return NULL;
> +
> +    ret = osinfo_db_get_os(db, os_str);
> +
> +    return ret;
> +}
> +
> +static OsinfoOs *
> +find_os_by_short_id(const char *short_id)
> +{
> +    OsinfoDb *db = get_default_osinfo_db();
> +    OsinfoOs *ret = NULL;
> +    OsinfoOsList *oses = NULL;
> +    GList *list, *list_iterator;

I generally go with the much shorter 'it' instead of list_iterator, no need
to change if you prefer the more explicit version

> +
> +    if (!db)
> +        return NULL;
> +
> +    oses = osinfo_db_get_os_list(db);
> +
> +    if (!oses)
> +        goto cleanup;
> +
> +    list = osinfo_list_get_elements(OSINFO_LIST(oses));
> +    for (list_iterator = list; list_iterator; list_iterator = list_iterator->next) {
> +        const char *id = osinfo_entity_get_param_value(list_iterator->data,
> +                                                       OSINFO_PRODUCT_PROP_SHORT_ID);
> +
> +        if (id && g_str_equal(id, short_id)) {

you can use g_strcmp0 here to avoid testing 'id'.

> +            ret = OSINFO_OS(list_iterator->data);
> +            g_object_ref(ret);
> +            break;
> +        }
> +    }
> +
> +    g_object_unref(oses);
> +
> +cleanup:
> +    return ret;
> +}
> +
> +static OsinfoOs *
> +guess_os_from_disk(GList *disk_list)
> +{
> +    OsinfoOs *ret = NULL;
> +    GList *tmp = g_list_first(disk_list);
> +    OsinfoLoader *loader = osinfo_loader_new();
> +    OsinfoDb *db;
> +
> +    osinfo_loader_process_default_path(loader, NULL);
> +    db = osinfo_loader_get_db(loader);
> +
> +    while (tmp) {

I much prefer the for loop you use for a similar purpose in
find_os_by_short_id, especially with the use of 'continue' in this loop.

> +        char *path = (char *) tmp->data;
> +        char *sep = strchr(path, ',');
> +        OsinfoMedia *media = NULL;
> +        OsinfoMedia *matched_media = NULL;
> +
> +        if (sep) {
> +            path = g_strndup(path, sep-path);
> +        }

the {} with the 1 line block are inconsistent with what is done in the rest
of the patch.

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20120914/26f50a41/attachment-0001.sig>