Hey, Thanks for the review!
On Wed, Apr 16, 2014 at 05:25:11PM -0400, Jonathon Jongsma wrote: > > diff --git a/src/ovirt-foreign-menu.c b/src/ovirt-foreign-menu.c > > new file mode 100644 > > index 0000000..642c0ef > > --- /dev/null > > +++ b/src/ovirt-foreign-menu.c > > @@ -0,0 +1,580 @@ > > +/* > > + * Virt Viewer: A virtual machine console viewer > > + * > > + * Copyright (C) 2007-2013 Red Hat, Inc. > > + * Copyright (C) 2009-2012 Daniel P. Berrange > > + * Copyright (C) 2010 Marc-André Lureau > > + * > > + * This program is free software; you can redistribute it and/or modify > > + * it under the terms of the GNU General Public License as published by > > + * the Free Software Foundation; either version 2 of the License, or > > + * (at your option) any later version. > > + * > > + * This program is distributed in the hope that it will be useful, > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > + * GNU General Public License for more details. > > + * > > + * You should have received a copy of the GNU General Public License > > + * along with this program; if not, write to the Free Software > > + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 > > USA > > + * > > + * Author: Daniel P. Berrange <[email protected]> > > + * Christope Fergeau <[email protected]> > > + */ > > Nitpick: Are these author/copyright notices accurate? Is it new code or > copied from somewhere? At least 2014 should probably be included? > > > + > > +#include <config.h> > > + > > +#include "ovirt-foreign-menu.h" > > +#include "virt-glib-compat.h" > > + > > +#if !GLIB_CHECK_VERSION(2, 26, 0) > > +#include "gbinding.h" > > +#include "gbinding.c" > > +#endif > > gbinding doesn't seem to be used in this patch? Yup, it's gone. [snip] > > + > > +static void > > +ovirt_foreign_menu_dispose(GObject *obj) > > +{ > > + OvirtForeignMenu *self = OVIRT_FOREIGN_MENU(obj); > > + > > + g_debug("Disposing of foreign menu"); > > necessary? Use DEBUG_LOG()? Not really necessary, I've removed it, and switched the other g_debug() calls to DEBUG_LOG(). However, DEBUG_LOG() seems to imply to get debug logs one has to run virt-viewer --debug and also set G_MESSAGES_DEBUG=all as DEBUG_LOG will call g_debug()? Any idea why the double switch to enable debugging? [snip] > > + g_return_if_fail(GTK_IS_MENU(menu)); > > + /* Unselect all other menu items, we want a GtkRadioMenuItem-like > > + * behaviour, but we need to allow to have no item set > > + */ > > + children = gtk_container_get_children(GTK_CONTAINER(menu)); > > + for (it = children; it != NULL; it = it->next) { > > + if (it->data != menuitem) { > > + gtk_check_menu_item_set_active(GTK_CHECK_MENU_ITEM(it->data), > > FALSE); > > + } > > + } > > + g_list_free(children); > > + > > + if (foreign_menu->priv->cdrom != NULL) { > > Perhaps use g_return_if_fail() here so we at least get a warning message > if cdrom is NULL? It seems unexpected that we'd have a menu of options > but no cdrom... I've changed this if() to a g_return_if_fail() at the beginning of the function. > > +static void cdroms_fetched_cb(GObject *source_object, > > + GAsyncResult *result, > > + gpointer user_data) > > +{ > > + GHashTable *cdroms; > > + OvirtCollection *cdrom_collection = OVIRT_COLLECTION(source_object); > > + OvirtForeignMenu *menu = OVIRT_FOREIGN_MENU(user_data); > > + GList *cdrom_list; > > + GError *error = NULL; > > + > > + ovirt_collection_fetch_finish(cdrom_collection, result, &error); > > + if (error != NULL) { > > + g_debug("Failed to fetch cdrom collection: %s", error->message); > > + g_clear_error(&error); > > + return; > > + } > > + > > + cdroms = ovirt_collection_get_resources(cdrom_collection); > > + > > + g_warn_if_fail(g_hash_table_size(cdroms) <= 1); > > + > > + cdrom_list = g_hash_table_get_values(cdroms); > > use GHashTableIter instead? A bit less likely to leak, etc. Ok, changed. > > > + if (cdrom_list != NULL) { > > + OvirtCdrom *cdrom; > > + > > + cdrom = OVIRT_CDROM(cdrom_list->data); > > + if (menu->priv->cdrom != NULL) { > > + g_object_unref(G_OBJECT(menu->priv->cdrom)); > > + } > > What if there is more than one cdrom drive? oVirt always adds 1 cdrom drive to VMs and does not allow to add more. I've added a comment there explaining that. [snip] > > +static void storage_domains_fetched_cb(GObject *source_object, > > + GAsyncResult *result, > > + gpointer user_data) > > +{ > > + GError *error = NULL; > > + OvirtForeignMenu *menu = OVIRT_FOREIGN_MENU(user_data); > > + OvirtCollection *collection = OVIRT_COLLECTION(source_object); > > + GList *storage_domains; > > + GList *it; > > + > > + ovirt_collection_fetch_finish(collection, result, &error); > > + if (error != NULL) { > > + g_debug("failed to fetch storage domains: %s", error->message); > > + g_clear_error(&error); > > + return; > > + } > > + > > + storage_domains = > > g_hash_table_get_values(ovirt_collection_get_resources(collection)); > > GHashTableIter again? Ok, changed. > > > + for (it = storage_domains; it != NULL; it = it->next) { > > + OvirtStorageDomain *domain; > > + OvirtCollection *file_collection; > > + char *name; > > + int type; > > + > > + domain = OVIRT_STORAGE_DOMAIN(it->data); > > + g_object_get(G_OBJECT(domain), "type", &type, "name", &name, NULL); > > + g_free(name); > > Why do you get the 'name' property and then free it immediately? Not sure why, I think I was initially matching on name before noticing oVirt REST API also provides a more appropriate type. It's gone now, thanks for catching that. > > > + > > + if (type != OVIRT_STORAGE_DOMAIN_TYPE_ISO) { > > + continue; > > + } > > + > > + file_collection = ovirt_storage_domain_get_files(domain); > > + if (file_collection != NULL) { > > + if (menu->priv->files) { > > + g_object_unref(G_OBJECT(menu->priv->files)); > > + } > > + menu->priv->files = g_object_ref(G_OBJECT(file_collection)); > > extra ref or documentation error? ovirt_storage_domain_get_files() > documentation says the return value is (transfer full). Documentation error throughout libgovirt _get_ functions, I'll push a fix there. ovirt_storage_domain_get_files() is (transfer none): OvirtCollection *ovirt_storage_domain_get_files(OvirtStorageDomain *domain) { const char *href; g_return_val_if_fail(OVIRT_IS_STORAGE_DOMAIN(domain), NULL); if (domain->priv->files != NULL) return domain->priv->files; href = ovirt_resource_get_sub_collection(OVIRT_RESOURCE(domain), "files"); if (href == NULL) return NULL; domain->priv->files = ovirt_collection_new(href, "files", OVIRT_TYPE_RESOURCE, "file"); return domain->priv->files; } [snip] > > +static void iso_list_fetched_cb(GObject *source_object, > > + GAsyncResult *result, > > + gpointer user_data) > > +{ > > + OvirtCollection *collection = OVIRT_COLLECTION(source_object); > > + GList *files; > > + GError *error = NULL; > > + > > + ovirt_collection_fetch_finish(collection, result, &error); > > + files = > > g_hash_table_get_values(ovirt_collection_get_resources(collection)); > > Check for error before calling _get_resources()? also, GHashTableIter? Yeah, not sure what's up with that call before checking for errors, I've moved the files = g_hash_table_get_values... call after the if (error != NULL) block. GHashTableIter does not seem to fit easily there though. > > > + if (error != NULL) { > > + g_debug("failed to fetch files for ISO storage domain: %s", > > + error->message); > > + g_clear_error(&error); > > + return; > > 'files' is theoretically leaked here, though it'll likely be NULL since there > was an error... Yup, the old code was weird, should no longer be an issue with 'files' init moved after the error check. > > +static gboolean ovirt_foreign_menu_refresh_iso_list(gpointer user_data) > > +{ > > + OvirtForeignMenu *menu; > > + > > + g_debug("Refreshing foreign menu iso list"); > > DEBUG_LOG()? Changed as all other g_debug I could find in this file. > > > + menu = OVIRT_FOREIGN_MENU(user_data); > > + ovirt_foreign_menu_fetch_iso_list_async(menu); > > + > > + /* ovirt_foreign_menu_fetch_iso_list() will program a new call to that > > + * function when it has finished fetching the iso list > > presumably you mean iso_list_fetched_cb() here? Kind of, I think I meant ovirt_foreign_menu_fetch_iso_list_async, which does schedule such a call through iso_list_fetched_cb. I've changed the comment to mention both ;) Christophe
pgpX0APBCfNLJ.pgp
Description: PGP signature
_______________________________________________ virt-tools-list mailing list [email protected] https://www.redhat.com/mailman/listinfo/virt-tools-list
