[libvirt] [libvirt-designer 1/2] Fix libvirt caps -> libosinfo platform short id mapping
The code was building an id starting with kvm- while libosinfo qemu description uses qemu-kvm-. Also, starting from qemu 1.2.0, there is no separate qemu-kvm tarball. guess_platform_from_connect is starting to be a bit magic, it may be better to add a <machine> attribute to libosinfo <platform> description and to use this to improve the matching between libosinfo data and libvirt caps. --- examples/virtxml.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/examples/virtxml.c b/examples/virtxml.c index 09f49cf..a68843d 100644 --- a/examples/virtxml.c +++ b/examples/virtxml.c @@ -442,13 +442,13 @@ guess_platform_from_connect(GVirConnection *conn) } /* do some mappings: - * QEMU -> kvm + * QEMU -> qemu-kvm * Xen -> xen */ type = g_ascii_strdown(hv_type, -1); - if (g_str_equal(type, "qemu")) { + if (g_str_equal(type, "qemu") && ver <= 1002000) { g_free(type); - type = g_strdup("kvm"); + type = g_strdup("qemu-kvm"); } major = ver / 1000000; -- 1.8.1.4
virtxml was doing its own loading of the libosinfo database, and gvir_designer_init() was loading it a second time. By adding a gvir_designer_get_osinfo_db() method, virtxml can use the same libosinfo database as the rest of libvirt-designer. --- examples/virtxml.c | 51 ++++++++++++---------------- libvirt-designer/libvirt-designer-domain.c | 9 ++++- libvirt-designer/libvirt-designer-internal.h | 3 -- libvirt-designer/libvirt-designer-main.c | 39 ++++++++++++++++----- libvirt-designer/libvirt-designer-main.h | 3 ++ libvirt-designer/libvirt-designer.sym | 1 + 6 files changed, 64 insertions(+), 42 deletions(-) diff --git a/examples/virtxml.c b/examples/virtxml.c index a68843d..8137c5a 100644 --- a/examples/virtxml.c +++ b/examples/virtxml.c @@ -35,7 +35,6 @@ GList *disk_str_list = NULL; GList *iface_str_list = NULL; -OsinfoDb *db = NULL; #define print_error(...) \ print_error_impl(__FUNCTION__, __LINE__, __VA_ARGS__) @@ -59,28 +58,6 @@ print_error_impl(const char *funcname, fprintf(stderr,"\n"); } -static gboolean -load_osinfo(void) -{ - GError *err = NULL; - gboolean ret = FALSE; - OsinfoLoader *loader = NULL; - - loader = osinfo_loader_new(); - osinfo_loader_process_default_path(loader, &err); - if (err) { - print_error("Unable to load default libosinfo DB: %s", err->message); - g_clear_error(&err); - } - - db = osinfo_loader_get_db(loader); - g_object_ref(db); - ret = TRUE; - - g_object_unref(loader); - return ret; -} - static gint entity_compare(gconstpointer a, gconstpointer b) { @@ -97,12 +74,14 @@ print_oses(const gchar *option_name, gpointer data, GError **error) { + OsinfoDb *db; OsinfoOsList *list = NULL; GList *oses = NULL; GList *os_iter; int ret = EXIT_FAILURE; - if (!db && !load_osinfo()) + db = gvir_designer_get_osinfo_db(); + if (!db) goto cleanup; printf(" Operating System ID\n" @@ -137,12 +116,14 @@ print_platforms(const gchar *option_name, gpointer data, GError **error) { + OsinfoDb *db; OsinfoPlatformList *list = NULL; GList *platforms = NULL; GList *platform_iter; int ret = EXIT_FAILURE; - if (!db && !load_osinfo()) + db = gvir_designer_get_osinfo_db(); + if (!db) goto cleanup; printf(" Platform ID\n" @@ -313,9 +294,11 @@ find_entity_by_short_id(OsinfoList *ent_list, static OsinfoOs * find_os(const gchar *os_str) { + OsinfoDb *db; OsinfoOs *ret = NULL; - if (!db && !load_osinfo()) + db = gvir_designer_get_osinfo_db(); + if (!db) return NULL; ret = osinfo_db_get_os(db, os_str); @@ -326,11 +309,13 @@ find_os(const gchar *os_str) static OsinfoOs * find_os_by_short_id(const char *short_id) { + OsinfoDb *db; OsinfoOs *ret = NULL; OsinfoOsList *oses = NULL; OsinfoEntity *found = NULL; - if (!db && !load_osinfo()) + db = gvir_designer_get_osinfo_db(); + if (!db) return NULL; oses = osinfo_db_get_os_list(db); @@ -353,10 +338,12 @@ cleanup: static OsinfoOs * guess_os_from_disk(GList *disk_list) { + OsinfoDb *db; OsinfoOs *ret = NULL; GList *list_it = NULL; - if (!db && !load_osinfo()) + db = gvir_designer_get_osinfo_db(); + if (!db) return NULL; for (list_it = disk_list; list_it; list_it = list_it->next) { @@ -387,9 +374,11 @@ guess_os_from_disk(GList *disk_list) static OsinfoPlatform * find_platform(const char *platform_str) { + OsinfoDb *db; OsinfoPlatform *ret = NULL; - if (!db && !load_osinfo()) + db = gvir_designer_get_osinfo_db(); + if (!db) return NULL; ret = osinfo_db_get_platform(db, platform_str); @@ -400,11 +389,13 @@ find_platform(const char *platform_str) static OsinfoPlatform * find_platform_by_short_id(const char *short_id) { + OsinfoDb *db; OsinfoPlatform *ret = NULL; OsinfoPlatformList *platforms = NULL; OsinfoEntity *found = NULL; - if (!db && !load_osinfo()) + db = gvir_designer_get_osinfo_db(); + if (!db) goto cleanup; platforms = osinfo_db_get_platform_list(db); diff --git a/libvirt-designer/libvirt-designer-domain.c b/libvirt-designer/libvirt-designer-domain.c index 3e31bd1..862daba 100644 --- a/libvirt-designer/libvirt-designer-domain.c +++ b/libvirt-designer/libvirt-designer-domain.c @@ -721,7 +721,14 @@ gvir_designer_domain_get_preferred_device(GVirDesignerDomain *design, OsinfoDeviceLink *dev_link = NULL; if (!deployment) { - priv->deployment = deployment = osinfo_db_find_deployment(osinfo_db, + OsinfoDb *db; + db = gvir_designer_get_osinfo_db(); + if (!db) { + g_set_error(error, GVIR_DESIGNER_DOMAIN_ERROR, 0, + "Unable to find any deployment in libosinfo database"); + goto cleanup; + } + priv->deployment = deployment = osinfo_db_find_deployment(db, priv->os, priv->platform); if (!deployment) { diff --git a/libvirt-designer/libvirt-designer-internal.h b/libvirt-designer/libvirt-designer-internal.h index bbef922..e95edfc 100644 --- a/libvirt-designer/libvirt-designer-internal.h +++ b/libvirt-designer/libvirt-designer-internal.h @@ -24,7 +24,4 @@ #ifndef __LIBVIRT_DESIGNER_INTERNAL_H__ #define __LIBVIRT_DESIGNER_INTERNAL_H__ -extern OsinfoLoader *osinfo_loader; -extern OsinfoDb *osinfo_db; - #endif /* __LIBVIRT_DESIGNER_INTERNAL_H__ */ diff --git a/libvirt-designer/libvirt-designer-main.c b/libvirt-designer/libvirt-designer-main.c index 5c70b57..7d730bf 100644 --- a/libvirt-designer/libvirt-designer-main.c +++ b/libvirt-designer/libvirt-designer-main.c @@ -30,7 +30,6 @@ #include <libvirt-designer/libvirt-designer.h> #include <libvirt-gconfig/libvirt-gconfig.h> -OsinfoLoader *osinfo_loader = NULL; OsinfoDb *osinfo_db = NULL; /** @@ -58,6 +57,20 @@ static void gvir_log_handler(const gchar *log_domain G_GNUC_UNUSED, } +static void gvir_designer_load_osinfo(void) +{ + /* Init libosinfo and load databases from default paths */ + /* XXX maybe we want to let users tell a different path via + * env variable or argv */ + OsinfoLoader *osinfo_loader = NULL; + + osinfo_loader = osinfo_loader_new(); + osinfo_loader_process_default_path(osinfo_loader, NULL); + + osinfo_db = g_object_ref(osinfo_loader_get_db(osinfo_loader)); + g_object_unref(G_OBJECT(osinfo_loader)); +} + /** * gvir_designer_init_check: * @argc: (inout): pointer to application's argc @@ -85,13 +98,23 @@ gboolean gvir_designer_init_check(int *argc, gvir_log_handler, NULL); #endif - /* Init libosinfo and load databases from default paths */ - /* XXX maybe we want to let users tell a different path via - * env variable or argv */ - osinfo_loader = osinfo_loader_new(); - osinfo_loader_process_default_path(osinfo_loader, NULL); - - osinfo_db = osinfo_loader_get_db(osinfo_loader); + gvir_designer_load_osinfo(); return TRUE; } + +/** + * gvir_designer_get_osinfo_db: + * + * Retrieves the global #OsinfoDb database + * + * Returns: (transfer none): an #OsinfoDb database owned by + * libvirt-designer which must not be unreferenced + */ +OsinfoDb *gvir_designer_get_osinfo_db(void) +{ + if (osinfo_db == NULL) + gvir_designer_load_osinfo(); + + return osinfo_db; +} diff --git a/libvirt-designer/libvirt-designer-main.h b/libvirt-designer/libvirt-designer-main.h index 2500ef7..9b874a4 100644 --- a/libvirt-designer/libvirt-designer-main.h +++ b/libvirt-designer/libvirt-designer-main.h @@ -24,6 +24,8 @@ #error "Only <libvirt-gdesigner/libvirt-gdesigner.h> can be included directly." #endif +#include <osinfo/osinfo.h> + #ifndef __LIBVIRT_DESIGNER_MAIN_H__ #define __LIBVIRT_DESIGNER_MAIN_H__ @@ -34,6 +36,7 @@ void gvir_designer_init(int *argc, gboolean gvir_designer_init_check(int *argc, char ***argv, GError **err); +OsinfoDb *gvir_designer_get_osinfo_db(void); G_END_DECLS diff --git a/libvirt-designer/libvirt-designer.sym b/libvirt-designer/libvirt-designer.sym index 79db09f..6f63abc 100644 --- a/libvirt-designer/libvirt-designer.sym +++ b/libvirt-designer/libvirt-designer.sym @@ -2,6 +2,7 @@ LIBVIRT_DESIGNER_0.0.1 { global: gvir_designer_init; gvir_designer_init_check; + gvir_designer_get_osinfo_db; gvir_designer_domain_new; gvir_designer_domain_get_type; -- 1.8.1.4
On Tue, Mar 19, 2013 at 10:46:36AM +0100, Christophe Fergeau wrote:
virtxml was doing its own loading of the libosinfo database, and gvir_designer_init() was loading it a second time. By adding a gvir_designer_get_osinfo_db() method, virtxml can use the same libosinfo database as the rest of libvirt-designer. --- examples/virtxml.c | 51 ++++++++++++---------------- libvirt-designer/libvirt-designer-domain.c | 9 ++++- libvirt-designer/libvirt-designer-internal.h | 3 -- libvirt-designer/libvirt-designer-main.c | 39 ++++++++++++++++----- libvirt-designer/libvirt-designer-main.h | 3 ++ libvirt-designer/libvirt-designer.sym | 1 + 6 files changed, 64 insertions(+), 42 deletions(-)
I don't think we should be using a global variable for the osinfo database in libvirt-designer code at all. The libvirt designer APIs should allow the database to be passed by the caller where needed. This allows the app to have separate DBs for different scenarios if they so need, which could be useful in multi-threaded apps that want to avoid the need todo mutex locking on the DBs. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Tue, Mar 19, 2013 at 10:46:35AM +0100, Christophe Fergeau wrote:
The code was building an id starting with kvm- while libosinfo qemu description uses qemu-kvm-. Also, starting from qemu 1.2.0, there is no separate qemu-kvm tarball. guess_platform_from_connect is starting to be a bit magic, it may be better to add a <machine> attribute to libosinfo <platform> description and to use this to improve the matching between libosinfo data and libvirt caps.
I've never been able to think up a satisfactory way to do the mapping yet, without black magic like this. The trick is we don't want to tie libosinfo to libvirt concepts directly, since we want it to be generally useful to anyone doing virt stuff regardless of whether they use libvirt.
diff --git a/examples/virtxml.c b/examples/virtxml.c index 09f49cf..a68843d 100644 --- a/examples/virtxml.c +++ b/examples/virtxml.c @@ -442,13 +442,13 @@ guess_platform_from_connect(GVirConnection *conn) }
/* do some mappings: - * QEMU -> kvm + * QEMU -> qemu-kvm * Xen -> xen */ type = g_ascii_strdown(hv_type, -1); - if (g_str_equal(type, "qemu")) { + if (g_str_equal(type, "qemu") && ver <= 1002000) { g_free(type); - type = g_strdup("kvm"); + type = g_strdup("qemu-kvm"); }
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Tue, Mar 19, 2013 at 03:24:22PM +0000, Daniel P. Berrange wrote:
On Tue, Mar 19, 2013 at 10:46:35AM +0100, Christophe Fergeau wrote:
The code was building an id starting with kvm- while libosinfo qemu description uses qemu-kvm-. Also, starting from qemu 1.2.0, there is no separate qemu-kvm tarball. guess_platform_from_connect is starting to be a bit magic, it may be better to add a <machine> attribute to libosinfo <platform> description and to use this to improve the matching between libosinfo data and libvirt caps.
I've never been able to think up a satisfactory way to do the mapping yet, without black magic like this. The trick is we don't want to tie libosinfo to libvirt concepts directly, since we want it to be generally useful to anyone doing virt stuff regardless of whether they use libvirt.
Agreed on not tying too much libosinfo with libvirt concepts. The <machine> tag seemed acceptable from that perspective as this initially is a qemu concept, so not too deeply tied to libvirt. Alternatively, maybe we can ship some specialized datamaps to transform libosinfo short IDs to something meaningful in a libvirt/... context. Applications could pick the most appropriate one depending on what they are using. Christophe
On Tue, Mar 19, 2013 at 04:54:46PM +0100, Christophe Fergeau wrote:
On Tue, Mar 19, 2013 at 03:24:22PM +0000, Daniel P. Berrange wrote:
On Tue, Mar 19, 2013 at 10:46:35AM +0100, Christophe Fergeau wrote:
The code was building an id starting with kvm- while libosinfo qemu description uses qemu-kvm-. Also, starting from qemu 1.2.0, there is no separate qemu-kvm tarball. guess_platform_from_connect is starting to be a bit magic, it may be better to add a <machine> attribute to libosinfo <platform> description and to use this to improve the matching between libosinfo data and libvirt caps.
I've never been able to think up a satisfactory way to do the mapping yet, without black magic like this. The trick is we don't want to tie libosinfo to libvirt concepts directly, since we want it to be generally useful to anyone doing virt stuff regardless of whether they use libvirt.
Agreed on not tying too much libosinfo with libvirt concepts. The <machine> tag seemed acceptable from that perspective as this initially is a qemu concept, so not too deeply tied to libvirt. Alternatively, maybe we can ship some specialized datamaps to transform libosinfo short IDs to something meaningful in a libvirt/... context. Applications could pick the most appropriate one depending on what they are using.
Perhaps we should be following the approach we use for detecting ISOs, and match against actual product release strings. Libvirt doesn't expose such info, but we could add an API to expose the full product string and/or version string. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Tue, Mar 19, 2013 at 03:58:18PM +0000, Daniel P. Berrange wrote:
Perhaps we should be following the approach we use for detecting ISOs, and match against actual product release strings. Libvirt doesn't expose such info, but we could add an API to expose the full product string and/or version string.
Sounds like a good idea. Another way would be for libosinfo to expose some platform_name and platform_version strings with well defined formats, this would allow for the matching to less magic. This is more magic than what you suggest, but has the advantage of not requiring libvirt changes. I might give a try to the regex match approach you suggest. Christophe
participants (2)
-
Christophe Fergeau -
Daniel P. Berrange