[virt-tools-list] [virt-viewer 2/7] ovirt: Add OvirtForeignMenu class

[Christophe Fergeau]

On Thu, Apr 17, 2014 at 01:26:09PM +0200, Marc-André Lureau wrote:
> On Wed, Apr 16, 2014 at 6:59 PM, Christophe Fergeau <cfergeau at redhat.com>wrote:
> > +
> > +
> > +static void
> > +ovirt_foreign_menu_set_property(GObject *object, guint property_id,
> > +                                       const GValue *value G_GNUC_UNUSED,
> > GParamSpec *pspec)
> > +{
> > +    OvirtForeignMenu *self = OVIRT_FOREIGN_MENU(object);
> > +    OvirtForeignMenuPrivate *priv = self->priv;
> > +
> > +    switch (property_id) {
> > +    case PROP_PROXY:
> > +        if (priv->proxy != NULL) {
> > +            g_object_unref(priv->proxy);
> > +        }
> >
> 
> newly written code can really benefit g_clear_object() / g_clear_pointer()
> - true for all the remaining if { unref ; foo = NULL } all over.

I'm not particularly fond of g_clear_object()/g_clear_pointer() and the not
really needed atomic operations that come with them.

[snip]
> > +
> > +
> > +static void
> > +ovirt_foreign_menu_next_async_step(OvirtForeignMenu *menu,
> > +                                   OvirtForeignMenuState current_state)
> > +{
> > +    current_state++;
> >
> 
> I would add a few g_return_if_fail() check about current_state values.

You mean, check if it does not grow too small/too big?

> 
> 
> > +
> > +    if (current_state == STATE_STORAGE_DOMAIN) {
> > +        if (menu->priv->files == NULL) {
> > +            ovirt_foreign_menu_fetch_storage_domain_async(menu);
> > +        } else {
> > +            current_state++;
> > +        }
> > +    }
> > +
> > +    if (current_state == STATE_VM_CDROM) {
> > +        if (menu->priv->cdrom == NULL) {
> > +            ovirt_foreign_menu_fetch_vm_cdrom_async(menu);
> > +        } else {
> > +            current_state++;
> > +        }
> > +    }
> > +
> > +    if (current_state == STATE_ISOS) {
> > +        g_warn_if_fail(menu->priv->api != NULL);
> > +        g_warn_if_fail(menu->priv->vm != NULL);
> > +        g_warn_if_fail(menu->priv->files != NULL);
> > +        g_warn_if_fail(menu->priv->cdrom != NULL);
> > +
> > +        ovirt_foreign_menu_refresh_iso_list(menu);
> > +    }
> > +}
> > +
> > +
> > +void
> > +ovirt_foreign_menu_start(OvirtForeignMenu *menu)
> > +{
> > +    ovirt_foreign_menu_next_async_step(menu, STATE_0);
> >
> 
> Why not calling all the async simultaneously? If they need to be chained,
> isn't it simpler to do it through the normal code flow?

They need to be chained, gathering the chaining there make the order more
obvious, make it easier to add more steps to the chain, I think that's why
I went with this approach (I wrote that code a bit too long ago to remember
all the details :( .

> 
> 
> > +}
> > +
> > +
> > +static void updated_cdrom_cb(GObject *source_object,
> > +                            GAsyncResult *result,
> > +                            G_GNUC_UNUSED gpointer user_data)
> > +{
> > +    GError *error = NULL;
> > +    ovirt_cdrom_update_finish(OVIRT_CDROM(source_object),
> > +                              result, &error);
> > +    g_debug("Finished updating cdrom content");
> > +    if (error != NULL) {
> > +        g_debug("Failed to update cdrom resource: %s", error->message);
> >
> 
> doesn't this deserve a warning? (same for similar errors)

Fair enough, I've changed all _finish errors to be reported with a
g_warning().

> > +
> > +static char *
> > +ovirt_foreign_menu_get_current_iso_name(OvirtForeignMenu *foreign_menu)
> > +{
> > +    char *name;
> > +
> > +    if (foreign_menu->priv->cdrom == NULL) {
> > +        return NULL;
> > +    }
> > +
> > +    g_object_get(G_OBJECT(foreign_menu->priv->cdrom), "file", &name,
> > NULL);
> >
> 
> g_object_get()/set() don't need explicit cast.

This gives us a warning if the object is corrupt for some reason. I've
changed the various occurrences in this file.

> 
> 
> > +    return name;
> > +}
> > +
> > +
> > +GtkWidget *ovirt_foreign_menu_get_gtk_menu(OvirtForeignMenu *foreign_menu)
> > +{
> > +    GtkWidget *gtk_menu;
> > +    GList *it;
> > +    char *current_iso;
> > +
> > +    g_debug("Creating GtkMenu for foreign menu");
> > +    current_iso = ovirt_foreign_menu_get_current_iso_name(foreign_menu);
> >
> 
> Could be NULL,

I think the code handles a NULL current_iso just fine.

> 
> 
> > +    gtk_menu = gtk_menu_new();
> > +    for (it = foreign_menu->priv->iso_names; it != NULL; it = it->next) {
> > +        GtkWidget *menuitem;
> > +
> > +        menuitem = gtk_check_menu_item_new_with_label((char *)it->data);
> > +        if (g_strcmp0((char *)it->data, current_iso) == 0) {
> >
> 
> why not use safer and clearer g_str_equal() ? (same all over)

I agree with more readable, but safer? g_str_equal does not handle NULL
args, g_strcmp0 does.

> > +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));
> > +    if (error != NULL) {
> > +        g_debug("failed to fetch files for ISO storage domain: %s",
> > +                error->message);
> > +        g_clear_error(&error);
> > +        return;
> > +    } else {
> > +        ovirt_foreign_menu_set_files(OVIRT_FOREIGN_MENU(user_data),
> > files);
> > +    }
> > +    g_list_free(files);
> > +
> > +    g_timeout_add_seconds(15, ovirt_foreign_menu_refresh_iso_list,
> > user_data);
> >
> 
> Depending on the load of the server, this could be quite heavy. Is this the
> recommended value by ovirt folks?

I didn't ask them, I'll check. Hopefully at some point it will be possible
to subscribe to change notifications rather than polling ;)

Thanks for the review,

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/5169485a/attachment.sig>