[virt-tools-list] [virt-viewer 2/7] ovirt: Add OvirtForeignMenu class
Christophe Fergeau
cfergeau at redhat.com
Thu Apr 17 14:56:07 UTC 2014
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 <berrange at redhat.com>
> > + * Christope Fergeau <cfergeau at redhat.com>
> > + */
>
> 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/virt-tools-list/attachments/20140417/246ebbb4/attachment.sig>
More information about the virt-tools-list
mailing list